diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/RetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/RetryPolicy.java index f6514cc91..be38283ac 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/RetryPolicy.java +++ b/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. diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java index 0ebbd11c1..88967ddf4 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java +++ b/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; diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicy.java index 6f79d62cf..6be7ad0d2 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicy.java +++ b/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)); } } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/NeverRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/NeverRetryPolicy.java index af8cc565b..b69c3cc67 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/NeverRetryPolicy.java +++ b/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 } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/SimpleRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/SimpleRetryPolicy.java index 0e5889b87..04d9e4c7e 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/SimpleRetryPolicy.java +++ b/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) { } /** diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/TimeoutRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/TimeoutRetryPolicy.java index 389667804..887f7e3c5 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/TimeoutRetryPolicy.java +++ b/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) { diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java index a6f422578..172088dc3 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/support/RetryTemplate.java +++ b/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 execute(RetryCallback retryCallback, RecoveryCallback recoveryCallback, RetryState retryState) - throws Exception, ExhaustedRetryException { + public final T execute(RetryCallback retryCallback, RecoveryCallback 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 handleRetryExhausted(RecoveryCallback 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; diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/AlwaysRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/AlwaysRetryPolicyTests.java index 020fe46b1..68735c783 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/AlwaysRetryPolicyTests.java +++ b/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)); } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java index 579700a8a..a7335dc20 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java @@ -77,17 +77,17 @@ public class CompositeRetryPolicyTests extends TestCase { final List list = new ArrayList(); 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 list = new ArrayList(); 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()); diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicyTests.java index 9684146bc..deaf608fb 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/ExceptionClassifierRetryPolicyTests.java +++ b/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 } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/NeverRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/NeverRetryPolicyTests.java index aed27dc9a..e2ea04ebd 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/NeverRetryPolicyTests.java +++ b/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)); } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/TimeoutRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/TimeoutRetryPolicyTests.java index 0d380c2ed..cce4cb0d4 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/TimeoutRetryPolicyTests.java +++ b/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 { diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/support/RetryTemplateTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/support/RetryTemplateTests.java index 7a8929aa4..d36d0bf9a 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/support/RetryTemplateTests.java +++ b/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() { + 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 { private int attempts; diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/support/StatefulRecoveryRetryTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/support/StatefulRecoveryRetryTests.java index a7ed38d7f..e80c43138 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/support/StatefulRecoveryRetryTests.java +++ b/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);