CAMEL-20199: Migrate TomcatReactiveExecutor from raw ThreadLocal to ContextValue - #25239
CAMEL-20199: Migrate TomcatReactiveExecutor from raw ThreadLocal to ContextValue#25239gnodet wants to merge 1 commit into
Conversation
…ontextValue Replace the reflection-based TrackingThreadLocal with ContextValue.newThreadLocal() to match the pattern already used in DefaultReactiveExecutor. This removes the fragile Thread.threadLocals reflection hack that breaks with virtual threads and aligns the Tomcat reactive executor with the virtual thread compatibility work. Changes: - Replace TrackingThreadLocal with ContextValue.newThreadLocal() - Remove ConcurrentHashMap thread tracking and reflection-based clearWorkers() - Refactor Worker class to use extracted methods (matching DefaultReactiveExecutor) - Use workers.remove() for clean shutdown instead of reflection - Drop public from test class/methods per JUnit 5 conventions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Looks good ✅
Clean, well-structured migration from fragile reflection-based ThreadLocal cleanup to ContextValue, achieving full parity with DefaultReactiveExecutor. The old reflection cleanup in clearWorkers() was actually broken (passed this instead of the ThreadLocal to ThreadLocalMap.remove), so this is both a modernization and a correctness improvement.
Notable behavioral fixes vs. the old code:
scheduleQueue()now correctly callsincrementPendingTasks(), fixing a stats accounting bug where pending counter could go negative- Statistics checking is now dynamic (rechecked per call) rather than cached at Worker construction time
One note on the PR description: it states "With ContextValue, on JDK 25+ this automatically uses ScopedValue instead of ThreadLocal." This is inaccurate for this specific usage — the code uses ContextValue.newThreadLocal(), which always uses ThreadLocal regardless of JDK version. Only ContextValue.newInstance() would use ScopedValue on JDK 25+. The newThreadLocal variant is necessary here because Workers need mutable state and remove() support, which ScopedValue does not provide.
CI passes on both JDK 17 and JDK 25.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
1 participant
Summary
Claude Code on behalf of @gnodet
Migrates
TomcatReactiveExecutorfrom rawThreadLocalwith reflection-based cleanup toContextValue.newThreadLocal(), achieving parity withDefaultReactiveExecutorwhich was already migrated.What changed:
TrackingThreadLocalinner class (which used fragileThread.class.getDeclaredField("threadLocals")reflection) withContextValue.newThreadLocal("CamelTomcatReactiveWorker", ...)ConcurrentHashMap<Thread, Field>thread tracking and the reflection-basedclearWorkers()methodworkers.remove()for clean cleanupWorkerclass to use extracted helper methods (matchingDefaultReactiveExecutor's structure)publicfrom test class/methods per JUnit 5 conventionsWhy it matters for virtual threads:
Thread.threadLocalsfield access) does not work with virtual threadsContextValue, on JDK 25+ this automatically usesScopedValueinstead ofThreadLocalArrayDequequeues are on the routing hot path — with rawThreadLocaland virtual threads, a new Worker was created per VT and never reusedPart of the umbrella issue CAMEL-20199 (Complete support of Virtual Threads).
Test plan
SimpleMockTestpasses (2 tests)🤖 Generated with Claude Code