Summary
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 behaviour between
JDK distributions/OS-es.
Current behaviour
- If several
@Recover methods are equally suitable (same distance in the
exception hierarchy, same parameter compatibility), the handler iterates over
this.methods.entrySet() — a HashMap backed by whatever order the JVM gave
us — and returns the last method that satisfies the tie-break rules.
- Because that order changes, the selected method is not deterministic.
Expected behaviour
Given the same set of @Recover methods the handler should always choose the
same target method, independent of JVM or library versions.
Reproduction
The built-in tests already expose the problem when run under
NonDex 2.1.7:
| test |
command (20 random seeds) |
typical failure |
| extends Throwable |
mvn -pl . edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.springframework.retry.annotation.RecoverAnnotationRecoveryHandlerTests#multipleQualifyingRecoverMethodsExtendsThrowable -DnondexRuns=20 |
expected: 2 but was: 1 |
| no Throwable |
mvn -pl . edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.springframework.retry.annotation.RecoverAnnotationRecoveryHandlerTests#multipleQualifyingRecoverMethodsWithNoThrowable -DnondexRuns=20 |
IllegalArgumentException |
| null Throwable |
mvn -pl . edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.springframework.retry.annotation.RecoverAnnotationRecoveryHandlerTests#multipleQualifyingRecoverMethodsWithNull -DnondexRuns=20 |
IllegalArgumentException |
Analysis
HashMap order + JVM reflection order ⇒ non-deterministic.- When two candidates tie, the current algorithm does not consider
which one is “more specific” in terms of first-parameter presence vs. absence.
Proposed fix (already working locally)
- Store
this.methods in a LinkedHashMap to preserve insertion order.
- Collect candidates in a stable order
– Arrays.sort by name ↑, parameter count ↑, full parameter type string ↑ –
then insert in reverse so that later methods override earlier ones in ties
(existing Spring convention: “declaration order wins”).
- Keep the original distance/parameter checks; no new tie-break rules needed.
After applying the above, all three flaky tests pass consistently (50 NonDex
runs with random seeds).
Environment
| Item |
Version |
| Spring-Retry |
2.0.6-SNAPSHOT (main) |
| JDK |
Temurin 17.0.9 & Oracle 17.0.9 |
| OS |
Ubuntu 22.04 |
| NonDex |
2.1.7 |
Contribution checklist
Summary
RecoverAnnotationRecoveryHandleroccasionally invokes an unexpected@Recovermethod when more than one candidate is a valid match.
The outcome depends on the order of
Class#getDeclaredMethods(), which is notspecified by the JVM.
In real builds this shows up as flaky tests or different behaviour between
JDK distributions/OS-es.
Current behaviour
@Recovermethods are equally suitable (same distance in theexception hierarchy, same parameter compatibility), the handler iterates over
this.methods.entrySet()— aHashMapbacked by whatever order the JVM gaveus — and returns the last method that satisfies the tie-break rules.
Expected behaviour
Given the same set of
@Recovermethods the handler should always choose thesame target method, independent of JVM or library versions.
Reproduction
The built-in tests already expose the problem when run under
NonDex 2.1.7:
mvn -pl . edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.springframework.retry.annotation.RecoverAnnotationRecoveryHandlerTests#multipleQualifyingRecoverMethodsExtendsThrowable -DnondexRuns=20expected: 2 but was: 1mvn -pl . edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.springframework.retry.annotation.RecoverAnnotationRecoveryHandlerTests#multipleQualifyingRecoverMethodsWithNoThrowable -DnondexRuns=20IllegalArgumentExceptionmvn -pl . edu.illinois:nondex-maven-plugin:2.1.7:nondex -Dtest=org.springframework.retry.annotation.RecoverAnnotationRecoveryHandlerTests#multipleQualifyingRecoverMethodsWithNull -DnondexRuns=20IllegalArgumentExceptionAnalysis
HashMaporder + JVM reflection order ⇒ non-deterministic.which one is “more specific” in terms of first-parameter presence vs. absence.
Proposed fix (already working locally)
this.methodsin aLinkedHashMapto preserve insertion order.–
Arrays.sortby name ↑, parameter count ↑, full parameter type string ↑ –then insert in reverse so that later methods override earlier ones in ties
(existing Spring convention: “declaration order wins”).
After applying the above, all three flaky tests pass consistently (50 NonDex
runs with random seeds).
Environment
Contribution checklist
Signed-off-by:trailer (DCO)./mvnw spring-javaformat:apply