GH-237: Fix Multiple RetryListeners for @Retryable

Resolves https://github.com/spring-projects/spring-retry/issues/237 - also don't include non-singletons or perform eager inits - `spring-javaformat` has reformatted some other code * Update main branch name in README. * Fix `getBeansOfType()`.
2021-04-20 14:53:09 -04:00
parent 5bdad2fe54
commit 7692782d37
4 changed files with 142 additions and 97 deletions
--- a/README.md
+++ b/README.md
@@ -623,7 +623,7 @@ interceptor.

 Spring Retry is released under the non-restrictive Apache 2.0 license
 and follows a very standard Github development process, using Github
-tracker for issues and merging pull requests into the master branch. If you want
+tracker for issues and merging pull requests into the main branch. If you want
 to contribute even something trivial, please do not hesitate, but do please
 follow the guidelines in the next paragraph.

@@ -637,6 +637,6 @@ ability to merge pull requests.

 ## Code of Conduct

-This project adheres to the [Contributor Covenant](https://github.com/spring-projects/spring-retry/blob/master/CODE_OF_CONDUCT.adoc).
+This project adheres to the [Contributor Covenant](https://github.com/spring-projects/spring-retry/blob/main/CODE_OF_CONDUCT.adoc).
 By participating, you  are expected to uphold this code. Please report unacceptable behavior to
 spring-code-of-conduct@pivotal.io.
--- a/src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java
+++ b/src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java
@@ -106,8 +106,8 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
 						result = method;
 					}
 					else if (distance == min) {
-						boolean parametersMatch = compareParameters(args,
-								meta.getArgCount(), method.getParameterTypes());
+						boolean parametersMatch = compareParameters(args, meta.getArgCount(),
+								method.getParameterTypes());
 						if (parametersMatch) {
 							result = method;
 						}
@@ -120,8 +120,8 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
 				Method method = entry.getKey();
 				if (method.getName().equals(this.recoverMethodName)) {
 					SimpleMetadata meta = entry.getValue();
-					if (meta.type.isAssignableFrom(cause) && compareParameters(args,
-							meta.getArgCount(), method.getParameterTypes())) {
+					if (meta.type.isAssignableFrom(cause)
+							&& compareParameters(args, meta.getArgCount(), method.getParameterTypes())) {
 						result = method;
 						break;
 					}
@@ -131,8 +131,7 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
 		return result;
 	}

-	private int calculateDistance(Class<? extends Throwable> cause,
-			Class<? extends Throwable> type) {
+	private int calculateDistance(Class<? extends Throwable> cause, Class<? extends Throwable> type) {
 		int result = 0;
 		Class<?> current = cause;
 		while (current != type && current != Throwable.class) {
@@ -142,17 +141,14 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
 		return result;
 	}

-	private boolean compareParameters(Object[] args, int argCount,
-			Class<?>[] parameterTypes) {
+	private boolean compareParameters(Object[] args, int argCount, Class<?>[] parameterTypes) {
 		if (argCount == (args.length + 1)) {
 			int startingIndex = 0;
-			if (parameterTypes.length > 0
-					&& Throwable.class.isAssignableFrom(parameterTypes[0])) {
+			if (parameterTypes.length > 0 && Throwable.class.isAssignableFrom(parameterTypes[0])) {
 				startingIndex = 1;
 			}
 			for (int i = startingIndex; i < parameterTypes.length; i++) {
-				final Object argument = i - startingIndex < args.length
-						? args[i - startingIndex] : null;
+				final Object argument = i - startingIndex < args.length ? args[i - startingIndex] : null;
 				if (argument == null) {
 					continue;
 				}
@@ -174,24 +170,19 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
 		}
 		ReflectionUtils.doWithMethods(target.getClass(), new MethodCallback() {
 			@Override
-			public void doWith(Method method)
-					throws IllegalArgumentException, IllegalAccessException {
+			public void doWith(Method method) throws IllegalArgumentException, IllegalAccessException {
 				Recover recover = AnnotationUtils.findAnnotation(method, Recover.class);
 				if (recover == null) {
 					recover = findAnnotationOnTarget(target, method);
 				}
-				if (recover != null
-						&& failingMethod
-								.getGenericReturnType() instanceof ParameterizedType
+				if (recover != null && failingMethod.getGenericReturnType() instanceof ParameterizedType
 						&& method.getGenericReturnType() instanceof ParameterizedType) {
-					if (isParameterizedTypeAssignable(
-							(ParameterizedType) method.getGenericReturnType(),
+					if (isParameterizedTypeAssignable((ParameterizedType) method.getGenericReturnType(),
 							(ParameterizedType) failingMethod.getGenericReturnType())) {
 						putToMethodsMap(method, types);
 					}
 				}
-				else if (recover != null && method.getReturnType()
-						.isAssignableFrom(failingMethod.getReturnType())) {
+				else if (recover != null && method.getReturnType().isAssignableFrom(failingMethod.getReturnType())) {
 					putToMethodsMap(method, types);
 				}
 			}
@@ -221,8 +212,7 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
 		for (int i = startingIndex; i < methodActualArgs.length; i++) {
 			Type methodArgType = methodActualArgs[i];
 			Type failingMethodArgType = failingMethodActualArgs[i];
-			if (methodArgType instanceof ParameterizedType
-					&& failingMethodArgType instanceof ParameterizedType) {
+			if (methodArgType instanceof ParameterizedType && failingMethodArgType instanceof ParameterizedType) {
 				return isParameterizedTypeAssignable((ParameterizedType) methodArgType,
 						(ParameterizedType) failingMethodArgType);
 			}
@@ -234,28 +224,23 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
 		return true;
 	}

-	private void putToMethodsMap(Method method,
-			Map<Class<? extends Throwable>, Method> types) {
+	private void putToMethodsMap(Method method, Map<Class<? extends Throwable>, Method> types) {
 		Class<?>[] parameterTypes = method.getParameterTypes();
-		if (parameterTypes.length > 0
-				&& Throwable.class.isAssignableFrom(parameterTypes[0])) {
+		if (parameterTypes.length > 0 && Throwable.class.isAssignableFrom(parameterTypes[0])) {
 			@SuppressWarnings("unchecked")
 			Class<? extends Throwable> type = (Class<? extends Throwable>) parameterTypes[0];
 			types.put(type, method);
-			RecoverAnnotationRecoveryHandler.this.methods.put(method,
-					new SimpleMetadata(parameterTypes.length, type));
+			RecoverAnnotationRecoveryHandler.this.methods.put(method, new SimpleMetadata(parameterTypes.length, type));
 		}
 		else {
 			RecoverAnnotationRecoveryHandler.this.classifier.setDefaultValue(method);
-			RecoverAnnotationRecoveryHandler.this.methods.put(method,
-					new SimpleMetadata(parameterTypes.length, null));
+			RecoverAnnotationRecoveryHandler.this.methods.put(method, new SimpleMetadata(parameterTypes.length, null));
 		}
 	}

 	private Recover findAnnotationOnTarget(Object target, Method method) {
 		try {
-			Method targetMethod = target.getClass().getMethod(method.getName(),
-					method.getParameterTypes());
+			Method targetMethod = target.getClass().getMethod(method.getName(), method.getParameterTypes());
 			return AnnotationUtils.findAnnotation(targetMethod, Recover.class);
 		}
 		catch (Exception e) {
@@ -302,8 +287,7 @@ public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationReco
 				result[0] = t;
 				startArgs = 1;
 			}
-			int length = result.length - startArgs > args.length ? args.length
-					: result.length - startArgs;
+			int length = result.length - startArgs > args.length ? args.length : result.length - startArgs;
 			if (length == 0) {
 				return result;
 			}
--- a/src/main/java/org/springframework/retry/annotation/RetryConfiguration.java
+++ b/src/main/java/org/springframework/retry/annotation/RetryConfiguration.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2014 the original author or authors.
+ * Copyright 2014-2021 the original author or authors.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@@ -25,6 +25,7 @@ import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;

 import org.aopalliance.aop.Advice;
+
 import org.springframework.aop.ClassFilter;
 import org.springframework.aop.IntroductionAdvisor;
 import org.springframework.aop.MethodMatcher;
@@ -38,7 +39,6 @@ import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryAware;
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.beans.factory.ListableBeanFactory;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.core.OrderComparator;
 import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.retry.RetryListener;
@@ -50,7 +50,6 @@ import org.springframework.stereotype.Component;
 import org.springframework.util.ObjectUtils;
 import org.springframework.util.ReflectionUtils;
 import org.springframework.util.ReflectionUtils.MethodCallback;
-import org.springframework.util.comparator.ComparableComparator;

 /**
 * Basic configuration for <code>@Retryable</code> processing. For stateful retry, if
@@ -86,25 +85,25 @@ public class RetryConfiguration extends AbstractPointcutAdvisor

 	@Override
 	public void afterPropertiesSet() throws Exception {
-		retryContextCache = findBean(RetryContextCache.class);
-		methodArgumentsKeyGenerator = findBean(MethodArgumentsKeyGenerator.class);
-		newMethodArgumentsIdentifier = findBean(NewMethodArgumentsIdentifier.class);
-		retryListeners = findBeans(RetryListener.class);
-		sleeper = findBean(Sleeper.class);
+		this.retryContextCache = findBean(RetryContextCache.class);
+		this.methodArgumentsKeyGenerator = findBean(MethodArgumentsKeyGenerator.class);
+		this.newMethodArgumentsIdentifier = findBean(NewMethodArgumentsIdentifier.class);
+		this.retryListeners = findBeans(RetryListener.class);
+		this.sleeper = findBean(Sleeper.class);
 		Set<Class<? extends Annotation>> retryableAnnotationTypes = new LinkedHashSet<Class<? extends Annotation>>(1);
 		retryableAnnotationTypes.add(Retryable.class);
 		this.pointcut = buildPointcut(retryableAnnotationTypes);
 		this.advice = buildAdvice();
 		if (this.advice instanceof BeanFactoryAware) {
-			((BeanFactoryAware) this.advice).setBeanFactory(beanFactory);
+			((BeanFactoryAware) this.advice).setBeanFactory(this.beanFactory);
 		}
 	}

 	private <T> List<T> findBeans(Class<? extends T> type) {
-		if (beanFactory instanceof ListableBeanFactory) {
-			ListableBeanFactory listable = (ListableBeanFactory) beanFactory;
-			if (listable.getBeanNamesForType(type).length == 1) {
-				ArrayList<T> list = new ArrayList<T>(listable.getBeansOfType(type).values());
+		if (this.beanFactory instanceof ListableBeanFactory) {
+			ListableBeanFactory listable = (ListableBeanFactory) this.beanFactory;
+			if (listable.getBeanNamesForType(type).length > 0) {
+				ArrayList<T> list = new ArrayList<T>(listable.getBeansOfType(type, false, false).values());
 				OrderComparator.sort(list);
 				return list;
 			}
@@ -113,9 +112,9 @@ public class RetryConfiguration extends AbstractPointcutAdvisor
 	}

 	private <T> T findBean(Class<? extends T> type) {
-		if (beanFactory instanceof ListableBeanFactory) {
-			ListableBeanFactory listable = (ListableBeanFactory) beanFactory;
-			if (listable.getBeanNamesForType(type).length == 1) {
+		if (this.beanFactory instanceof ListableBeanFactory) {
+			ListableBeanFactory listable = (ListableBeanFactory) this.beanFactory;
+			if (listable.getBeanNamesForType(type, false, false).length == 1) {
 				return listable.getBean(type);
 			}
 		}
@@ -132,7 +131,7 @@ public class RetryConfiguration extends AbstractPointcutAdvisor

 	@Override
 	public ClassFilter getClassFilter() {
-		return pointcut.getClassFilter();
+		return this.pointcut.getClassFilter();
 	}

 	@Override
@@ -156,20 +155,20 @@ public class RetryConfiguration extends AbstractPointcutAdvisor

 	protected Advice buildAdvice() {
 		AnnotationAwareRetryOperationsInterceptor interceptor = new AnnotationAwareRetryOperationsInterceptor();
-		if (retryContextCache != null) {
-			interceptor.setRetryContextCache(retryContextCache);
+		if (this.retryContextCache != null) {
+			interceptor.setRetryContextCache(this.retryContextCache);
 		}
-		if (retryListeners != null) {
-			interceptor.setListeners(retryListeners);
+		if (this.retryListeners != null) {
+			interceptor.setListeners(this.retryListeners);
 		}
-		if (methodArgumentsKeyGenerator != null) {
-			interceptor.setKeyGenerator(methodArgumentsKeyGenerator);
+		if (this.methodArgumentsKeyGenerator != null) {
+			interceptor.setKeyGenerator(this.methodArgumentsKeyGenerator);
 		}
-		if (newMethodArgumentsIdentifier != null) {
-			interceptor.setNewItemIdentifier(newMethodArgumentsIdentifier);
+		if (this.newMethodArgumentsIdentifier != null) {
+			interceptor.setNewItemIdentifier(this.newMethodArgumentsIdentifier);
 		}
-		if (sleeper != null) {
-			interceptor.setSleeper(sleeper);
+		if (this.sleeper != null) {
+			interceptor.setSleeper(this.sleeper);
 		}
 		return interceptor;
 	}
@@ -253,7 +252,8 @@ public class RetryConfiguration extends AbstractPointcutAdvisor
 					if (found.get()) {
 						return;
 					}
-					Annotation annotation = AnnotationUtils.findAnnotation(method, annotationType);
+					Annotation annotation = AnnotationUtils.findAnnotation(method,
+							AnnotationMethodsResolver.this.annotationType);
 					if (annotation != null) {
 						found.set(true);
 					}
--- a/src/test/java/org/springframework/retry/annotation/EnableRetryTests.java
+++ b/src/test/java/org/springframework/retry/annotation/EnableRetryTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2019 the original author or authors.
+ * Copyright 2014-2021 the original author or authors.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
@@ -30,9 +30,14 @@ import org.springframework.context.annotation.AnnotationConfigApplicationContext
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.support.PropertySourcesPlaceholderConfigurer;
+import org.springframework.core.Ordered;
+import org.springframework.retry.RetryCallback;
+import org.springframework.retry.RetryContext;
+import org.springframework.retry.RetryListener;
 import org.springframework.retry.backoff.ExponentialBackOffPolicy;
 import org.springframework.retry.backoff.Sleeper;
 import org.springframework.retry.interceptor.RetryInterceptorBuilder;
+import org.springframework.retry.listener.RetryListenerSupport;
 import org.springframework.retry.policy.SimpleRetryPolicy;
 import org.springframework.retry.support.RetryTemplate;

@@ -60,6 +65,10 @@ public class EnableRetryTests {
 		assertTrue(AopUtils.isAopProxy(service));
 		service.service();
 		assertEquals(3, service.getCount());
+		TestConfiguration config = context.getBean(TestConfiguration.class);
+		assertTrue(config.listener1);
+		assertTrue(config.listener2);
+		assertTrue(config.twoFirst);
 		context.close();
 	}

@@ -279,6 +288,12 @@ public class EnableRetryTests {
 			return pspc;
 		}

+		boolean listener1;
+
+		boolean listener2;
+
+		protected boolean twoFirst;
+
 		@SuppressWarnings("serial")
 		@Bean
 		public Sleeper sleeper() {
@@ -294,6 +309,48 @@ public class EnableRetryTests {
 			return new Service();
 		}

+		@Bean
+		public RetryListener listener1() {
+			return new OrderedListener() {
+
+				@Override
+				public <T, E extends Throwable> boolean open(RetryContext context, RetryCallback<T, E> callback) {
+
+					TestConfiguration.this.listener1 = true;
+					TestConfiguration.this.twoFirst = true;
+					return super.open(context, callback);
+				}
+
+				@Override
+				public int getOrder() {
+					return Integer.MAX_VALUE;
+				}
+
+			};
+		}
+
+		@Bean
+		public RetryListener listener2() {
+			return new OrderedListener() {
+
+				private boolean listener1;
+
+				@Override
+				public <T, E extends Throwable> boolean open(RetryContext context, RetryCallback<T, E> callback) {
+
+					TestConfiguration.this.listener2 = true;
+					TestConfiguration.this.twoFirst = false;
+					return super.open(context, callback);
+				}
+
+				@Override
+				public int getOrder() {
+					return Integer.MIN_VALUE;
+				}
+
+			};
+		}
+
 		@Bean
 		public MultiService multiService() {
 			return new MultiService();
@@ -377,13 +434,13 @@ public class EnableRetryTests {

 		@Retryable(RuntimeException.class)
 		public void service() {
-			if (count++ < 2) {
+			if (this.count++ < 2) {
 				throw new RuntimeException("Planned");
 			}
 		}

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

 	}
@@ -394,20 +451,20 @@ public class EnableRetryTests {

 		@Retryable(RuntimeException.class)
 		public void service() {
-			if (count++ < 2) {
+			if (this.count++ < 2) {
 				throw new RuntimeException("Planned");
 			}
 		}

 		@Retryable(RuntimeException.class)
 		public void other() {
-			if (count++ < 3) {
+			if (this.count++ < 3) {
 				throw new RuntimeException("Other");
 			}
 		}

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

 	}
@@ -420,7 +477,7 @@ public class EnableRetryTests {

 		@Retryable(RuntimeException.class)
 		public void service() {
-			count++;
+			this.count++;
 			throw new RuntimeException("Planned");
 		}

@@ -430,11 +487,11 @@ public class EnableRetryTests {
 		}

 		public Throwable getCause() {
-			return cause;
+			return this.cause;
 		}

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

 	}
@@ -445,13 +502,13 @@ public class EnableRetryTests {
 		private int count = 0;

 		public void service() {
-			if (count++ < 2) {
+			if (this.count++ < 2) {
 				throw new RuntimeException("Planned");
 			}
 		}

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

 	}
@@ -462,13 +519,13 @@ public class EnableRetryTests {

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

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

 	}
@@ -481,13 +538,13 @@ public class EnableRetryTests {

 		@Retryable(exclude = IllegalStateException.class)
 		public void service() {
-			if (count++ < 2) {
-				throw exceptionToThrow;
+			if (this.count++ < 2) {
+				throw this.exceptionToThrow;
 			}
 		}

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

 		public void setExceptionToThrow(RuntimeException exceptionToThrow) {
@@ -502,13 +559,13 @@ public class EnableRetryTests {

 		@Retryable(stateful = true)
 		public void service(int value) {
-			if (count++ < 2) {
+			if (this.count++ < 2) {
 				throw new RuntimeException("Planned");
 			}
 		}

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

 	}
@@ -519,13 +576,13 @@ public class EnableRetryTests {

 		@Retryable(interceptor = "retryInterceptor")
 		public void service() {
-			if (count++ < 4) {
+			if (this.count++ < 4) {
 				throw new RuntimeException("Planned");
 			}
 		}

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

 	}
@@ -536,14 +593,14 @@ public class EnableRetryTests {

 		@Retryable(exceptionExpression = "#{message.contains('this can be retried')}")
 		public void service1() {
-			if (count++ < 2) {
+			if (this.count++ < 2) {
 				throw new RuntimeException("this can be retried");
 			}
 		}

 		@Retryable(exceptionExpression = "#{message.contains('this can be retried')}")
 		public void service2() {
-			count++;
+			this.count++;
 			throw new RuntimeException("this cannot be retried");
 		}

@@ -551,27 +608,27 @@ public class EnableRetryTests {
 				maxAttemptsExpression = "#{@integerFiveBean}", backoff = @Backoff(delayExpression = "#{${one}}",
 						maxDelayExpression = "#{${five}}", multiplierExpression = "#{${onePointOne}}"))
 		public void service3() {
-			if (count++ < 8) {
+			if (this.count++ < 8) {
 				throw new RuntimeException();
 			}
 		}

 		@Retryable(exceptionExpression = "message.contains('this can be retried')")
 		public void service4() {
-			if (count++ < 10) {
+			if (this.count++ < 10) {
 				throw new RuntimeException("this can be retried");
 			}
 		}

 		@Retryable(exceptionExpression = "message.contains('this can be retried')", include = RuntimeException.class)
 		public void service5() {
-			if (count++ < 11) {
+			if (this.count++ < 11) {
 				throw new RuntimeException("this can be retried");
 			}
 		}

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

 	}
@@ -614,21 +671,21 @@ public class EnableRetryTests {
 		@Override
 		@Retryable
 		public void service1() {
-			if (count++ < 1) {
+			if (this.count++ < 1) {
 				throw new RuntimeException("Planned");
 			}
 		}

 		@Override
 		public void service2() {
-			if (count++ < 3) {
+			if (this.count++ < 3) {
 				throw new RuntimeException("Planned");
 			}
 		}

 		@Override
 		public int getCount() {
-			return count;
+			return this.count;
 		}

 		@Override
@@ -697,23 +754,27 @@ public class EnableRetryTests {

 		@Override
 		public void service1() {
-			if (count++ < 2) {
+			if (this.count++ < 2) {
 				throw new RuntimeException("Planned");
 			}
 		}

 		@Override
 		public void service2() {
-			if (count++ < 4) {
+			if (this.count++ < 4) {
 				throw new RuntimeException("Planned");
 			}
 		}

 		@Override
 		public int getCount() {
-			return count;
+			return this.count;
 		}

 	}

+	public abstract static class OrderedListener extends RetryListenerSupport implements Ordered {
+
+	}
+
 }