From 419f16274ae8b3f3e7e7406e2db97443509636de Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Tue, 12 Apr 2016 07:24:22 -0400 Subject: [PATCH] Rethrow original exception even if exhausted only There doesn't seem to be much justification for throwing RetryExhaustedException unconditionally when the context is marked as exhausted. It would be better to behave, as far as the caller is concerned, as if the cause had been the original exception. --- .../retry/support/RetryTemplate.java | 65 +++++++++---------- .../retry/support/RetryTemplateTests.java | 38 +++++++---- 2 files changed, 57 insertions(+), 46 deletions(-) diff --git a/src/main/java/org/springframework/retry/support/RetryTemplate.java b/src/main/java/org/springframework/retry/support/RetryTemplate.java index 6d6adc6..a57853c 100644 --- a/src/main/java/org/springframework/retry/support/RetryTemplate.java +++ b/src/main/java/org/springframework/retry/support/RetryTemplate.java @@ -23,7 +23,6 @@ import java.util.List; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; - import org.springframework.retry.ExhaustedRetryException; import org.springframework.retry.RecoveryCallback; import org.springframework.retry.RetryCallback; @@ -119,9 +118,9 @@ public class RetryTemplate implements RetryOperations { * @see #setListeners(RetryListener[]) */ public void registerListener(RetryListener listener) { - List list = new ArrayList(Arrays.asList(listeners)); + List list = new ArrayList(Arrays.asList(this.listeners)); list.add(listener); - listeners = list.toArray(new RetryListener[list.size()]); + this.listeners = list.toArray(new RetryListener[list.size()]); } /** @@ -153,6 +152,7 @@ public class RetryTemplate implements RetryOperations { * @throws TerminatedRetryException if the retry has been manually terminated by a * listener. */ + @Override public final T execute(RetryCallback retryCallback) throws E { return doExecute(retryCallback, null, null); } @@ -167,6 +167,7 @@ public class RetryTemplate implements RetryOperations { * @throws TerminatedRetryException if the retry has been manually terminated by a * listener. */ + @Override public final T execute(RetryCallback retryCallback, RecoveryCallback recoveryCallback) throws E { return doExecute(retryCallback, recoveryCallback, null); @@ -181,6 +182,7 @@ public class RetryTemplate implements RetryOperations { * @param retryState the {@link RetryState} * @throws ExhaustedRetryException if the retry has been exhausted. */ + @Override public final T execute(RetryCallback retryCallback, RetryState retryState) throws E, ExhaustedRetryException { return doExecute(retryCallback, null, retryState); @@ -195,6 +197,7 @@ public class RetryTemplate implements RetryOperations { * @param recoveryCallback the {@link RecoveryCallback} * @param retryState the {@link RetryState} */ + @Override public final T execute(RetryCallback retryCallback, RecoveryCallback recoveryCallback, RetryState retryState) throws E, ExhaustedRetryException { @@ -223,8 +226,8 @@ public class RetryTemplate implements RetryOperations { // Allow the retry policy to initialise itself... RetryContext context = open(retryPolicy, state); - if (logger.isTraceEnabled()) { - logger.trace("RetryContext retrieved: " + context); + if (this.logger.isTraceEnabled()) { + this.logger.trace("RetryContext retrieved: " + context); } // Make sure the context is available globally for clients who need @@ -267,8 +270,8 @@ public class RetryTemplate implements RetryOperations { while (canRetry(retryPolicy, context) && !context.isExhaustedOnly()) { try { - if (logger.isDebugEnabled()) { - logger.debug("Retry: count=" + context.getRetryCount()); + if (this.logger.isDebugEnabled()) { + this.logger.debug("Retry: count=" + context.getRetryCount()); } // Reset the last exception, so if we are successful // the close interceptors will not think we failed... @@ -295,20 +298,20 @@ public class RetryTemplate implements RetryOperations { catch (BackOffInterruptedException ex) { lastException = e; // back off was prevented by another thread - fail the retry - if (logger.isDebugEnabled()) { - logger.debug("Abort retry because interrupted: count=" + context.getRetryCount()); + if (this.logger.isDebugEnabled()) { + this.logger.debug("Abort retry because interrupted: count=" + context.getRetryCount()); } throw ex; } } - if (logger.isDebugEnabled()) { - logger.debug("Checking for rethrow: count=" + context.getRetryCount()); + if (this.logger.isDebugEnabled()) { + this.logger.debug("Checking for rethrow: count=" + context.getRetryCount()); } if (shouldRethrow(retryPolicy, context, state)) { - if (logger.isDebugEnabled()) { - logger.debug("Rethrow in retry for policy: count=" + context.getRetryCount()); + if (this.logger.isDebugEnabled()) { + this.logger.debug("Rethrow in retry for policy: count=" + context.getRetryCount()); } throw RetryTemplate.wrapIfNecessary(e); } @@ -322,12 +325,8 @@ public class RetryTemplate implements RetryOperations { */ } - if (logger.isDebugEnabled()) { - logger.debug("Retry failed last attempt: count=" + context.getRetryCount()); - } - - if (context.isExhaustedOnly()) { - rethrow(context, "Retry exhausted after last attempt with no recovery path."); + if (this.logger.isDebugEnabled()) { + this.logger.debug("Retry failed last attempt: count=" + context.getRetryCount()); } return handleRetryExhausted(recoveryCallback, context, state); @@ -370,7 +369,7 @@ public class RetryTemplate implements RetryOperations { boolean succeeded) { if (state != null) { if (succeeded) { - retryContextCache.remove(state.getKey()); + this.retryContextCache.remove(state.getKey()); retryPolicy.close(context); } } @@ -389,13 +388,13 @@ public class RetryTemplate implements RetryOperations { RetryContext context, Throwable e) { if (state != null) { Object key = state.getKey(); - if (context.getRetryCount() > 0 && !retryContextCache.containsKey(key)) { + if (context.getRetryCount() > 0 && !this.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); + this.retryContextCache.put(key, context); } retryPolicy.registerThrowable(context, e); } @@ -423,14 +422,14 @@ public class RetryTemplate implements RetryOperations { // If there is no cache hit we can avoid the possible expense of the // cache re-hydration. - if (!retryContextCache.containsKey(key)) { + if (!this.retryContextCache.containsKey(key)) { // The cache is only used if there is a failure. return doOpenInternal(retryPolicy); } - RetryContext context = retryContextCache.get(key); + RetryContext context = this.retryContextCache.get(key); if (context == null) { - if (retryContextCache.containsKey(key)) { + if (this.retryContextCache.containsKey(key)) { throw new RetryException( "Inconsistent state for failed item: no history found. " + "Consider whether equals() or hashCode() for the item might be inconsistent, " @@ -467,20 +466,20 @@ public class RetryTemplate implements RetryOperations { protected T handleRetryExhausted(RecoveryCallback recoveryCallback, RetryContext context, RetryState state) throws Throwable { if (state != null) { - retryContextCache.remove(state.getKey()); + this.retryContextCache.remove(state.getKey()); } if (recoveryCallback != null) { return recoveryCallback.recover(context); } if (state != null) { - logger.debug("Retry exhausted after last attempt with no recovery path."); + this.logger.debug("Retry exhausted after last attempt with no recovery path."); rethrow(context, "Retry exhausted after last attempt with no recovery path"); } throw wrapIfNecessary(context.getLastThrowable()); } protected void rethrow(RetryContext context, String message) throws E { - if (throwLastExceptionOnExhausted) { + if (this.throwLastExceptionOnExhausted) { @SuppressWarnings("unchecked") E rethrow = (E) context.getLastThrowable(); throw rethrow; @@ -508,7 +507,7 @@ public class RetryTemplate implements RetryOperations { boolean result = true; - for (RetryListener listener : listeners) { + for (RetryListener listener : this.listeners) { result = result && listener.open(context, callback); } @@ -518,15 +517,15 @@ public class RetryTemplate implements RetryOperations { private void doCloseInterceptors(RetryCallback callback, RetryContext context, Throwable lastException) { - for (int i = listeners.length; i-- > 0;) { - listeners[i].close(context, callback, lastException); + for (int i = this.listeners.length; i-- > 0;) { + this.listeners[i].close(context, callback, lastException); } } private void doOnErrorInterceptors(RetryCallback callback, RetryContext context, Throwable throwable) { - for (int i = listeners.length; i-- > 0;) { - listeners[i].onError(context, callback, throwable); + for (int i = this.listeners.length; i-- > 0;) { + this.listeners[i].onError(context, callback, throwable); } } diff --git a/src/test/java/org/springframework/retry/support/RetryTemplateTests.java b/src/test/java/org/springframework/retry/support/RetryTemplateTests.java index 6a31af2..3faf4bf 100644 --- a/src/test/java/org/springframework/retry/support/RetryTemplateTests.java +++ b/src/test/java/org/springframework/retry/support/RetryTemplateTests.java @@ -32,7 +32,6 @@ import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; import org.springframework.classify.BinaryExceptionClassifier; -import org.springframework.retry.ExhaustedRetryException; import org.springframework.retry.RecoveryCallback; import org.springframework.retry.RetryCallback; import org.springframework.retry.RetryContext; @@ -105,6 +104,7 @@ public class RetryTemplateTests { Exception.class, true))); final Object value = new Object(); Object result = retryTemplate.execute(callback, new RecoveryCallback() { + @Override public Object recover(RetryContext context) throws Exception { return value; } @@ -226,16 +226,17 @@ public class RetryTemplateTests { try { RetryTemplate retryTemplate = new RetryTemplate(); retryTemplate.execute(new RetryCallback() { + @Override public Object doWithRetry(RetryContext status) throws Exception { status.setExhaustedOnly(); throw new IllegalStateException("Retry this operation"); } }); fail("Expected ExhaustedRetryException"); - } catch (ExhaustedRetryException ex) { + } catch (IllegalStateException ex) { // Expected for internal retry policy (external would recover // gracefully) - assertEquals("Retry this operation", ex.getCause().getMessage()); + assertEquals("Retry this operation", ex.getMessage()); } } @@ -245,6 +246,7 @@ public class RetryTemplateTests { RetryTemplate retryTemplate = new RetryTemplate(); retryTemplate.setThrowLastExceptionOnExhausted(true); retryTemplate.execute(new RetryCallback() { + @Override public Object doWithRetry(RetryContext status) throws Exception { status.setExhaustedOnly(); throw new IllegalStateException("Retry this operation"); @@ -263,15 +265,17 @@ public class RetryTemplateTests { RetryTemplate outer = new RetryTemplate(); final RetryTemplate inner = new RetryTemplate(); outer.execute(new RetryCallback() { + @Override public Object doWithRetry(RetryContext status) throws Throwable { - context = status; - count++; + RetryTemplateTests.this.context = status; + RetryTemplateTests.this.count++; Object result = inner.execute(new RetryCallback() { + @Override public Object doWithRetry(RetryContext status) throws Throwable { - count++; - assertNotNull(context); - assertNotSame(status, context); - assertSame(context, status.getParent()); + RetryTemplateTests.this.count++; + assertNotNull(RetryTemplateTests.this.context); + assertNotSame(status, RetryTemplateTests.this.context); + assertSame(RetryTemplateTests.this.context, status.getParent()); assertSame("The context should be the child", status, RetrySynchronizationManager.getContext()); return null; @@ -282,7 +286,7 @@ public class RetryTemplateTests { return result; } }); - assertEquals(2, count); + assertEquals(2, this.count); } @Test @@ -291,6 +295,7 @@ public class RetryTemplateTests { retryTemplate.setRetryPolicy(new NeverRetryPolicy()); try { retryTemplate.execute(new RetryCallback() { + @Override public Object doWithRetry(RetryContext context) throws Exception { throw new Error("Realllly bad!"); } @@ -312,6 +317,7 @@ public class RetryTemplateTests { }); try { retryTemplate.execute(new RetryCallback() { + @Override public Object doWithRetry(RetryContext context) throws Exception { throw new RuntimeException("Realllly bad!"); } @@ -326,12 +332,14 @@ public class RetryTemplateTests { public void testBackOffInterrupted() throws Throwable { RetryTemplate retryTemplate = new RetryTemplate(); retryTemplate.setBackOffPolicy(new StatelessBackOffPolicy() { + @Override protected void doBackOff() throws BackOffInterruptedException { throw new BackOffInterruptedException("foo"); } }); try { retryTemplate.execute(new RetryCallback() { + @Override public Object doWithRetry(RetryContext context) throws Exception { throw new RuntimeException("Bad!"); } @@ -364,6 +372,7 @@ public class RetryTemplateTests { try { tested.execute(new RetryCallback() { + @Override public Object doWithRetry(RetryContext context) throws Exception { throw new Exception("maybe next time!"); } @@ -392,9 +401,10 @@ public class RetryTemplateTests { private Exception exceptionToThrow = new Exception(); + @Override public Object doWithRetry(RetryContext status) throws Exception { this.attempts++; - if (attempts < attemptsBeforeSuccess) { + if (this.attempts < this.attemptsBeforeSuccess) { throw this.exceptionToThrow; } return null; @@ -415,14 +425,16 @@ public class RetryTemplateTests { public int startCalls; + @Override public BackOffContext start(RetryContext status) { - startCalls++; + this.startCalls++; return null; } + @Override public void backOff(BackOffContext backOffContext) throws BackOffInterruptedException { - backOffCalls++; + this.backOffCalls++; } } }