Retry policy needed to know if the callback was successful.

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

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

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

@@ -68,15 +68,15 @@ public class CompositeRetryPolicy extends AbstractStatelessRetryPolicy {
 	 * 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)
+	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean)
 	 */
-	public void close(RetryContext context) {
+	public void close(RetryContext context, boolean succeeded) {
 		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]);
+				policies[i].close(contexts[i], succeeded);
 			}
 			catch (RuntimeException e) {
 				if (exception==null) {
@@ -108,7 +108,7 @@ public class CompositeRetryPolicy extends AbstractStatelessRetryPolicy {
 	 * 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)
+	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean)
 	 */
 	public void registerThrowable(RetryContext context, Throwable throwable) throws TerminatedRetryException {
 		RetryContext[] contexts = ((CompositeRetryContext) context).contexts;

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

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

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

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

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

@@ -87,10 +87,10 @@ public class RecoveryCallbackRetryPolicy extends AbstractStatefulRetryPolicy {
 	/**
 	 * Delegates to the delegate context.
 	 * 
-	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext)
+	 * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext, boolean)
 	 */
-	public void close(RetryContext context) {
+	public void close(RetryContext context, boolean succeeded) {
-		((RetryPolicy) context).close(context);
+		((RetryPolicy) context).close(context, succeeded);
 	}
 
 	/**
@@ -156,8 +156,11 @@ public class RecoveryCallbackRetryPolicy extends AbstractStatefulRetryPolicy {
 			return delegate.canRetry(this.delegateContext);
 		}
 
-		public void close(RetryContext context) {
+		public void close(RetryContext context, boolean succeeded) {
-			delegate.close(this.delegateContext);
+			if (succeeded) {
+				retryContextCache.remove(key);
+				delegate.close(this.delegateContext, succeeded);
+			}
 		}
 
 		public RetryContext open(RetryCallback callback, RetryContext parent) {

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

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

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

@@ -56,7 +56,7 @@ public class TimeoutRetryPolicy extends AbstractStatelessRetryPolicy {
 		return ((TimeoutRetryContext) context).isAlive();
 	}
 
-	public void close(RetryContext context) {
+	public void close(RetryContext context, boolean succeeded) {
 	}
 
 	public RetryContext open(RetryCallback callback, RetryContext parent) {

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

@@ -205,7 +205,7 @@ public class RetryTemplate implements RetryOperations {
 
 		}
 		finally {
-			retryPolicy.close(context);
+			retryPolicy.close(context, lastException==null);
 			doCloseInterceptors(callback, context, lastException);
 			RetrySynchronizationManager.clear();
 		}

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);
+		policy.close(context, true);
 		assertTrue(policy.canRetry(context));
 	}
 

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

@@ -78,17 +78,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) {
+			public void close(RetryContext context, boolean succeeded) {
 				list.add("1");
 			}
 		}, new MockRetryPolicySupport() {
-			public void close(RetryContext context) {
+			public void close(RetryContext context, boolean succeeded) {
 				list.add("2");
 			}
 		} });
 		RetryContext context = policy.open(null, null);
 		assertNotNull(context);
-		policy.close(context);
+		policy.close(context, true);
 		assertEquals(2, list.size());
 	}
 
@@ -96,19 +96,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) {
+			public void close(RetryContext context, boolean succeeded) {
 				list.add("1");
 				throw new RuntimeException("Pah!");
 			}
 		}, new MockRetryPolicySupport() {
-			public void close(RetryContext context) {
+			public void close(RetryContext context, boolean succeeded) {
 				list.add("2");
 			}
 		} });
 		RetryContext context = policy.open(null, null);
 		assertNotNull(context);
 		try {
-			policy.close(context);
+			policy.close(context, true);
 			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) {
+					public void close(RetryContext context, boolean succeeded) {
 						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);
+		policy.close(context, true);
 		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);
+		policy.close(context, true);
 		assertEquals(1, count); // now classified
 	}
 

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

@@ -0,0 +1,133 @@
+/*
+ * Copyright 2006-2007 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.batch.retry.policy;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import org.junit.Test;
+import org.springframework.batch.retry.RetryCallback;
+import org.springframework.batch.retry.RetryContext;
+import org.springframework.batch.retry.callback.RecoveryRetryCallback;
+import org.springframework.batch.retry.support.RetryTemplate;
+
+/**
+ * @author Dave Syer
+ * 
+ */
+public class ExternalRetryIntergrationTests {
+
+	@Test
+	public void testExternalRetryWithFailAndNoRetry() throws Exception {
+		MockRetryCallback callback = new MockRetryCallback();
+
+		RecoveryRetryCallback recoveryCallback = new RecoveryRetryCallback("foo", callback);
+
+		RetryTemplate retryTemplate = new RetryTemplate();
+		RecoveryCallbackRetryPolicy retryPolicy = new RecoveryCallbackRetryPolicy(new SimpleRetryPolicy(1));
+		MapRetryContextCache cache = new MapRetryContextCache();
+		retryPolicy.setRetryContextCache(cache);
+		retryTemplate.setRetryPolicy(retryPolicy);
+
+		assertFalse(cache.containsKey("foo"));
+
+		Object result = "start_foo";
+		try {
+			result = retryTemplate.execute(recoveryCallback);
+			// The first failed attempt we expect to retry...
+			fail("Expected RuntimeException");
+		}
+		catch (RuntimeException e) {
+			assertNull(e.getMessage());
+		}
+
+		assertTrue(cache.containsKey("foo"));
+
+		try {
+			result = retryTemplate.execute(recoveryCallback);
+			// We always get a second attempt...
+		}
+		catch (IllegalArgumentException e) {
+			// This is now the "exhausted" message:
+			assertNotNull(e.getMessage());
+			// But if template is external we should
+			// swallow the exception when retry is impossible.
+			fail("Did not expect IllegalArgumentException");
+		}
+
+		assertFalse(cache.containsKey("foo"));
+
+		// Callback is called once: the recovery path should be called in
+		// handleRetryExhausted (so not in this test)...
+		assertEquals(1, callback.attempts);
+		assertEquals(null, result);
+	}
+
+	@Test
+	public void testExternalRetryWithSuccessOnRetry() throws Exception {
+		MockRetryCallback callback = new MockRetryCallback();
+
+		RecoveryRetryCallback recoveryCallback = new RecoveryRetryCallback("foo", callback);
+
+		RetryTemplate retryTemplate = new RetryTemplate();
+		RecoveryCallbackRetryPolicy retryPolicy = new RecoveryCallbackRetryPolicy(new SimpleRetryPolicy(2));
+		MapRetryContextCache cache = new MapRetryContextCache();
+		retryPolicy.setRetryContextCache(cache);
+		retryTemplate.setRetryPolicy(retryPolicy);
+
+		assertFalse(cache.containsKey("foo"));
+
+		Object result = "start_foo";
+		try {
+			result = retryTemplate.execute(recoveryCallback);
+			// The first failed attempt we expect to retry...
+			fail("Expected RuntimeException");
+		}
+		catch (RuntimeException e) {
+			assertNull(e.getMessage());
+		}
+
+		assertTrue(cache.containsKey("foo"));
+
+		result = retryTemplate.execute(recoveryCallback);
+
+		assertFalse(cache.containsKey("foo"));
+
+		assertEquals(2, callback.attempts);
+		assertEquals("bar", result);
+	}
+
+	/**
+	 * @author Dave Syer
+	 * 
+	 */
+	private final class MockRetryCallback implements RetryCallback {
+		int attempts = 0;
+
+		public Object doWithRetry(RetryContext context) throws Throwable {
+			attempts++;
+			if (attempts < 2) {
+				throw new RuntimeException();
+			}
+			return "bar";
+		}
+	}
+
+}

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

@@ -94,7 +94,6 @@ public class ExternalRetryPolicyTests extends TestCase {
 		}
 		catch (IllegalArgumentException e) {
 			// Expected
-			System.err.println(e.getMessage());
 			assertTrue(Pattern.matches(".*not.*Throwable.*", e.getMessage()));
 		}
 
@@ -192,7 +191,7 @@ public class ExternalRetryPolicyTests extends TestCase {
 			return attempts < retryLimit;
 		}
 
-		public void close(RetryContext context) {
+		public void close(RetryContext context, boolean succeeded) {
 			// do nothing
 		}
 

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);
+		policy.close(context, true);
 		assertFalse(policy.canRetry(context));
 	}
 

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

@@ -109,7 +109,7 @@ public class RecoveryRetryPolicyTests extends TestCase {
 		assertNotNull(context);
 		policy.registerThrowable(context, new Exception());
 		assertFalse(policy.canRetry(context));
-		policy.close(context);
+		policy.close(context, true);
 		// still can't retry, even if policy is closed
 		// (not that this would happen in practice)...
 		assertFalse(policy.canRetry(context));
@@ -129,14 +129,14 @@ public class RecoveryRetryPolicyTests extends TestCase {
 		assertNotNull(context);
 		policy.registerThrowable(context, new Exception());
 		assertTrue(policy.canRetry(context));
-		policy.close(context);
+		policy.close(context, false);
 
 		// Second call...
 		context = policy.open(callback, null);
 		assertNotNull(context);
 		policy.registerThrowable(context, new Exception());
 		assertFalse(policy.canRetry(context));
-		policy.close(context);
+		policy.close(context, true);
 
 	}
 
@@ -229,7 +229,7 @@ public class RecoveryRetryPolicyTests extends TestCase {
 
 		// False before close...
 		assertFalse(policy.canRetry(context));
-		policy.close(context);
+		policy.close(context, true);
 		Object result = policy.handleRetryExhausted(context);
 		assertNull(result); // default result is null
 

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);
+		policy.close(context, true);
 	}
 
 	public void testRetryCount() throws Exception {