fix: make RecoverAnnotationRecoveryHandler deterministic#497
fix: make RecoverAnnotationRecoveryHandler deterministic#497artembilan merged 2 commits intospring-projects:mainfrom
Conversation
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Signed-off-by: chihyuh3 <chihyuh3@fa25-cs527-010.cs.illinois.edu>
Please, use your legal name according to DCO requirements.
You can configure your Git client to provide a proper name.
Please, also add your name to the @author list of the affected class.
| } | ||
|
|
||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Please, ensure a new line in the end of file.
| @Override | ||
| public T recover(Object[] args, Throwable cause) { | ||
| Class<? extends Throwable> causeType = (cause == null) ? null : cause.getClass(); | ||
| Method method = findClosestMatch(args, cause.getClass()); |
There was a problem hiding this comment.
You have just extracted the type, but don't use it here.
chihyu0917
force-pushed
the
fix-bugs
branch
2 times, most recently
from
5ea121e to
42632b9
Compare
|
Thanks for your review! I corrected those mistakes that you mentioned.
|
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Please, run tests locally (./mvnw clean verify) before pushing:
Error: Failures:
Error: EnableRetryNoThreadLocalTests>EnableRetryTests.recoveryWithoutParam:151
expected: "test"
but was: null
Error: EnableRetryTests.recoveryWithoutParam:151
expected: "test"
but was: null
Error: RecoverAnnotationRecoveryHandlerTests.noMatch:105
Expecting actual throwable to be an instance of:
org.springframework.retry.ExhaustedRetryException
but was:
java.lang.IllegalArgumentException: argument type mismatch
Also, pay attention to Spring Java Format plugin: https://github.com/spring-io/spring-javaformat.
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>
|
Thanks for the review and feedback! My apologies for the regressions in the previous attempt. I have now run the full The root cause of the non-determinism was twofold: the unpredictable order of methods returned by reflection, and the use of unordered HashMaps which destroyed any stable ordering. This PR now makes the entire selection process deterministic by: Sorting Discovered Methods: The init method now sorts all found @recover methods alphabetically, ensuring a stable initial processing order. This approach resolves the original issue and passes all existing tests. Ready for another look. Thanks! |
| return result; | ||
| } | ||
|
|
||
| private Method findMethodWithNoThrowable(Object[] args, List<Method> methods) { |
There was a problem hiding this comment.
Looks like this can be static and therefore goes to the end of class.
| return null; | ||
| } | ||
|
|
||
| private int calculateDistance(Class<?> cause, Class<?> type) { |
| int startingIndex = 0; | ||
| if (parameterTypes.length > 0 && Throwable.class.isAssignableFrom(parameterTypes[0])) { | ||
| startingIndex = 1; | ||
| private boolean compareParameters(Object[] args, Class<?>[] parameterTypes, boolean hasThrowable) { |
Signed-off-by: Chih-Yu Huang <selina221947@gmail.com>
|
Following your suggestion, I’ve converted them to |
|
Thank you; and looking forward for more! |
|
Great, thanks! |
2 participants
This PR supersedes discussion in #496
When multiple @recover candidates exist, choose the most specific and deterministic handler instead of relying on reflection order. Previously the selected @recover method depended on HashMap iteration order. Switching to LinkedHashMap + deterministic sorting removes this problem. Verified by NonDex.