Skip to content

fix: make RecoverAnnotationRecoveryHandler deterministic #497

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.

Sign up for GitHub

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

Merged
artembilan merged 2 commits into from
Oct 2, 2025
Merged

fix: make RecoverAnnotationRecoveryHandler deterministic #497

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9705743
fix: make RecoverAnnotationRecoveryHandler deterministic
chihyu0917 Oct 1, 2025
09ec5f1
refactor: Mark utility methods as static per code review
chihyu0917 Oct 2, 2025
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
187 changes: 120 additions & 67 deletions src/main/java/org/springframework/retry/annotation/RecoverAnnotationRecoveryHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,13 @@
import java.lang.reflect.Method;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.util.HashMap;
import java.util.Arrays;
import java.util.List;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.stream.Collectors;

import org.springframework.classify.SubclassClassifier;
import org.springframework.core.annotation.AnnotatedElementUtils;
Expand Down Expand Up @@ -55,12 +60,13 @@
* @author Gianluca Medici
* @author Lijinliang
* @author Yanming Zhou
* @author Chih-Yu Huang
*/
public class RecoverAnnotationRecoveryHandler<T> implements MethodInvocationRecoverer<T> {

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

private final Map<Method, SimpleMetadata> methods = new HashMap<>();
private final Map<Method, SimpleMetadata> methods = new LinkedHashMap<>();

private final Object target;

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

@Override
public T recover(Object[] args, Throwable cause) {
Method method = findClosestMatch(args, cause.getClass());
Class<? extends Throwable> causeType = (cause == null) ? null : cause.getClass();
Method method = findClosestMatch(args, causeType);
if (method == null) {
throw new ExhaustedRetryException("Cannot locate recovery method", cause);
}
Expand Down Expand Up @@ -112,50 +119,91 @@ private Method findMethodOnProxy(Method method, Object proxy) {
}

private Method findClosestMatch(Object[] args, Class<? extends Throwable> cause) {
Method result = null;
if (StringUtils.hasText(this.recoverMethodName)) {
return findMethodByName(args, cause);
}

if (!StringUtils.hasText(this.recoverMethodName)) {
int min = Integer.MAX_VALUE;
for (Map.Entry<Method, SimpleMetadata> entry : this.methods.entrySet()) {
Method method = entry.getKey();
SimpleMetadata meta = entry.getValue();
Class<? extends Throwable> type = meta.getType();
if (type == null) {
type = Throwable.class;
List<Method> withThrowable = new ArrayList<>();
List<Method> withoutThrowable = new ArrayList<>();
for (Method method : this.methods.keySet()) {
SimpleMetadata meta = this.methods.get(method);
if (meta.getType() != null) {
withThrowable.add(method);
}
else {
withoutThrowable.add(method);
}
}

Method result = findMethodWithThrowable(args, cause, withThrowable);
if (result == null) {
result = findMethodWithNoThrowable(args, withoutThrowable);
}
return result;
}

private Method findMethodWithNoThrowable(Object[] args, List<Method> methods) {
Copy link
Member

@artembilan artembilan Oct 2, 2025

Choose a reason for hiding this comment

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

Looks like this can be static and therefore goes to the end of class.

Method result = null;
for (Method method : methods) {
if (compareParameters(args, method.getParameterTypes(), false)) {
if (result == null || result.getParameterCount() < method.getParameterCount()) {
result = method;
}
if (type.isAssignableFrom(cause)) {
int distance = calculateDistance(cause, type);
if (distance < min) {
min = distance;
result = method;
}
}
return result;
}

private Method findMethodWithThrowable(Object[] args, Class<? extends Throwable> cause, List<Method> methods) {
Method result = null;
int minDistance = Integer.MAX_VALUE;
List<Method> candidates = new ArrayList<>();

if (cause != null) {
for (Method method : methods) {
SimpleMetadata meta = this.methods.get(method);
Class<? extends Throwable> exceptionType = meta.getType();
if (exceptionType.isAssignableFrom(cause)) {
int distance = calculateDistance(cause, exceptionType);
if (distance < minDistance) {
minDistance = distance;
candidates.clear();
candidates.add(method);
}
else if (distance == min) {
boolean parametersMatch = compareParameters(args, meta.getArgCount(),
method.getParameterTypes(), false);
if (parametersMatch) {
result = method;
}
else if (distance == minDistance) {
candidates.add(method);
}
}
}
}
else {
for (Map.Entry<Method, SimpleMetadata> entry : this.methods.entrySet()) {
Method method = entry.getKey();
if (method.getName().equals(this.recoverMethodName)) {
SimpleMetadata meta = entry.getValue();
if ((meta.type == null || meta.type.isAssignableFrom(cause))
&& compareParameters(args, meta.getArgCount(), method.getParameterTypes(), true)) {
result = method;
break;
}

for (Method method : candidates) {
if (compareParameters(args, method.getParameterTypes(), true)) {
if (result == null || result.getParameterCount() < method.getParameterCount()) {
result = method;
}
}
}
return result;
}

private int calculateDistance(Class<? extends Throwable> cause, Class<? extends Throwable> type) {
private Method findMethodByName(Object[] args, Class<? extends Throwable> cause) {
for (Map.Entry<Method, SimpleMetadata> entry : this.methods.entrySet()) {
Method method = entry.getKey();
if (method.getName().equals(this.recoverMethodName)) {
SimpleMetadata meta = entry.getValue();
Class<? extends Throwable> exceptionType = meta.getType();
if (exceptionType == null || (cause != null && exceptionType.isAssignableFrom(cause))) {
if (compareParameters(args, method.getParameterTypes(), exceptionType != null)) {
return method;
}
}
}
}
return null;
}

private int calculateDistance(Class<?> cause, Class<?> type) {
Copy link
Member

@artembilan artembilan Oct 2, 2025

Choose a reason for hiding this comment

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

Also static

int result = 0;
Class<?> current = cause;
while (current != type && current != Throwable.class) {
Expand All @@ -165,53 +213,59 @@ private int calculateDistance(Class<? extends Throwable> cause, Class<? extends
return result;
}

private boolean compareParameters(Object[] args, int argCount, Class<?>[] parameterTypes,
boolean withRecoverMethodName) {
if ((withRecoverMethodName && argCount == args.length) || argCount == (args.length + 1)) {
int startingIndex = 0;
if (parameterTypes.length > 0 && Throwable.class.isAssignableFrom(parameterTypes[0])) {
startingIndex = 1;
private boolean compareParameters(Object[] args, Class<?>[] parameterTypes, boolean hasThrowable) {
Copy link
Member

@artembilan artembilan Oct 2, 2025

Choose a reason for hiding this comment

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

static?

int argCount = args.length;
int paramCount = parameterTypes.length;
int argIndex = 0;
int paramIndex = hasThrowable ? 1 : 0;

while (paramIndex < paramCount) {
Class<?> parameterType = parameterTypes[paramIndex];
Object argument = (argIndex < argCount) ? args[argIndex] : null;

if (argument == null && parameterType.isPrimitive()) {
return false;
}
for (int i = startingIndex; i < parameterTypes.length; i++) {
final Object argument = i - startingIndex < args.length ? args[i - startingIndex] : null;
if (argument == null) {
continue;
}
Class<?> parameterType = parameterTypes[i];
parameterType = ClassUtils.resolvePrimitiveIfNecessary(parameterType);
if (!parameterType.isAssignableFrom(argument.getClass())) {
return false;
}
if (argument != null && !ClassUtils.isAssignable(parameterType, argument.getClass())) {
return false;
}
return true;
paramIndex++;
argIndex++;
}
return false;
return true;
}

private void init(final Object target, Method method) {
final Map<Class<? extends Throwable>, Method> types = new HashMap<>();
final Map<Class<? extends Throwable>, Method> types = new LinkedHashMap<>();
final Method failingMethod = method;
Retryable retryable = AnnotatedElementUtils.findMergedAnnotation(method, Retryable.class);
if (retryable != null) {
this.recoverMethodName = retryable.recover();
}
ReflectionUtils.doWithMethods(target.getClass(), candidate -> {
Method[] declared = target.getClass().getDeclaredMethods();
Arrays.sort(declared, Comparator.comparing(Method::getName)
.thenComparingInt(Method::getParameterCount)
.thenComparing(
m -> Arrays.stream(m.getParameterTypes()).map(Class::getName).collect(Collectors.joining(","))));

for (Method candidate : declared) {
Recover recover = AnnotatedElementUtils.findMergedAnnotation(candidate, Recover.class);
if (recover == null) {
recover = findAnnotationOnTarget(target, candidate);
}
if (recover != null && failingMethod.getGenericReturnType() instanceof ParameterizedType
&& candidate.getGenericReturnType() instanceof ParameterizedType) {
if (isParameterizedTypeAssignable((ParameterizedType) candidate.getGenericReturnType(),
(ParameterizedType) failingMethod.getGenericReturnType())) {
if (recover != null) {
if (failingMethod.getGenericReturnType() instanceof ParameterizedType
&& candidate.getGenericReturnType() instanceof ParameterizedType) {
if (isParameterizedTypeAssignable((ParameterizedType) candidate.getGenericReturnType(),
(ParameterizedType) failingMethod.getGenericReturnType())) {
putToMethodsMap(candidate, types);
}
}
else if (candidate.getReturnType().isAssignableFrom(failingMethod.getReturnType())) {
putToMethodsMap(candidate, types);
}
}
else if (recover != null && candidate.getReturnType().isAssignableFrom(failingMethod.getReturnType())) {
putToMethodsMap(candidate, types);
}
});
this.classifier.setTypeMap(types);
}
optionallyFilterMethodsBy(failingMethod.getReturnType());
}

Expand Down Expand Up @@ -261,11 +315,10 @@ private void putToMethodsMap(Method method, Map<Class<? extends Throwable>, Meth
@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));
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));
this.methods.put(method, new SimpleMetadata(parameterTypes.length, null));
}
}

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

private void optionallyFilterMethodsBy(Class<?> returnClass) {
Map<Method, SimpleMetadata> filteredMethods = new HashMap<>();
Map<Method, SimpleMetadata> filteredMethods = new LinkedHashMap<>();
for (Method method : this.methods.keySet()) {
if (method.getReturnType() == returnClass) {
filteredMethods.put(method, this.methods.get(method));
Expand Down