spring-projectsGH-264: Enable Skipping Recovery

Resolves spring-projects#264

README.md

@@ -597,6 +597,24 @@ When evaluating at runtime, a root object containing the method arguments is pas
 **Note:** The arguments are not available until the method has been called at least once; they will be null initially, which means, for example, you can't set the initial `maxAttempts` using an argument value, you can, however, change the `maxAttempts` after the first failure and before any retries are performed.
 Also, the arguments are only available when using stateless retry (which includes the `@CircuitBreaker`).
 
+Version 2.0 adds more flexibility to exception classification.
+
+```java
+@Retryable(retryFor = RuntimeException.class, noRetryFor = IllegalStateException.class, notRecoverable = {
+        IllegalArgumentException.class, IllegalStateException.class })
+public void service() {
+    ...
+}
+
+@Recover
+public void recover(Throwable cause) {
+    ...
+}
+```
+
+`retryFor` and `noRetryFor` are replacements of `include` and `exclude` properties, which are now deprecated.
+The new `notRecoverable` property allows the recovery method(s) to be skipped, even if one matches the exception type; the exception is thrown to the caller either after retries are exhausted, or immediately, if the exception is not retryable.
+
 ##### Examples
 
 ```java

src/main/java/org/springframework/retry/RetryContext.java

@@ -55,6 +55,12 @@ public interface RetryContext extends AttributeAccessor {
 	 */
 	String EXHAUSTED = "context.exhausted";
 
+	/**
+	 * Retry context attribute that is non-null (and true) if the exception is not
+	 * recoverable.
+	 */
+	String NO_RECOVERY = "context.no-recovery";
+
 	/**
 	 * Signal to the framework that no more attempts should be made to try or retry the
 	 * current {@link RetryCallback}.

src/main/java/org/springframework/retry/annotation/AnnotationAwareRetryOperationsInterceptor.java

@@ -344,11 +344,11 @@ private RetryPolicy getRetryPolicy(Annotation retryable, boolean stateless) {
 		boolean hasExpression = StringUtils.hasText(exceptionExpression);
 		if (includes.length == 0) {
 			@SuppressWarnings("unchecked")
-			Class<? extends Throwable>[] value = (Class<? extends Throwable>[]) attrs.get("include");
+			Class<? extends Throwable>[] value = (Class<? extends Throwable>[]) attrs.get("retryFor");
 			includes = value;
 		}
 		@SuppressWarnings("unchecked")
-		Class<? extends Throwable>[] excludes = (Class<? extends Throwable>[]) attrs.get("exclude");
+		Class<? extends Throwable>[] excludes = (Class<? extends Throwable>[]) attrs.get("noRetryFor");
 		Integer maxAttempts = (Integer) attrs.get("maxAttempts");
 		String maxAttemptsExpression = (String) attrs.get("maxAttemptsExpression");
 		Expression parsedExpression = null;
@@ -360,8 +360,9 @@ private RetryPolicy getRetryPolicy(Annotation retryable, boolean stateless) {
 			}
 		}
 		final Expression expression = parsedExpression;
+		SimpleRetryPolicy simple = null;
 		if (includes.length == 0 && excludes.length == 0) {
-			SimpleRetryPolicy simple = hasExpression
+			simple = hasExpression
 					? new ExpressionRetryPolicy(resolve(exceptionExpression)).withBeanFactory(this.beanFactory)
 					: new SimpleRetryPolicy();
 			if (expression != null) {
@@ -370,7 +371,6 @@ private RetryPolicy getRetryPolicy(Annotation retryable, boolean stateless) {
 			else {
 				simple.setMaxAttempts(maxAttempts);
 			}
-			return simple;
 		}
 		Map<Class<? extends Throwable>, Boolean> policyMap = new HashMap<>();
 		for (Class<? extends Throwable> type : includes) {
@@ -380,17 +380,24 @@ private RetryPolicy getRetryPolicy(Annotation retryable, boolean stateless) {
 			policyMap.put(type, false);
 		}
 		boolean retryNotExcluded = includes.length == 0;
-		if (hasExpression) {
-			return new ExpressionRetryPolicy(maxAttempts, policyMap, true, exceptionExpression, retryNotExcluded)
-					.withBeanFactory(this.beanFactory);
-		}
-		else {
-			SimpleRetryPolicy policy = new SimpleRetryPolicy(maxAttempts, policyMap, true, retryNotExcluded);
-			if (expression != null) {
-				policy.setMaxAttempts(() -> evaluate(expression, Integer.class, stateless));
+		if (simple == null) {
+			if (hasExpression) {
+				simple = new ExpressionRetryPolicy(maxAttempts, policyMap, true, resolve(exceptionExpression),
+						retryNotExcluded).withBeanFactory(this.beanFactory);
+			}
+			else {
+				simple = new SimpleRetryPolicy(maxAttempts, policyMap, true, retryNotExcluded);
+				if (expression != null) {
+					simple.setMaxAttempts(() -> evaluate(expression, Integer.class, stateless));
+				}
 			}
-			return policy;
 		}
+		@SuppressWarnings("unchecked")
+		Class<? extends Throwable>[] noRecovery = (Class<? extends Throwable>[]) attrs.get("notRecoverable");
+		if (noRecovery != null && noRecovery.length > 0) {
+			simple.setNotRecoverable(noRecovery);
+		}
+		return simple;
 	}
 
 	private BackOffPolicy getBackoffPolicy(Backoff backoff, boolean stateless) {

src/main/java/org/springframework/retry/annotation/CircuitBreaker.java

@@ -22,6 +22,8 @@
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
+import org.springframework.core.annotation.AliasFor;
+
 /**
  * Annotation for a method invocation that is retryable.
  *
@@ -48,17 +50,51 @@
 	 * Exception types that are retryable. Defaults to empty (and if excludes is also
 	 * empty all exceptions are retried).
 	 * @return exception types to retry
+	 * @deprecated in favor of {@link #retryFor()}.
 	 */
+	@AliasFor("retryFor")
+	@Deprecated
 	Class<? extends Throwable>[] include() default {};
 
+	/**
+	 * Exception types that are retryable. Defaults to empty (and, if noRetryFor is also
+	 * empty, all exceptions are retried).
+	 * @return exception types to retry
+	 * @since 2.0
+	 */
+	@AliasFor("include")
+	Class<? extends Throwable>[] retryFor() default {};
+
 	/**
 	 * Exception types that are not retryable. Defaults to empty (and if includes is also
 	 * empty all exceptions are retried). If includes is empty but excludes is not, all
 	 * not excluded exceptions are retried
 	 * @return exception types not to retry
+	 * @deprecated in favor of {@link #noRetryFor()}.
 	 */
+	@Deprecated
+	@AliasFor("noRetryFor")
 	Class<? extends Throwable>[] exclude() default {};
 
+	/**
+	 * Exception types that are not retryable. Defaults to empty (and, if retryFor is also
+	 * empty, all exceptions are retried). If retryFor is empty but excludes is not, all
+	 * other exceptions are retried
+	 * @return exception types not to retry
+	 * @since 2.0
+	 */
+	@AliasFor("exclude")
+	Class<? extends Throwable>[] noRetryFor() default {};
+
+	/**
+	 * Exception types that are not recoverable; these exceptions are thrown to the caller
+	 * without calling any recoverer (immediately if also in {@link #noRetryFor()}).
+	 * Defaults to empty.
+	 * @return exception types not to retry
+	 * @since 2.0
+	 */
+	Class<? extends Throwable>[] notRecoverable() default {};
+
 	/**
 	 * @return the maximum number of attempts (including the first failure), defaults to 3
 	 */

src/main/java/org/springframework/retry/annotation/Retryable.java

@@ -22,6 +22,8 @@
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
+import org.springframework.core.annotation.AliasFor;
+
 /**
  * Annotation for a method invocation that is retryable.
  *
@@ -52,27 +54,63 @@
 	String interceptor() default "";
 
 	/**
-	 * Exception types that are retryable. Synonym for includes(). Defaults to empty (and
+	 * Exception types that are retryable. Synonym for include(). Defaults to empty (and
 	 * if excludes is also empty all exceptions are retried).
 	 * @return exception types to retry
+	 * @deprecated in favor of {@link #retryFor()}
 	 */
+	@Deprecated
 	Class<? extends Throwable>[] value() default {};
 
 	/**
-	 * Exception types that are retryable. Defaults to empty (and if excludes is also
-	 * empty all exceptions are retried).
+	 * Exception types that are retryable. Defaults to empty (and, if exclude is also
+	 * empty, all exceptions are retried).
 	 * @return exception types to retry
+	 * @deprecated in favor of {@link #retryFor()}.
 	 */
+	@AliasFor("retryFor")
+	@Deprecated
 	Class<? extends Throwable>[] include() default {};
 
 	/**
-	 * Exception types that are not retryable. Defaults to empty (and if includes is also
+	 * Exception types that are retryable. Defaults to empty (and, if noRetryFor is also
+	 * empty, all exceptions are retried).
+	 * @return exception types to retry
+	 * @since 2.0
+	 */
+	@AliasFor("include")
+	Class<? extends Throwable>[] retryFor() default {};
+
+	/**
+	 * Exception types that are not retryable. Defaults to empty (and if include is also
 	 * empty all exceptions are retried). If includes is empty but excludes is not, all
 	 * not excluded exceptions are retried
 	 * @return exception types not to retry
+	 * @deprecated in favor of {@link #noRetryFor()}.
 	 */
+	@Deprecated
+	@AliasFor("noRetryFor")
 	Class<? extends Throwable>[] exclude() default {};
 
+	/**
+	 * Exception types that are not retryable. Defaults to empty (and, if retryFor is also
+	 * empty, all exceptions are retried). If retryFor is empty but excludes is not, all
+	 * other exceptions are retried
+	 * @return exception types not to retry
+	 * @since 2.0
+	 */
+	@AliasFor("exclude")
+	Class<? extends Throwable>[] noRetryFor() default {};
+
+	/**
+	 * Exception types that are not recoverable; these exceptions are thrown to the caller
+	 * without calling any recoverer (immediately if also in {@link #noRetryFor()}).
+	 * Defaults to empty.
+	 * @return exception types not to retry
+	 * @since 2.0
+	 */
+	Class<? extends Throwable>[] notRecoverable() default {};
+
 	/**
 	 * A unique label for statistics reporting. If not provided the caller may choose to
 	 * ignore it, or provide a default.

src/main/java/org/springframework/retry/policy/SimpleRetryPolicy.java

@@ -16,6 +16,7 @@
 
 package org.springframework.retry.policy;
 
+import java.util.HashMap;
 import java.util.Map;
 import java.util.function.Supplier;
 
@@ -70,6 +71,8 @@ public class SimpleRetryPolicy implements RetryPolicy {
 
 	private BinaryExceptionClassifier retryableClassifier = new BinaryExceptionClassifier(false);
 
+	private BinaryExceptionClassifier recoverableClassifier = new BinaryExceptionClassifier(true);
+
 	/**
 	 * Create a {@link SimpleRetryPolicy} with the default number of retry attempts,
 	 * retrying all exceptions.
@@ -153,6 +156,20 @@ public void setMaxAttempts(int maxAttempts) {
 		this.maxAttempts = maxAttempts;
 	}
 
+	/**
+	 * Configure throwables that should not be passed to a recoverer (if present) but
+	 * thrown immediately.
+	 * @param noRecovery the throwables.
+	 * @since 3.0
+	 */
+	public void setNotRecoverable(Class<? extends Throwable>... noRecovery) {
+		Map<Class<? extends Throwable>, Boolean> map = new HashMap<>();
+		for (Class<? extends Throwable> clazz : noRecovery) {
+			map.put(clazz, false);
+		}
+		this.recoverableClassifier = new BinaryExceptionClassifier(map, true);
+	}
+
 	/**
 	 * Set a supplier for the number of attempts before retries are exhausted. Includes
 	 * the initial attempt before the retries begin so, generally, will be {@code >= 1}.
@@ -190,7 +207,14 @@ public int getMaxAttempts() {
 	@Override
 	public boolean canRetry(RetryContext context) {
 		Throwable t = context.getLastThrowable();
-		return (t == null || retryForException(t)) && context.getRetryCount() < getMaxAttempts();
+		boolean can = (t == null || retryForException(t)) && context.getRetryCount() < getMaxAttempts();
+		if (!can && !this.recoverableClassifier.classify(t)) {
+			context.setAttribute(RetryContext.NO_RECOVERY, true);
+		}
+		else {
+			context.removeAttribute(RetryContext.NO_RECOVERY);
+		}
+		return can;
 	}
 
 	/**

src/main/java/org/springframework/retry/support/RetryTemplate.java

@@ -537,20 +537,27 @@ protected <T> T handleRetryExhausted(RecoveryCallback<T> recoveryCallback, Retry
 		if (state != null && !context.hasAttribute(GLOBAL_STATE)) {
 			this.retryContextCache.remove(state.getKey());
 		}
+		boolean doRecover = !Boolean.TRUE.equals(context.getAttribute(RetryContext.NO_RECOVERY));
 		if (recoveryCallback != null) {
-			T recovered = recoveryCallback.recover(context);
-			context.setAttribute(RetryContext.RECOVERED, true);
-			return recovered;
+			if (doRecover) {
+				T recovered = recoveryCallback.recover(context);
+				context.setAttribute(RetryContext.RECOVERED, true);
+				return recovered;
+			}
+			else {
+				logger.debug("Retry exhausted and recovery disabled for this throwable");
+			}
 		}
 		if (state != null) {
 			this.logger.debug("Retry exhausted after last attempt with no recovery path.");
-			rethrow(context, "Retry exhausted after last attempt with no recovery path");
+			rethrow(context, "Retry exhausted after last attempt with no recovery path",
+					this.throwLastExceptionOnExhausted || !doRecover);
 		}
 		throw wrapIfNecessary(context.getLastThrowable());
 	}
 
-	protected <E extends Throwable> void rethrow(RetryContext context, String message) throws E {
-		if (this.throwLastExceptionOnExhausted) {
+	protected <E extends Throwable> void rethrow(RetryContext context, String message, boolean wrap) throws E {
+		if (wrap) {
 			@SuppressWarnings("unchecked")
 			E rethrow = (E) context.getLastThrowable();
 			throw rethrow;

src/test/java/org/springframework/retry/annotation/EnableRetryTests.java

@@ -48,7 +48,6 @@
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 import static org.assertj.core.api.Assertions.setMaxStackTraceElementsDisplayed;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -121,7 +120,13 @@ public void recovery() {
 		RecoverableService service = context.getBean(RecoverableService.class);
 		service.service();
 		assertThat(service.getCount()).isEqualTo(3);
-		assertNotNull(service.getCause());
+		assertThat(service.getCause()).isExactlyInstanceOf(RuntimeException.class);
+		assertThatIllegalArgumentException().isThrownBy(() -> service.service());
+		assertThat(service.getCount()).isEqualTo(6);
+		assertThat(service.getCause()).isExactlyInstanceOf(RuntimeException.class);
+		assertThatIllegalStateException().isThrownBy(() -> service.service());
+		assertThat(service.getCount()).isEqualTo(7);
+		assertThat(service.getCause()).isExactlyInstanceOf(RuntimeException.class);
 		context.close();
 	}
 
@@ -582,10 +587,18 @@ protected static class RecoverableService {
 
 		boolean otherAdviceCalled;
 
-		@Retryable(RuntimeException.class)
+		@Retryable(retryFor = RuntimeException.class, noRetryFor = IllegalStateException.class,
+				notRecoverable = { IllegalArgumentException.class, IllegalStateException.class })
 		public void service() {
-			this.count++;
-			throw new RuntimeException("Planned");
+			if (this.count++ >= 3 && count < 7) {
+				throw new IllegalArgumentException("Planned");
+			}
+			else if (count > 6) {
+				throw new IllegalStateException("Planned");
+			}
+			else {
+				throw new RuntimeException("Planned");
+			}
 		}
 
 		@Recover
@@ -632,7 +645,7 @@ protected static class ExcludesService {
 
 		private int count = 0;
 
-		@Retryable(include = RuntimeException.class, exclude = IllegalStateException.class)
+		@Retryable(retryFor = RuntimeException.class, noRetryFor = IllegalStateException.class)
 		public void service() {
 			if (this.count++ < 2) {
 				throw new IllegalStateException("Planned");
@@ -651,7 +664,7 @@ protected static class ExcludesOnlyService {
 
 		private RuntimeException exceptionToThrow;
 
-		@Retryable(exclude = IllegalStateException.class)
+		@Retryable(noRetryFor = IllegalStateException.class)
 		public void service() {
 			if (this.count++ < 2) {
 				throw this.exceptionToThrow;