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