Skip to content

Commit 12246fe

Browse files
chihyu0917chihyu0917
authored
GH-496: Make RecoverAnnotationRecoveryHandler deterministic
Fixes: #496 The `RecoverAnnotationRecoveryHandler` occasionally invokes an unexpected `@Recover` method when more than one candidate is a valid match. The outcome depends on the order of `Class#getDeclaredMethods()`, which is not specified by the JVM. In real builds this shows up as flaky tests or different behavior between JDK distributions/OS-es. * When multiple `@Recover` candidates exist, choose the most specific and deterministic handler instead of relying on reflection order. Signed-off-by: Chih-Yu Huang <selina221947@gmail.com>
1 parent 307c1f1 commit 12246fe

File tree

1 file changed

+120
-67
lines changed

1 file changed

+120
-67
lines changed

‎src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java‎

Lines changed: 120 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,13 @@
1919
import java.lang.reflect.Method;
2020
import java.lang.reflect.ParameterizedType;
2121
import java.lang.reflect.Type;
22-
import java.util.HashMap;
22+
import java.util.Arrays;
23+
import java.util.List;
24+
import java.util.ArrayList;
25+
import java.util.Comparator;
26+
import java.util.LinkedHashMap;
2327
import java.util.Map;
28+
import java.util.stream.Collectors;
2429

2530
import org.springframework.classify.SubclassClassifier;
2631
import org.springframework.core.annotation.AnnotatedElementUtils;
@@ -55,12 +60,13 @@
5560
* @author Gianluca Medici
5661
* @author Lijinliang
5762
* @author Yanming Zhou
63+
* @author Chih-Yu Huang
5864
*/
5965
public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationRecoverer<T> {
6066

6167
private final SubclassClassifier<Throwable, Method> classifier = new SubclassClassifier<>();
6268

63-
private final Map<Method, SimpleMetadata> methods = new HashMap<>();
69+
private final Map<Method, SimpleMetadata> methods = new LinkedHashMap<>();
6470

6571
private final Object target;
6672

@@ -73,7 +79,8 @@ public RecoverAnnotationRecoveryHandler(Object target, Method method) {
7379

7480
@Override
7581
public T recover(Object[] args, Throwable cause) {
76-
Method method = findClosestMatch(args, cause.getClass());
82+
Class<? extends Throwable> causeType = (cause == null) ? null : cause.getClass();
83+
Method method = findClosestMatch(args, causeType);
7784
if (method == null) {
7885
throw new ExhaustedRetryException("Cannot locate recovery method", cause);
7986
}
@@ -112,50 +119,91 @@ private Method findMethodOnProxy(Method method, Object proxy) {
112119
}
113120

114121
private Method findClosestMatch(Object[] args, Class<? extends Throwable> cause) {
115-
Method result = null;
122+
if (StringUtils.hasText(this.recoverMethodName)) {
123+
return findMethodByName(args, cause);
124+
}
116125

117-
if (!StringUtils.hasText(this.recoverMethodName)) {
118-
int min = Integer.MAX_VALUE;
119-
for (Map.Entry<Method, SimpleMetadata> entry : this.methods.entrySet()) {
120-
Method method = entry.getKey();
121-
SimpleMetadata meta = entry.getValue();
122-
Class<? extends Throwable> type = meta.getType();
123-
if (type == null) {
124-
type = Throwable.class;
126+
List<Method> withThrowable = new ArrayList<>();
127+
List<Method> withoutThrowable = new ArrayList<>();
128+
for (Method method : this.methods.keySet()) {
129+
SimpleMetadata meta = this.methods.get(method);
130+
if (meta.getType() != null) {
131+
withThrowable.add(method);
132+
}
133+
else {
134+
withoutThrowable.add(method);
135+
}
136+
}
137+
138+
Method result = findMethodWithThrowable(args, cause, withThrowable);
139+
if (result == null) {
140+
result = findMethodWithNoThrowable(args, withoutThrowable);
141+
}
142+
return result;
143+
}
144+
145+
private static Method findMethodWithNoThrowable(Object[] args, List<Method> methods) {
146+
Method result = null;
147+
for (Method method : methods) {
148+
if (compareParameters(args, method.getParameterTypes(), false)) {
149+
if (result == null || result.getParameterCount() < method.getParameterCount()) {
150+
result = method;
125151
}
126-
if (type.isAssignableFrom(cause)) {
127-
int distance = calculateDistance(cause, type);
128-
if (distance < min) {
129-
min = distance;
130-
result = method;
152+
}
153+
}
154+
return result;
155+
}
156+
157+
private Method findMethodWithThrowable(Object[] args, Class<? extends Throwable> cause, List<Method> methods) {
158+
Method result = null;
159+
int minDistance = Integer.MAX_VALUE;
160+
List<Method> candidates = new ArrayList<>();
161+
162+
if (cause != null) {
163+
for (Method method : methods) {
164+
SimpleMetadata meta = this.methods.get(method);
165+
Class<? extends Throwable> exceptionType = meta.getType();
166+
if (exceptionType.isAssignableFrom(cause)) {
167+
int distance = calculateDistance(cause, exceptionType);
168+
if (distance < minDistance) {
169+
minDistance = distance;
170+
candidates.clear();
171+
candidates.add(method);
131172
}
132-
else if (distance == min) {
133-
boolean parametersMatch = compareParameters(args, meta.getArgCount(),
134-
method.getParameterTypes(), false);
135-
if (parametersMatch) {
136-
result = method;
137-
}
173+
else if (distance == minDistance) {
174+
candidates.add(method);
138175
}
139176
}
140177
}
141178
}
142-
else {
143-
for (Map.Entry<Method, SimpleMetadata> entry : this.methods.entrySet()) {
144-
Method method = entry.getKey();
145-
if (method.getName().equals(this.recoverMethodName)) {
146-
SimpleMetadata meta = entry.getValue();
147-
if ((meta.type == null || meta.type.isAssignableFrom(cause))
148-
&& compareParameters(args, meta.getArgCount(), method.getParameterTypes(), true)) {
149-
result = method;
150-
break;
151-
}
179+
180+
for (Method method : candidates) {
181+
if (compareParameters(args, method.getParameterTypes(), true)) {
182+
if (result == null || result.getParameterCount() < method.getParameterCount()) {
183+
result = method;
152184
}
153185
}
154186
}
155187
return result;
156188
}
157189

158-
private int calculateDistance(Class<? extends Throwable> cause, Class<? extends Throwable> type) {
190+
private Method findMethodByName(Object[] args, Class<? extends Throwable> cause) {
191+
for (Map.Entry<Method, SimpleMetadata> entry : this.methods.entrySet()) {
192+
Method method = entry.getKey();
193+
if (method.getName().equals(this.recoverMethodName)) {
194+
SimpleMetadata meta = entry.getValue();
195+
Class<? extends Throwable> exceptionType = meta.getType();
196+
if (exceptionType == null || (cause != null && exceptionType.isAssignableFrom(cause))) {
197+
if (compareParameters(args, method.getParameterTypes(), exceptionType != null)) {
198+
return method;
199+
}
200+
}
201+
}
202+
}
203+
return null;
204+
}
205+
206+
private static int calculateDistance(Class<?> cause, Class<?> type) {
159207
int result = 0;
160208
Class<?> current = cause;
161209
while (current != type && current != Throwable.class) {
@@ -165,53 +213,59 @@ private int calculateDistance(Class<? extends Throwable> cause, Class<? extends
165213
return result;
166214
}
167215

168-
private boolean compareParameters(Object[] args, int argCount, Class<?>[] parameterTypes,
169-
boolean withRecoverMethodName) {
170-
if ((withRecoverMethodName && argCount == args.length) || argCount == (args.length + 1)) {
171-
int startingIndex = 0;
172-
if (parameterTypes.length > 0 && Throwable.class.isAssignableFrom(parameterTypes[0])) {
173-
startingIndex = 1;
216+
private static boolean compareParameters(Object[] args, Class<?>[] parameterTypes, boolean hasThrowable) {
217+
int argCount = args.length;
218+
int paramCount = parameterTypes.length;
219+
int argIndex = 0;
220+
int paramIndex = hasThrowable ? 1 : 0;
221+
222+
while (paramIndex < paramCount) {
223+
Class<?> parameterType = parameterTypes[paramIndex];
224+
Object argument = (argIndex < argCount) ? args[argIndex] : null;
225+
226+
if (argument == null && parameterType.isPrimitive()) {
227+
return false;
174228
}
175-
for (int i = startingIndex; i < parameterTypes.length; i++) {
176-
final Object argument = i - startingIndex < args.length ? args[i - startingIndex] : null;
177-
if (argument == null) {
178-
continue;
179-
}
180-
Class<?> parameterType = parameterTypes[i];
181-
parameterType = ClassUtils.resolvePrimitiveIfNecessary(parameterType);
182-
if (!parameterType.isAssignableFrom(argument.getClass())) {
183-
return false;
184-
}
229+
if (argument != null && !ClassUtils.isAssignable(parameterType, argument.getClass())) {
230+
return false;
185231
}
186-
return true;
232+
paramIndex++;
233+
argIndex++;
187234
}
188-
return false;
235+
return true;
189236
}
190237

191238
private void init(final Object target, Method method) {
192-
final Map<Class<? extends Throwable>, Method> types = new HashMap<>();
239+
final Map<Class<? extends Throwable>, Method> types = new LinkedHashMap<>();
193240
final Method failingMethod = method;
194241
Retryable retryable = AnnotatedElementUtils.findMergedAnnotation(method, Retryable.class);
195242
if (retryable != null) {
196243
this.recoverMethodName = retryable.recover();
197244
}
198-
ReflectionUtils.doWithMethods(target.getClass(), candidate -> {
245+
Method[] declared = target.getClass().getDeclaredMethods();
246+
Arrays.sort(declared, Comparator.comparing(Method::getName)
247+
.thenComparingInt(Method::getParameterCount)
248+
.thenComparing(
249+
m -> Arrays.stream(m.getParameterTypes()).map(Class::getName).collect(Collectors.joining(","))));
250+
251+
for (Method candidate : declared) {
199252
Recover recover = AnnotatedElementUtils.findMergedAnnotation(candidate, Recover.class);
200253
if (recover == null) {
201254
recover = findAnnotationOnTarget(target, candidate);
202255
}
203-
if (recover != null && failingMethod.getGenericReturnType() instanceof ParameterizedType
204-
&& candidate.getGenericReturnType() instanceof ParameterizedType) {
205-
if (isParameterizedTypeAssignable((ParameterizedType) candidate.getGenericReturnType(),
206-
(ParameterizedType) failingMethod.getGenericReturnType())) {
256+
if (recover != null) {
257+
if (failingMethod.getGenericReturnType() instanceof ParameterizedType
258+
&& candidate.getGenericReturnType() instanceof ParameterizedType) {
259+
if (isParameterizedTypeAssignable((ParameterizedType) candidate.getGenericReturnType(),
260+
(ParameterizedType) failingMethod.getGenericReturnType())) {
261+
putToMethodsMap(candidate, types);
262+
}
263+
}
264+
else if (candidate.getReturnType().isAssignableFrom(failingMethod.getReturnType())) {
207265
putToMethodsMap(candidate, types);
208266
}
209267
}
210-
else if (recover != null && candidate.getReturnType().isAssignableFrom(failingMethod.getReturnType())) {
211-
putToMethodsMap(candidate, types);
212-
}
213-
});
214-
this.classifier.setTypeMap(types);
268+
}
215269
optionallyFilterMethodsBy(failingMethod.getReturnType());
216270
}
217271

@@ -261,11 +315,10 @@ private void putToMethodsMap(Method method, Map<Class<? extends Throwable>, Meth
261315
@SuppressWarnings("unchecked")
262316
Class<? extends Throwable> type = (Class<? extends Throwable>) parameterTypes[0];
263317
types.put(type, method);
264-
RecoverAnnotationRecoveryHandler.this.methods.put(method, new SimpleMetadata(parameterTypes.length, type));
318+
this.methods.put(method, new SimpleMetadata(parameterTypes.length, type));
265319
}
266320
else {
267-
RecoverAnnotationRecoveryHandler.this.classifier.setDefaultValue(method);
268-
RecoverAnnotationRecoveryHandler.this.methods.put(method, new SimpleMetadata(parameterTypes.length, null));
321+
this.methods.put(method, new SimpleMetadata(parameterTypes.length, null));
269322
}
270323
}
271324

@@ -280,7 +333,7 @@ private Recover findAnnotationOnTarget(Object target, Method method) {
280333
}
281334

282335
private void optionallyFilterMethodsBy(Class<?> returnClass) {
283-
Map<Method, SimpleMetadata> filteredMethods = new HashMap<>();
336+
Map<Method, SimpleMetadata> filteredMethods = new LinkedHashMap<>();
284337
for (Method method : this.methods.keySet()) {
285338
if (method.getReturnType() == returnClass) {
286339
filteredMethods.put(method, this.methods.get(method));

0 commit comments

Comments
 (0)