Spring/spring-retry
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Rethrow original exception even if exhausted only

Browse Source 
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.
This commit is contained in:
Dave Syer
2016-04-12 07:24:22 -04:00
parent 8c247151c9
commit 419f16274a
2 changed files with 57 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
src/main/java/org/springframework/retry/support/RetryTemplate.java
View File

@@ -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<RetryListener> list = new ArrayList<RetryListener>(Arrays.asList(listeners));
List<RetryListener> list = new ArrayList<RetryListener>(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, E extends Throwable> T execute(RetryCallback<T, E> 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, E extends Throwable> T execute(RetryCallback<T, E> retryCallback,
RecoveryCallback<T> 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, E extends Throwable> T execute(RetryCallback<T, E> 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, E extends Throwable> T execute(RetryCallback<T, E> retryCallback,
RecoveryCallback<T> 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.<E>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> T handleRetryExhausted(RecoveryCallback<T> 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 <E extends Throwable> 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 <T, E extends Throwable> void doCloseInterceptors(RetryCallback<T, E> 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 <T, E extends Throwable> void doOnErrorInterceptors(RetryCallback<T, E> 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);
}
}

38
src/test/java/org/springframework/retry/support/RetryTemplateTests.java
View File

@@ -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<Object>() {
@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<Object, Exception>() {
@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<Object, Throwable>() {
@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<Object, Throwable>() {
@Override
public Object doWithRetry(RetryContext status) throws Throwable {
context = status;
count++;
RetryTemplateTests.this.context = status;
RetryTemplateTests.this.count++;
Object result = inner.execute(new RetryCallback<Object, Throwable>() {
@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<Object, Exception>() {
@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<Object, Exception>() {
@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<Object, Exception>() {
@Override
public Object doWithRetry(RetryContext context) throws Exception {
throw new RuntimeException("Bad!");
}
@@ -364,6 +372,7 @@ public class RetryTemplateTests {
try {
tested.execute(new RetryCallback<Object, Exception>() {
@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++;
}
}
}