RESOLVED - issue BATCH-572: Retryable exceptions cannot be skippable

* Deprecated StatefulRetryStepFactoryBean
* Added generic cache limit property to SkipLimitStepFactoryBean (replacing skipCacheLimit)
* Testing

spring-batch-core/src/main/java/org/springframework/batch/core/step/AbstractStep.java

@@ -180,7 +180,7 @@ public abstract class AbstractStep implements Step, InitializingBean, BeanNameAw
 		}
 		catch (Throwable e) {
 
-			logger.error("Encountered an error executing the step");
+			logger.error("Encountered an error executing the step: "+e.getClass()+": "+e.getMessage());
 			stepExecution.setStatus(determineBatchStatus(e));
 			exitStatus = getDefaultExitStatusForFailure(e);
 

spring-batch-core/src/main/java/org/springframework/batch/core/step/item/SimpleStepFactoryBean.java

@@ -40,7 +40,6 @@ import org.springframework.util.Assert;
  * appropriate subclass of this factory bean to configure skip or retry.
  * 
  * @see SkipLimitStepFactoryBean
- * @see StatefulRetryStepFactoryBean
  * 
  * @author Dave Syer
  * 

spring-batch-core/src/main/java/org/springframework/batch/core/step/item/SkipLimitStepFactoryBean.java

@@ -23,6 +23,7 @@ import org.springframework.batch.retry.RetryOperations;
 import org.springframework.batch.retry.backoff.BackOffPolicy;
 import org.springframework.batch.retry.callback.RecoveryRetryCallback;
 import org.springframework.batch.retry.policy.ExceptionClassifierRetryPolicy;
+import org.springframework.batch.retry.policy.MapRetryContextCache;
 import org.springframework.batch.retry.policy.NeverRetryPolicy;
 import org.springframework.batch.retry.policy.RecoveryCallbackRetryPolicy;
 import org.springframework.batch.retry.policy.SimpleRetryPolicy;
@@ -43,7 +44,6 @@ import org.springframework.batch.support.SubclassExceptionClassifier;
  * {@link #setNoRollbackForExceptionClasses(Class[])} list.
  * 
  * @see SimpleStepFactoryBean
- * @see StatefulRetryStepFactoryBean
  * 
  * @author Dave Syer
  * @author Robert Kasanicky
@@ -61,8 +61,7 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 
 	private ItemKeyGenerator itemKeyGenerator;
 
-	// TODO: build this into the retry policy
-	private int skipCacheCapacity = 1024;
+	private int cacheCapacity = 0;
 
 	private int retryLimit;
 
@@ -81,6 +80,27 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 		this.retryLimit = retryLimit;
 	}
 
+	/**
+	 * Public setter for the capacity of the cache in the retry policy. If more
+	 * items than this fail without being skipped or recovered an exception will
+	 * be thrown. This is to guard against inadvertent infinite loops generated
+	 * by item identity problems. If a large number of items are failing and not
+	 * being recognized as skipped, it usually signals a problem with the key
+	 * generation (often equals and hashCode in the item itself). So it is
+	 * better to enforce a strict limit than have weird looking errors, where a
+	 * skip limit is reached without anything being skipped.<br/>
+	 * 
+	 * The default value should be high enough and more for most purposes. To
+	 * breach the limit in a single-threaded step typically you have to have
+	 * this many failures in a single transaction. Defaults to the value in the
+	 * {@link MapRetryContextCache}.
+	 * 
+	 * @param cacheCapacity the cacheCapacity to set
+	 */
+	public void setCacheCapacity(int cacheCapacity) {
+		this.cacheCapacity = cacheCapacity;
+	}
+
 	/**
 	 * Public setter for the Class[].
 	 * @param retryableExceptionClasses the retryableExceptionClasses to set
@@ -149,24 +169,6 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 		this.itemKeyGenerator = itemKeyGenerator;
 	}
 
-	/**
-	 * Public setter for the capacity of the skipped item cache. If a large
-	 * number of items are failing and not being recognized as skipped, it
-	 * usually signals a problem with the key generation (often equals and
-	 * hashCode in the item itself). So it is better to enforce a strict limit
-	 * than have weird looking errors, where a skip limit is reached without
-	 * anything being skipped.<br/>
-	 * 
-	 * The default value is 1024 which should be high enough and more for most
-	 * purposes. To breach the limit in a single-threaded step typically you
-	 * have to have this many failures in a single transaction.
-	 * 
-	 * @param skipCacheCapacity the capacity to set
-	 */
-	public void setSkipCacheCapacity(int skipCacheCapacity) {
-		this.skipCacheCapacity = skipCacheCapacity;
-	}
-
 	/**
 	 * Skippable noRollbackForExceptionClasses will *not* cause transaction
 	 * rollback.
@@ -192,7 +194,7 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 			addFatalExceptionIfMissing(RetryException.class);
 
 			SimpleRetryPolicy simpleRetryPolicy = new SimpleRetryPolicy(retryLimit);
-			if (retryableExceptionClasses.length>0) { // otherwise we retry
+			if (retryableExceptionClasses.length > 0) { // otherwise we retry
 				// all exceptions
 				simpleRetryPolicy.setRetryableExceptionClasses(retryableExceptionClasses);
 			}
@@ -218,6 +220,9 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 
 			RecoveryCallbackRetryPolicy recoveryCallbackRetryPolicy = new RecoveryCallbackRetryPolicy(retryPolicy);
 			recoveryCallbackRetryPolicy.setRecoverableExceptionClasses(noRollbackForExceptionClasses);
+			if (cacheCapacity > 0) {
+				recoveryCallbackRetryPolicy.setRetryContextCache(new MapRetryContextCache(cacheCapacity));
+			}
 
 			RetryTemplate retryTemplate = new RetryTemplate();
 			if (retryListeners != null) {

spring-batch-core/src/main/java/org/springframework/batch/core/step/item/StatefulRetryStepFactoryBean.java

@@ -39,6 +39,9 @@ import org.springframework.batch.retry.RetryPolicy;
  * 
  * @author Dave Syer
  * 
+ * @deprecated All the features of this factory bean were moved to
+ * {@link SkipLimitStepFactoryBean} in version 1.1.0.
+ * 
  */
 public class StatefulRetryStepFactoryBean extends SkipLimitStepFactoryBean {
 

spring-batch-core/src/test/java/org/springframework/batch/core/step/item/StatefulRetryStepFactoryBeanTests.java

@@ -37,11 +37,14 @@ import org.springframework.batch.core.repository.dao.MapJobInstanceDao;
 import org.springframework.batch.core.repository.dao.MapStepExecutionDao;
 import org.springframework.batch.core.repository.support.SimpleJobRepository;
 import org.springframework.batch.core.step.AbstractStep;
+import org.springframework.batch.core.step.skip.SkipLimitExceededException;
+import org.springframework.batch.item.AbstractItemReader;
 import org.springframework.batch.item.AbstractItemWriter;
 import org.springframework.batch.item.ItemReader;
 import org.springframework.batch.item.ItemWriter;
 import org.springframework.batch.item.support.ListItemReader;
 import org.springframework.batch.retry.RetryException;
+import org.springframework.batch.retry.policy.RetryCacheCapacityExceededException;
 import org.springframework.batch.support.transaction.ResourcelessTransactionManager;
 import org.springframework.batch.support.transaction.TransactionAwareProxyFactory;
 import org.springframework.transaction.support.TransactionSynchronizationManager;
@@ -54,7 +57,7 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 
 	protected final Log logger = LogFactory.getLog(getClass());
 
-	private StatefulRetryStepFactoryBean factory = new StatefulRetryStepFactoryBean();
+	private SkipLimitStepFactoryBean factory = new SkipLimitStepFactoryBean();
 
 	private List recovered = new ArrayList();
 
@@ -215,4 +218,78 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 		// terminator
 		assertEquals(17, count);
 	}
+
+	public void testRetryWithNoSkip() throws Exception {
+		factory.setRetryableExceptionClasses(new Class[] { Exception.class });
+		factory.setRetryLimit(4);
+		factory.setSkipLimit(0);
+		List items = TransactionAwareProxyFactory.createTransactionalList();
+		items.addAll(Arrays.asList(new String[] { "b" }));
+		ItemReader provider = new ListItemReader(items) {
+			public Object read() {
+				Object item = super.read();
+				count++;
+				return item;
+			}
+		};
+		ItemWriter itemWriter = new AbstractItemWriter() {
+			public void write(Object item) throws Exception {
+				logger.debug("Write Called! Item: [" + item + "]");
+				throw new RuntimeException("Write error - planned but retryable.");
+			}
+		};
+		factory.setItemReader(provider);
+		factory.setItemWriter(itemWriter);
+		AbstractStep step = (AbstractStep) factory.getObject();
+
+		StepExecution stepExecution = new StepExecution(step.getName(), jobExecution);
+		try {
+			step.execute(stepExecution);
+			fail("Expected SkipLimitExceededException");
+		}
+		catch (SkipLimitExceededException e) {
+			// expected
+		}
+
+		assertEquals(0, stepExecution.getSkipCount());
+		// b is processed 4 times plus the null at end
+		assertEquals(5, count);
+	}
+
+	public void testCacheLimitWithRetry() throws Exception {
+		factory.setRetryableExceptionClasses(new Class[] { Exception.class });
+		factory.setRetryLimit(2);
+		// set the cache limit lower than the number of unique un-recovered
+		// errors expected
+		factory.setCacheCapacity(2);
+		ItemReader provider = new AbstractItemReader() {
+			public Object read() {
+				Object item = new Object();
+				count++;
+				return item;
+			}
+		};
+		ItemWriter itemWriter = new AbstractItemWriter() {
+			public void write(Object item) throws Exception {
+				logger.debug("Write Called! Item: [" + item + "]");
+				throw new RuntimeException("Write error - planned but retryable.");
+			}
+		};
+		factory.setItemReader(provider);
+		factory.setItemWriter(itemWriter);
+		AbstractStep step = (AbstractStep) factory.getObject();
+
+		StepExecution stepExecution = new StepExecution(step.getName(), jobExecution);
+		try {
+			step.execute(stepExecution);
+			fail("Expected RetryCacheCapacityExceededException");
+		}
+		catch (RetryCacheCapacityExceededException e) {
+			// expected
+		}
+
+		assertEquals(0, stepExecution.getSkipCount());
+		// 2 processed and cached, 3rd barfed because cache was full
+		assertEquals(3, count);
+	}
 }

spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/RetryContextCache.java

@@ -19,7 +19,7 @@ package org.springframework.batch.retry.policy;
 import org.springframework.batch.retry.RetryContext;
 
 /**
- * Simple map-like bstraction for stateful retry policies to use when storing
+ * Simple map-like abstraction for stateful retry policies to use when storing
  * and retrieving {@link RetryContext} instances.
  * 
  * @author Dave Syer