Fix concurrency races in CancellableFuture and Options setMethodOptions

[saikat709]

Fixes two concurrency bugs in feign-core and adds test coverage for both scenarios.

Summary of Changes

• CancellableFuture race fix: Switches inner to an AtomicReference<CompletableFuture<T>> and adds an isCancelled() re-check after setInner() to propagate cancellation immediately if cancel() arrives before setInner().
• Options.setMethodOptions race fix: Uses computeIfAbsent with an inner ConcurrentHashMap to eliminate the check-then-act race on threadToMethodOptions.
• Request.Body immutability: Makes data final and delegates the no-arg Body() constructor to this(null).

Tests Added

• CancellableFutureTest: Verifies cancellation propagation before response/retry, cancellation after retry, and normal completion.
• OptionsTest#concurrentSetMethodOptionsDoesNotThrow: Verifies multi-threaded concurrent calls to setMethodOptions.

$ ./mvnw test -pl core -Dtest=CancellableFutureTest,OptionsTest -Dtoolchain.skip=true
Tests run: 9, Failures: 0, Errors: 0, Skipped: 0

[velo]

The production fixes here look sound. I traced all four cancel()/setInner() interleavings in CancellableFuture and every one correctly ends with the inner future cancelled (CompletableFuture.cancel() is idempotent, so the redundant double-cancel in some paths is harmless). Request.Options' computeIfAbsent + ConcurrentHashMap is the right pattern and removes an unconditional allocation on every lookup. Request.Body.data going final is a clean, strictly-safe cleanup.

The blocking issue is the tests: I ran CancellableFutureTest and OptionsTest#concurrentSetMethodOptionsDoesNotThrow against the pre-fix code (merge-base), and all three pass identically whether or not the fix is applied. They don't exercise the race. Root cause: the custom AsyncClient lambdas in CancellableFutureTest block with CountDownLatch.await(...) inside execute(), which runs synchronously on the caller's thread for a custom client — so api.get() itself blocks on that latch before returning, meaning result.cancel(true) is only ever reached after the call chain has already finished, never concurrently with setInner(). The ~2s timings are the tests burning out their own internal await timeout, not synchronizing with a second thread — there is no second thread here. Same story for the map test: getThreadIdentifier() already produces a unique key per live thread, so ConcurrentHashMap already guarantees safety for distinct keys, and the new test doesn't force two threads to contend on the same key either.

Could you rewrite these to actually force the interleaving — e.g. drive setInner()/cancel() from two independent threads coordinated by latches, with the mock client returning immediately instead of blocking inside execute() — or, if a specific production incident motivated this, link it? As shipped this merges as "tested" but leaves both races uncovered, so a future refactor could reintroduce either bug with CI staying green.

Two small non-blocking notes: inner only ever calls get()/set(), never compareAndSet/updateAndGet, so a plain volatile CompletableFuture<T> inner would give the same happens-before guarantees with less indirection than AtomicReference. And setInner() now has a side effect (can trigger a cancellation) that its name doesn't suggest anymore — worth a rename or a comment.

Happy to merge once the tests actually cover the race.

[saikat709]

Thanks for the thorough review @velo. You were right on all counts. Here's what was changed:

• CancellableFutureTest — rewritten to force real concurrency
  The old tests blocked inside execute(), so api.get() never returned until the client was done — cancel() was never concurrent with setInner(). Fixed by returning immediately from execute() with a pending CompletableFuture and moving all coordination latches to the caller:

• cancelBeforeSetInnerRacesCorrectly — cancels the outer future before completing clientFuture; when clientFuture then resolves, setInner() sees isCancelled()==true and immediately forwards cancellation to the newly registered inner future.

• cancelAfterSetInnerRacesCorrectly — retry execute() returns immediately; a latch confirms setInner() has been called before cancel() fires; then verifies the pipeTo guard (isDone() check) prevents the subsequent retry completion from overwriting the cancellation.
  OptionsTest — force real contention on the same outer key

Since getThreadIdentifier() returns a unique key per thread, the old test never had two threads contending on the same computeIfAbsent slot. Added a protected String threadIdentifier() hook to Options (wrapping Util.getThreadIdentifier()), then overrode it in a test-only SharedKeyOptions subclass to return "shared-key". All 20 threads now collide on the same outer map key. The assertion was also strengthened: all 20 entries must be present after completion, not just "no exception".

Minor production polish (per your notes)

AtomicReference<CompletableFuture> inner → volatile CompletableFuture inner (we only ever read/write, never CAS)
Added Javadoc to setInner() documenting the cancellation side-effect
All 9 tests pass: Tests run: 9, Failures: 0, Errors: 0, Skipped: 0

[velo]

Thanks — this is a good round. The test rewrite is what I was after: coordinating the latches from the caller instead of blocking inside execute() is the only way those races actually reproduce, and forcing all 20 threads onto one outer key in OptionsTest is the right call. volatile on inner and the javadoc on setInner() are both fine.

Two things before I merge.

1. protected String threadIdentifier() on Request.Options. I don't want to ship production API whose own javadoc says it exists for tests:

Subclasses may override this to provide a fixed identifier, which is useful in tests to force concurrent threads to contend on the same outer map key.

Request.Options is public, so that's permanent surface. getMethodOptions/setMethodOptions are @Experimental and excluded from japicmp, but threadIdentifier() isn't — once it ships I can't remove it without a major bump.

Any of these works for me, in order of preference:

• Drop it, and have the test drive contention another way — a fixed-size pool where threads genuinely collide, or a subclass overriding getMethodOptions/setMethodOptions.
• Keep it, but mark it @Experimental so it stays out of the binary contract.
• Make it package-private — OptionsTest is in feign, so it has access.

2. CI. setup-environment died in wget fetching mvnd/1.0.2, which Apache removed from downloads.apache.org. That's fixed on master since #3557 (mvnd 1.0.6), but CircleCI builds your branch head, which still pins 1.0.2 — so re-running alone won't help. Merge master into fix/concurrency-races and it should go green.

The rest looks right. computeIfAbsent is safe here since threadToMethodOptions is already a ConcurrentHashMap, and making Body.data final is a good cleanup.

[saikat709]

Fair enough. Synced with latest main and made that threadIdentifier() method package private. Check again when you have time. @velo