-
Notifications
You must be signed in to change notification settings - Fork 536
Closed
Description
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
@Recovermethods are equally suitable (same distance in the
exception hierarchy, same parameter compatibility), the handler iterates over
this.methods.entrySet()— aHashMapbacked 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
HashMaporder + 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.methodsin aLinkedHashMapto preserve insertion order. - Collect candidates in a stable 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”). - 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
- Signed the Spring Individual CLA
- Commit(s) contain
Signed-off-by:trailer (DCO) - Code is formatted by
./mvnw spring-javaformat:apply - Added/updated unit tests – existing flaky tests now stable
- Squashed into a single commit with message: