OPEN - issue BATCH-777: Parametrise RetryCallback and related interfaces

Revert RetryPolicy.close() to its former signature

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

@@ -49,9 +49,8 @@ public interface RetryPolicy {
 	/**
 	 * @param context a retry status created by the
 	 * {@link #open(RetryContext)} method of this manager.
-	 * @param succeeded true if the retry callback succeeded
 	 */
-	void close(RetryContext context, boolean succeeded);
+	void close(RetryContext context);
 
 	/**
 	 * Called once per retry attempt, after the callback fails.

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

@@ -66,15 +66,15 @@ public class CompositeRetryPolicy implements RetryPolicy {
 	 * created. If any of them fails to close the exception is propagated (and
 	 * those later in the chain are closed before re-throwing).
 	 * 
-	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean)
+	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext)
 	 */
-	public void close(RetryContext context, boolean succeeded) {
+	public void close(RetryContext context) {
 		RetryContext[] contexts = ((CompositeRetryContext) context).contexts;
 		RetryPolicy[] policies = ((CompositeRetryContext) context).policies;
 		RuntimeException exception = null;
 		for (int i = 0; i < contexts.length; i++) {
 			try {
-				policies[i].close(contexts[i], succeeded);
+				policies[i].close(contexts[i]);
 			}
 			catch (RuntimeException e) {
 				if (exception==null) {
@@ -105,7 +105,7 @@ public class CompositeRetryPolicy implements RetryPolicy {
 	 * Delegate to the policies that were in operation when the context was
 	 * created.
 	 * 
-	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean)
+	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext)
 	 */
 	public void registerThrowable(RetryContext context, Exception throwable) {
 		RetryContext[] contexts = ((CompositeRetryContext) context).contexts;

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

@@ -79,11 +79,11 @@ public class ExceptionClassifierRetryPolicy implements RetryPolicy {
 	/**
 	 * Delegate to the policy currently activated in the context.
 	 * 
-	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean)
+	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext)
 	 */
-	public void close(RetryContext context, boolean succeeded) {
+	public void close(RetryContext context) {
 		RetryPolicy policy = (RetryPolicy) context;
-		policy.close(context, succeeded);
+		policy.close(context);
 	}
 
 	/**
@@ -136,10 +136,10 @@ public class ExceptionClassifierRetryPolicy implements RetryPolicy {
 			return policy.canRetry(this.context);
 		}
 
-		public void close(RetryContext context, boolean succeeded) {
+		public void close(RetryContext context) {
 			// Only close those policies that have been used (opened):
 			for (RetryPolicy policy : contexts.keySet()) {
-				policy.close(getContext(policy), succeeded);
+				policy.close(getContext(policy));
 			}
 		}
 

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

@@ -43,9 +43,9 @@ public class NeverRetryPolicy implements RetryPolicy {
 	/**
 	 * Do nothing.
 	 * 
-	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean)
+	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext)
 	 */
-	public void close(RetryContext context, boolean succeeded) {
+	public void close(RetryContext context) {
 		// no-op
 	}
 

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

@@ -123,9 +123,9 @@ public class SimpleRetryPolicy implements RetryPolicy {
 	}
 
 	/**
-	 * @see org.springframework.batch.retry.RetryPolicy#close(RetryContext, boolean)
+	 * @see org.springframework.batch.retry.RetryPolicy#close(RetryContext)
 	 */
-	public void close(RetryContext status, boolean succeeded) {
+	public void close(RetryContext status) {
 	}
 
 	/**

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

@@ -54,7 +54,7 @@ public class TimeoutRetryPolicy implements RetryPolicy {
 		return ((TimeoutRetryContext) context).isAlive();
 	}
 
-	public void close(RetryContext context, boolean succeeded) {
+	public void close(RetryContext context) {
 	}
 
 	public RetryContext open(RetryContext parent) {

spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java

@@ -175,8 +175,8 @@ public class RetryTemplate implements RetryOperations {
 	 * @see org.springframework.batch.retry.RetryOperations#execute(RetryCallback,
 	 * RetryState)
 	 */
-	public final <T> T execute(RetryCallback<T> retryCallback, RecoveryCallback<T> recoveryCallback, RetryState retryState)
-			throws Exception, ExhaustedRetryException {
+	public final <T> T execute(RetryCallback<T> retryCallback, RecoveryCallback<T> recoveryCallback,
+			RetryState retryState) throws Exception, ExhaustedRetryException {
 		return doExecute(retryCallback, recoveryCallback, retryState);
 	}
 
@@ -282,6 +282,9 @@ public class RetryTemplate implements RetryOperations {
 	}
 
 	/**
+	 * Clean up the cache if necessary and close the context provided (if the
+	 * flag indicates that processing was successful).
+	 * 
 	 * @param context
 	 * @param state
 	 * @param succeeded
@@ -290,11 +293,11 @@ public class RetryTemplate implements RetryOperations {
 		if (state != null) {
 			if (succeeded) {
 				retryContextCache.remove(state.getKey());
-				retryPolicy.close(context, succeeded);
+				retryPolicy.close(context);
 			}
 		}
 		else {
-			retryPolicy.close(context, succeeded);
+			retryPolicy.close(context);
 		}
 	}
 
@@ -307,23 +310,23 @@ public class RetryTemplate implements RetryOperations {
 	protected void registerThrowable(RetryPolicy retryPolicy, RetryState state, RetryContext context, Exception e) {
 		if (state != null) {
 			Object key = state.getKey();
-			// TODO: this comparison assumes that hashCode is the limiting
-			// factor. Actually the cache should be able to decide for us.
-			// if (initialHashCode != key.hashCode()) {
-			// throw new RetryException(
-			// "Inconsistent state for failed item key: hashCode has changed. "
-			// +
-			// "Consider whether equals() or hashCode() for the item might be inconsistent, "
-			// + "or if you need to supply a better ItemKeyGenerator");
-			// }
+			if (context.getRetryCount() > 0 && !retryContextCache.containsKey(key)) {
+				throw new RetryException("Inconsistent state for failed item key: cache key has changed. "
+						+ "Consider whether equals() or hashCode() for the key might be inconsistent, "
+						+ "or if you need to supply a better key");
+			}
 			retryContextCache.put(key, context);
 		}
 		retryPolicy.registerThrowable(context, e);
 	}
 
 	/**
-	 * @param retryPolicy
-	 * @return a retry context
+	 * Delegate to the {@link RetryPolicy} having checked in the cache for an
+	 * existing value if the state is not null.
+	 * 
+	 * @param retryPolicy a {@link RetryPolicy} to delegate the context creation
+	 * @return a retry context, either a new one or the one used last time the
+	 * same state was encountered
 	 */
 	protected RetryContext open(RetryPolicy retryPolicy, RetryState state) {
 
@@ -364,17 +367,22 @@ public class RetryTemplate implements RetryOperations {
 	}
 
 	/**
+	 * Actions to take after final attempt has failed. If there is state clean
+	 * up the cache. If there is a recovery callback, execute that and return
+	 * its result. Otherwise throw an exception.
+	 * 
 	 * @param recoveryCallback the callback for recovery (might be null)
 	 * @param context the current retry context
-	 * @throws Exception if the callback does, and if there is no callback then
-	 * definitely the last exception from the context
+	 * @throws Exception if the callback does, and if there is no callback and
+	 * the state is null then the last exception from the context
+	 * @throws ExhaustedRetryException if the state is not null and there is no
+	 * recovery callback
 	 */
 	protected <T> T handleRetryExhausted(RecoveryCallback<T> recoveryCallback, RetryContext context, RetryState state)
 			throws Exception {
 		if (state != null) {
 			retryContextCache.remove(state.getKey());
 		}
-		// TODO: test this when state==null
 		if (recoveryCallback != null) {
 			return recoveryCallback.recover(context);
 		}
@@ -389,13 +397,13 @@ public class RetryTemplate implements RetryOperations {
 	/**
 	 * Extension point for subclasses to decide on behaviour after catching an
 	 * exception in a {@link RetryCallback}. Normal stateless behaviour is not
-	 * to rethrow, and if there is state we rethrow if the policy can still
-	 * retry.
+	 * to rethrow, and if there is state we rethrow.
 	 * 
 	 * @param retryPolicy
 	 * @param context the current context
 	 * 
-	 * @return false but subclasses might choose otherwise
+	 * @return true if the state is not null but subclasses might choose
+	 * otherwise
 	 */
 	protected boolean shouldRethrow(RetryPolicy retryPolicy, RetryContext context, RetryState state) {
 		return state != null;

spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/AlwaysRetryPolicyTests.java

@@ -29,7 +29,7 @@ public class AlwaysRetryPolicyTests extends TestCase {
 		assertTrue(policy.canRetry(context));
 		policy.registerThrowable(context, null);
 		assertTrue(policy.canRetry(context));
-		policy.close(context, true);
+		policy.close(context);
 		assertTrue(policy.canRetry(context));
 	}
 

spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java

@@ -77,17 +77,17 @@ public class CompositeRetryPolicyTests extends TestCase {
 		final List<String> list = new ArrayList<String>();
 		CompositeRetryPolicy policy = new CompositeRetryPolicy();
 		policy.setPolicies(new RetryPolicy[] { new MockRetryPolicySupport() {
-			public void close(RetryContext context, boolean succeeded) {
+			public void close(RetryContext context) {
 				list.add("1");
 			}
 		}, new MockRetryPolicySupport() {
-			public void close(RetryContext context, boolean succeeded) {
+			public void close(RetryContext context) {
 				list.add("2");
 			}
 		} });
 		RetryContext context = policy.open(null);
 		assertNotNull(context);
-		policy.close(context, true);
+		policy.close(context);
 		assertEquals(2, list.size());
 	}
 
@@ -95,19 +95,19 @@ public class CompositeRetryPolicyTests extends TestCase {
 		final List<String> list = new ArrayList<String>();
 		CompositeRetryPolicy policy = new CompositeRetryPolicy();
 		policy.setPolicies(new RetryPolicy[] { new MockRetryPolicySupport() {
-			public void close(RetryContext context, boolean succeeded) {
+			public void close(RetryContext context) {
 				list.add("1");
 				throw new RuntimeException("Pah!");
 			}
 		}, new MockRetryPolicySupport() {
-			public void close(RetryContext context, boolean succeeded) {
+			public void close(RetryContext context) {
 				list.add("2");
 			}
 		} });
 		RetryContext context = policy.open(null);
 		assertNotNull(context);
 		try {
-			policy.close(context, true);
+			policy.close(context);
 			fail("Expected RuntimeException");
 		} catch (RuntimeException e) {
 			assertEquals("Pah!", e.getMessage());

spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicyTests.java

@@ -106,7 +106,7 @@ public class ExceptionClassifierRetryPolicyTests extends TestCase {
 	public void testClose() throws Exception {
 		policy.setPolicyMap(Collections.singletonMap(ExceptionClassifierSupport.DEFAULT,
 				(RetryPolicy) new MockRetryPolicySupport() {
-					public void close(RetryContext context, boolean succeeded) {
+					public void close(RetryContext context) {
 						count++;
 					}
 				}));
@@ -114,12 +114,12 @@ public class ExceptionClassifierRetryPolicyTests extends TestCase {
 
 		// The mapped (child) policy hasn't been used yet, so if we close now
 		// we don't incur the possible expense of ceating the child context.
-		policy.close(context, true);
+		policy.close(context);
 		assertEquals(0, count); // not classified yet
 		// This forces a child context to be created and the child policy is
 		// then closed
 		policy.registerThrowable(context, new IllegalStateException());
-		policy.close(context, true);
+		policy.close(context);
 		assertEquals(1, count); // now classified
 	}
 

spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/NeverRetryPolicyTests.java

@@ -31,7 +31,7 @@ public class NeverRetryPolicyTests extends TestCase {
 		assertTrue(policy.canRetry(context));
 		policy.registerThrowable(context, null);
 		assertFalse(policy.canRetry(context));
-		policy.close(context, true);
+		policy.close(context);
 		assertFalse(policy.canRetry(context));
 	}
 

spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/TimeoutRetryPolicyTests.java

@@ -30,7 +30,7 @@ public class TimeoutRetryPolicyTests extends TestCase {
 		assertTrue(policy.canRetry(context));
 		Thread.sleep(200);
 		assertFalse(policy.canRetry(context));
-		policy.close(context, true);
+		policy.close(context);
 	}
 
 	public void testRetryCount() throws Exception {

spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/support/RetryTemplateTests.java

@@ -16,11 +16,17 @@
 
 package org.springframework.batch.retry.support;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
+
 import java.util.HashSet;
 
-import junit.framework.TestCase;
-
+import org.junit.Test;
 import org.springframework.batch.retry.ExhaustedRetryException;
+import org.springframework.batch.retry.RecoveryCallback;
 import org.springframework.batch.retry.RetryCallback;
 import org.springframework.batch.retry.RetryContext;
 import org.springframework.batch.retry.backoff.BackOffContext;
@@ -32,14 +38,15 @@ import org.springframework.batch.retry.policy.SimpleRetryPolicy;
 
 /**
  * @author Rob Harrop
- * @since 2.1
+ * @author Dave Syer
  */
-public class RetryTemplateTests extends TestCase {
+public class RetryTemplateTests {
 
 	RetryContext context;
 
 	int count = 0;
 
+	@Test
 	public void testSuccessfulRetry() throws Exception {
 		for (int x = 1; x <= 10; x++) {
 			MockRetryCallback callback = new MockRetryCallback();
@@ -51,6 +58,23 @@ public class RetryTemplateTests extends TestCase {
 		}
 	}
 
+	@Test
+	public void testSuccessfulRecovery() throws Exception {
+		MockRetryCallback callback = new MockRetryCallback();
+		callback.setAttemptsBeforeSuccess(3);
+		RetryTemplate retryTemplate = new RetryTemplate();
+		retryTemplate.setRetryPolicy(new SimpleRetryPolicy(2));
+		final Object value = new Object();
+		Object result = retryTemplate.execute(callback, new RecoveryCallback<Object>() {
+			public Object recover(RetryContext context) throws Exception {
+				return value;
+			}
+		});
+		assertEquals(2, callback.attempts);
+		assertEquals(value, result);		
+	}
+
+	@Test
 	public void testAlwaysTryAtLeastOnce() throws Exception {
 		MockRetryCallback callback = new MockRetryCallback();
 		RetryTemplate retryTemplate = new RetryTemplate();
@@ -59,6 +83,7 @@ public class RetryTemplateTests extends TestCase {
 		assertEquals(1, callback.attempts);
 	}
 
+	@Test
 	public void testNoSuccessRetry() throws Exception {
 		MockRetryCallback callback = new MockRetryCallback();
 		// Something that won't be thrown by JUnit...
@@ -79,6 +104,7 @@ public class RetryTemplateTests extends TestCase {
 		fail("Expected IllegalArgumentException");
 	}
 
+	@Test
 	public void testDefaultConfigWithExceptionSubclass() throws Exception {
 		MockRetryCallback callback = new MockRetryCallback();
 		int attempts = 3;
@@ -91,6 +117,7 @@ public class RetryTemplateTests extends TestCase {
 		assertEquals(attempts, callback.attempts);
 	}
 
+	@Test
 	public void testSetExceptions() throws Exception {
 		RetryTemplate template = new RetryTemplate();
 		SimpleRetryPolicy policy = new SimpleRetryPolicy();
@@ -119,6 +146,7 @@ public class RetryTemplateTests extends TestCase {
 		assertEquals(attempts, callback.attempts);
 	}
 
+	@Test
 	public void testBackOffInvoked() throws Exception {
 		for (int x = 1; x <= 10; x++) {
 			MockRetryCallback callback = new MockRetryCallback();
@@ -134,6 +162,7 @@ public class RetryTemplateTests extends TestCase {
 		}
 	}
 
+	@Test
 	public void testEarlyTermination() throws Exception {
 		try {
 			RetryTemplate retryTemplate = new RetryTemplate();
@@ -152,6 +181,7 @@ public class RetryTemplateTests extends TestCase {
 		}
 	}
 
+	@Test
 	public void testNestedContexts() throws Exception {
 		RetryTemplate outer = new RetryTemplate();
 		final RetryTemplate inner = new RetryTemplate();
@@ -176,6 +206,7 @@ public class RetryTemplateTests extends TestCase {
 		assertEquals(2, count);
 	}
 
+	@Test
 	public void testRethrowError() throws Exception {
 		RetryTemplate retryTemplate = new RetryTemplate();
 		retryTemplate.setRetryPolicy(new NeverRetryPolicy());
@@ -192,6 +223,7 @@ public class RetryTemplateTests extends TestCase {
 		}
 	}
 
+	@Test
 	public void testBackOffInterrupted() throws Exception {
 		RetryTemplate retryTemplate = new RetryTemplate();
 		retryTemplate.setBackOffPolicy(new StatelessBackOffPolicy() {
@@ -211,7 +243,7 @@ public class RetryTemplateTests extends TestCase {
 			assertEquals("foo", e.getMessage());
 		}
 	}
-
+	
 	private static class MockRetryCallback implements RetryCallback<Object> {
 
 		private int attempts;

spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/support/StatefulRecoveryRetryTests.java

@@ -174,13 +174,23 @@ public class StatefulRecoveryRetryTests {
 		};
 
 		try {
-			// TODO: test this
-			// retryTemplate.execute(callback, state);
-			// fail("Expected RetryException");
+			retryTemplate.execute(callback, state);
+			fail("Expected RuntimeException");
+		}
+		catch (RuntimeException ex) {
+			String message = ex.getMessage();
+			assertEquals("Barf!", message);
+		}
+		// Only fails second attempt because the algorithm to detect
+		// inconsistent has codes relies on the cache having been used for this
+		// item already...
+		try {
+			retryTemplate.execute(callback, state);
+			fail("Expected RetryException");
 		}
 		catch (RetryException ex) {
 			String message = ex.getMessage();
-			assertTrue("Message doesn't contain 'inconsistent': " + message, message.indexOf("inconsistent") >= 0);
+			assertTrue("Message doesn't contain 'inconsistent': " + message, message.contains("inconsistent"));
 		}
 
 		RetryContext context = retryTemplate.open(retryPolicy, state);