Skip to content

Rethrow particular not-retryable exceptions #281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ Expressions can contain property placeholders, such as `#{${max.delay}}` or
during initialization. There is no root object for the evaluation but they can reference
other beans in the context.

Version 1.3.3 added the `rethrow` attribute to `@Retryable`; when `true` any exceptions that are not retryable are thrown unchanged to the caller and the `@Recover` method is not called.
Version 1.3.3 added the `rethrow` and `rethrowExceptions` attributes to `@Retryable`; when `rethrow` is `true` any exceptions that are not retryable are thrown unchanged to the caller and the `@Recover` method is not called. The same behavior for exceptions listed in `rethrowExceptions`.

#### <a name="Additional_Dependencies"></a> Additional Dependencies

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ private MethodInterceptor getStatelessInterceptor(Object target, Method method,
RetryTemplate template = createTemplate(retryable.listeners());
template.setRetryPolicy(getRetryPolicy(retryable));
template.setBackOffPolicy(getBackoffPolicy(retryable.backoff()));
template.setNoRecoveryForNotRetryable(retryable.rethrow());
template.setNoRecoveryForNotRetryable(retryable.rethrow() || retryable.rethrowExceptions().length > 0);
return RetryInterceptorBuilder.stateless().retryOperations(template).label(retryable.label())
.recoverer(getRecoverer(target, method, retryable.rethrow())).build();
}
Expand All @@ -226,7 +226,7 @@ private MethodInterceptor getStatefulInterceptor(Object target, Method method, R
boolean rethrow = retryable.rethrow();
RetryTemplate template = createTemplate(retryable.listeners());
template.setRetryContextCache(this.retryContextCache);
template.setNoRecoveryForNotRetryable(rethrow);
template.setNoRecoveryForNotRetryable(rethrow || retryable.rethrowExceptions().length > 0);

CircuitBreaker circuit = AnnotatedElementUtils.findMergedAnnotation(method, CircuitBreaker.class);
if (circuit == null) {
Expand Down Expand Up @@ -343,7 +343,11 @@ private RetryPolicy getRetryPolicy(Annotation retryable) {
Integer.class);
}
}
if (includes.length == 0 && excludes.length == 0) {
Class<? extends Throwable>[] rethrowExceptions = (Class<? extends Throwable>[]) attrs.get("rethrowExceptions");
if (rethrowExceptions == null) {
rethrowExceptions = new Class[0];
}
if (includes.length == 0 && excludes.length == 0 && rethrowExceptions.length == 0) {
SimpleRetryPolicy simple = hasExpression
? new ExpressionRetryPolicy(resolve(exceptionExpression)).withBeanFactory(this.beanFactory)
: new SimpleRetryPolicy();
Expand All @@ -357,6 +361,9 @@ private RetryPolicy getRetryPolicy(Annotation retryable) {
for (Class<? extends Throwable> type : excludes) {
policyMap.put(type, false);
}
for (Class<? extends Throwable> type : rethrowExceptions) {
policyMap.put(type, false);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is identical to the previous 3 lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is using rethrowExceptions, the previous for is using excludes to iterate over.

Copy link
Contributor

@garyrussell garyrussell Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but why do we need rethrowExceptions when we already have excludes + rethrow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using rethrowExceptions on Retryable sets the noRecoveryForNotRetryable flag of RetryTemplate, so the listed exceptions are not only not-retryable, but also directly rethrown (without recovery and wrapping).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, just set rethrow alongside excludes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sine 1.3.3 hasn't been built yet, maybe we should revert that commit until this is sorted out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's sounds reasonable.
Looks like that one was requested by @ttulka as well: ec245fd.

So, we are safe so to not release anything on the matter and continue discussion 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - maybe we should move this whole thing to the new 2.0 branch where we can overhaul the entire attribute list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly happy about not getting anything in the next release, but as far as you guys decide on the API attributes, I would love to implement it if you want me to.

All in all, this feature is going to save me a lot of work!

Many thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you are "not happy", but you realized that the previous commit did not satisfy all your needs and submitted this new PR, that does not use the new attribute from the previous commit.

It made no sense to us to release a version with a new attribute that would immediately become obsolete in the "next" release.

We are happy that you are willing to contribute, but we need to finalize a design that makes sense for the entire community, and provides minimal confusion for users with simpler use cases.

Thanks, again, for your contribution; we will continue to work this through.

boolean retryNotExcluded = includes.length == 0;
if (hasExpression) {
return new ExpressionRetryPolicy(maxAttempts, policyMap, true, exceptionExpression, retryNotExcluded)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,4 +138,12 @@
*/
boolean rethrow() default false;

/**
* For these exception types raw exceptions are thrown without being wrapped and no
* recovery is performed not-retryable exceptions.
* @return exception types not to recover
* @since 1.3.3
*/
Class<? extends Throwable>[] rethrowExceptions() default {};

}
Original file line number Diff line number Diff line change
Expand Up @@ -547,8 +547,7 @@ protected <T> T handleRetryExhausted(RecoveryCallback<T> recoveryCallback, Retry
this.retryContextCache.remove(state.getKey());
}
// TODO: In 2.0 add retryForException() to the interface.
if (this.noRecoveryForNotRetryable && retryPolicy instanceof SimpleRetryPolicy
&& !((SimpleRetryPolicy) retryPolicy).retryForException(context.getLastThrowable())) {
if (shouldRethrowWithoutRecovery(context.getLastThrowable())) {
throw context.getLastThrowable();
}
if (recoveryCallback != null) {
Expand Down Expand Up @@ -587,6 +586,11 @@ protected boolean shouldRethrow(RetryPolicy retryPolicy, RetryContext context, R
return state != null && state.rollbackFor(context.getLastThrowable());
}

private boolean shouldRethrowWithoutRecovery(Throwable throwable) {
return this.noRecoveryForNotRetryable && retryPolicy instanceof SimpleRetryPolicy
&& !((SimpleRetryPolicy) retryPolicy).retryForException(throwable);
}

private <T, E extends Throwable> boolean doOpenInterceptors(RetryCallback<T, E> callback, RetryContext context) {
boolean result = true;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,22 @@ public void rethrow() {
context.close();
}

@Test
public void rethrowExceptions() {
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(TestConfiguration.class);
RethrowExceptionsService service = context.getBean(RethrowExceptionsService.class);
for (int i = 0; i < 3; i++) {
try {
service.service();
}
catch (RuntimeException e) {
assertEquals("Planned", e.getMessage());
}
}
assertEquals(3, service.getCount());
context.close();
}

private Object target(Object target) {
if (!AopUtils.isAopProxy(target)) {
return target;
Expand Down Expand Up @@ -455,6 +471,11 @@ public RethrowService rethrowService() {
return new RethrowService();
}

@Bean
public RethrowExceptionsService rethrowExceptionsService() {
return new RethrowExceptionsService();
}

@Bean
public MethodInterceptor retryInterceptor() {
return RetryInterceptorBuilder.stateless().maxAttempts(5).build();
Expand Down Expand Up @@ -734,6 +755,23 @@ public int getCount() {

}

private static class RethrowExceptionsService {

private int count = 0;

@Retryable(include = IllegalArgumentException.class, rethrowExceptions = RuntimeException.class)
public void service() {
if (this.count++ < 2) {
throw new RuntimeException("Planned");
}
}

public int getCount() {
return this.count;
}

}

public static class ExceptionChecker {

public boolean shouldRetry(Throwable t) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package org.springframework.retry.support;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;

import org.junit.Test;
Expand Down Expand Up @@ -431,6 +433,57 @@ public Object recover(RetryContext context) throws Exception {
assertEquals(value, result);
}

@Test
public void testRethrowExceptionsForNotRetryable() throws Throwable {
Map<Class<? extends Throwable>, Boolean> retryableExceptions = new HashMap<Class<? extends Throwable>, Boolean>();
retryableExceptions.put(IllegalArgumentException.class, true);
retryableExceptions.put(IllegalStateException.class, false);
SimpleRetryPolicy policy = new SimpleRetryPolicy(1, retryableExceptions);
RetryTemplate retryTemplate = new RetryTemplate();
retryTemplate.setRetryPolicy(policy);
retryTemplate.setNoRecoveryForNotRetryable(true);
try {
retryTemplate.execute(new RetryCallback<Object, Exception>() {
@Override
public Object doWithRetry(RetryContext context) throws Exception {
throw new IllegalStateException("Realllly bad!");
}
}, new RecoveryCallback<Object>() {
@Override
public Object recover(RetryContext context) throws Exception {
return new Object();
}
});
fail("Expected RuntimeException");
}
catch (IllegalStateException e) {
assertEquals("Realllly bad!", e.getMessage());
}
}

@Test
public void testRethrowExceptionsForRetryable() throws Throwable {
Map<Class<? extends Throwable>, Boolean> retryableExceptions = new HashMap<Class<? extends Throwable>, Boolean>();
retryableExceptions.put(IllegalArgumentException.class, true);
retryableExceptions.put(IllegalStateException.class, false);
SimpleRetryPolicy policy = new SimpleRetryPolicy(1, retryableExceptions);
RetryTemplate retryTemplate = new RetryTemplate();
retryTemplate.setRetryPolicy(policy);
final Object value = new Object();
Object result = retryTemplate.execute(new RetryCallback<Object, Exception>() {
@Override
public Object doWithRetry(RetryContext context) throws Exception {
throw new IllegalStateException("Will be recovered");
}
}, new RecoveryCallback<Object>() {
@Override
public Object recover(RetryContext context) throws Exception {
return value;
}
});
assertEquals(value, result);
}

private static class MockRetryCallback implements RetryCallback<Object, Exception> {

private int attempts;
Expand Down