CAMEL-20199: Deprecate camel-reactive-executor-tomcat

[gnodet]

Summary

Deprecate camel-reactive-executor-tomcat instead of refactoring it.

The cross-thread ThreadLocal cleanup that distinguished this component from DefaultReactiveExecutor relied on reflective access to the private Thread.threadLocals field. This approach:

• Is denied by the JDK module system since JDK 17 (--illegal-access=deny is the default)
• Is incompatible with virtual threads (different thread-local storage structure)
• Was already silently failing (all reflection exceptions were swallowed)

Without that cleanup, this executor is functionally identical to the default. Rather than maintaining a clone, this PR deprecates the component with clear migration guidance.

Changes

• @Deprecated(since = "4.22") on TomcatReactiveExecutor
• (deprecated) suffix in pom.xml name
• Updated component documentation with deprecation notice and migration instructions
• Upgrade guide entry in camel-4x-upgrade-guide-4_22.adoc

Migration

Remove camel-reactive-executor-tomcat from your dependencies. Camel will automatically use the built-in DefaultReactiveExecutor.

Claude Code on behalf of @gnodet

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

[github-actions]

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

• First-time contributors require MANUAL approval for the GitHub Actions to run
• You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
• You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
• Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

[github-actions]

🧪 CI tested the following changed modules:

• catalog/camel-catalog
• components/camel-reactive-executor-tomcat
• docs

---

🔬 Scalpel shadow comparison — Scalpel: 11 tested, 27 compile-only — current: 9 all tested

Maveniverse Scalpel detected 38 affected modules (current approach: 9).

⚠️ Modules only in Scalpel (29)

• apache-camel
• camel-allcomponents
• camel-catalog
• camel-catalog-console
• camel-catalog-lucene
• camel-catalog-maven
• camel-catalog-suggest
• camel-componentdsl
• camel-csimple-maven-plugin
• camel-endpointdsl
• camel-endpointdsl-support
• camel-itest
• camel-jbang-core
• camel-jbang-it
• camel-jbang-main
• camel-jbang-plugin-edit
• camel-jbang-plugin-generate
• camel-jbang-plugin-kubernetes
• camel-jbang-plugin-test
• camel-kamelet-main
• camel-launcher
• camel-report-maven-plugin
• camel-route-parser
• camel-yaml-dsl
• camel-yaml-dsl-deserializers
• camel-yaml-dsl-maven-plugin
• coverage
• docs
• dummy-component

Skip-tests mode would test 11 modules (3 direct + 8 downstream), skip tests for 27 (generated code, meta-modules)

Modules Scalpel would test (11)

• camel-catalog
• camel-jbang-mcp
• camel-jbang-plugin-mcp
• camel-jbang-plugin-route-parser
• camel-jbang-plugin-tui
• camel-jbang-plugin-validate
• camel-launcher-container
• camel-reactive-executor-tomcat
• camel-yaml-dsl-validator
• camel-yaml-dsl-validator-maven-plugin
• docs

Modules with tests skipped (27)

• apache-camel
• camel-allcomponents
• camel-catalog-console
• camel-catalog-lucene
• camel-catalog-maven
• camel-catalog-suggest
• camel-componentdsl
• camel-csimple-maven-plugin
• camel-endpointdsl
• camel-endpointdsl-support
• camel-itest
• camel-jbang-core
• camel-jbang-it
• camel-jbang-main
• camel-jbang-plugin-edit
• camel-jbang-plugin-generate
• camel-jbang-plugin-kubernetes
• camel-jbang-plugin-test
• camel-kamelet-main
• camel-launcher
• camel-report-maven-plugin
• camel-route-parser
• camel-yaml-dsl
• camel-yaml-dsl-deserializers
• camel-yaml-dsl-maven-plugin
• coverage
• dummy-component

ℹ️ Shadow mode — Scalpel observes but does not affect test execution. Learn more

All tested modules (38 modules)

• Camel :: All Components Sync point
• Camel :: Assembly
• Camel :: Catalog :: CSimple Maven Plugin (deprecated)
• Camel :: Catalog :: Camel Catalog
• Camel :: Catalog :: Camel Report Maven Plugin
• Camel :: Catalog :: Camel Route Parser
• Camel :: Catalog :: Console
• Camel :: Catalog :: Dummy Component
• Camel :: Catalog :: Lucene (deprecated)
• Camel :: Catalog :: Maven
• Camel :: Catalog :: Suggest
• Camel :: Component DSL
• Camel :: Coverage
• Camel :: Docs
• Camel :: Endpoint DSL
• Camel :: Endpoint DSL :: Support
• Camel :: Integration Tests
• Camel :: JBang :: Core
• Camel :: JBang :: Integration tests
• Camel :: JBang :: MCP
• Camel :: JBang :: Main
• Camel :: JBang :: Plugin :: Edit
• Camel :: JBang :: Plugin :: Generate
• Camel :: JBang :: Plugin :: Kubernetes
• Camel :: JBang :: Plugin :: MCP
• Camel :: JBang :: Plugin :: Route Parser
• Camel :: JBang :: Plugin :: TUI
• Camel :: JBang :: Plugin :: Testing
• Camel :: JBang :: Plugin :: Validate
• Camel :: Kamelet Main
• Camel :: Launcher
• Camel :: Launcher :: Container
• Camel :: Reactive Executor :: Tomcat (deprecated)
• Camel :: YAML DSL
• Camel :: YAML DSL :: Deserializers
• Camel :: YAML DSL :: Maven Plugins
• Camel :: YAML DSL :: Validator
• Camel :: YAML DSL :: Validator Maven Plugin

---

⚙️ View full build and test results

[gnodet]

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 calls incrementPendingTasks(), 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

[gnodet]

Review Summary (informational — not a formal approval)

Clean, well-structured migration from broken reflection-based ThreadLocal cleanup to ContextValue. The PR achieves exact structural parity with DefaultReactiveExecutor and fixes two latent bugs along the way.

Key observations

1. Bug fix in clearWorkers(): The old reflection-based cleanup was non-functional — ObjectHelper.invokeMethodSafe(m, map, this) passed this (the TomcatReactiveExecutor instance) to ThreadLocalMap.remove(ThreadLocal), which expects a ThreadLocal key. The correct argument would have been workers. Since the old cleanup never actually removed anything, the new workers.remove() is not a regression.

2. Bug fix in scheduleQueue(): Now correctly calls incrementPendingTasks(), which the old code omitted. The old code would silently skip the increment, causing the pending task counter to go negative when executeFromQueue() decremented it.

3. Dynamic statistics: Statistics checking is now dynamic (reads executor.statisticsEnabled per call) instead of cached at Worker construction time, matching DefaultReactiveExecutor.

Suggestions

• Consider updating the class-level Javadoc to reflect the current cleanup mechanism, since it still describes the Tomcat classloader-leak workaround that has been removed.
• The broader architectural question of whether TomcatReactiveExecutor still serves a distinct purpose from DefaultReactiveExecutor (now that cross-thread cleanup is removed) may be worth a follow-up discussion on the JIRA ticket. The @Experimental annotation gives flexibility here.

Test convention updates are correct: dropped public from class and test methods (JUnit 5 convention).

Note: This is an informational review only — an agent cannot approve its operator's own PRs. Human review is required.

---

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of @gnodet

[davsclaus]

Thanks for the cleanup work here @gnodet.

The core concern is that removing the cross-thread ThreadLocal cleanup breaks the component's primary purpose. The old clearWorkers() iterated through all tracked threads and reflectively removed the ThreadLocal entry from each one — this was specifically designed to prevent Tomcat's thread-leak detector from firing on undeploy (CAMEL-17588). The new workers.remove() in doShutdown() only removes the value for the current thread (the shutdown thread), leaving ThreadLocals on all other Tomcat worker threads intact.

After this PR, TomcatReactiveExecutor is functionally identical to DefaultReactiveExecutor — same Worker class, same shutdown behavior. The only remaining difference is the @JdkService annotation and class name.

Given that:

• The component is @Experimental
• The old reflection hack was fragile and incompatible with virtual threads
• The distinguishing feature (cross-thread cleanup) is now removed

I think we should deprecate this component rather than refactoring it into a clone of DefaultReactiveExecutor. If the leak detection no longer works, there's no reason for this component to exist as a separate module.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of @davsclaus

[davsclaus]

This workers.remove() only clears the ThreadLocal for the current thread (the shutdown thread). The old clearWorkers() iterated through all tracked Tomcat worker threads and removed the ThreadLocal entry from each one — that was the whole point of this component.

With this change, ThreadLocals on all other Tomcat pool threads remain, which is the exact leak the old code was designed to prevent. Since the cross-thread cleanup is gone, this component is now functionally identical to DefaultReactiveExecutor and should be deprecated instead.

[gnodet]

Thanks for the thorough review — you're absolutely right. The cross-thread ThreadLocal cleanup was the entire raison d'être for this component, and migrating to ContextValue (which only cleans the calling thread) would make it a clone of DefaultReactiveExecutor.

I've reworked the PR to deprecate the component instead:

• Reverted the ContextValue migration (back to original code)
• Added @Deprecated(since = \"4.22\") with a clear Javadoc explaining why
• Added (deprecated) suffix in pom.xml
• Updated the component docs with a deprecation notice and migration path
• Added an upgrade guide entry

The old reflection hack was already silently broken on JDK 17+ (--illegal-access=deny is the default), and incompatible with virtual threads. Deprecation is the right call.

_Claude Code on behalf of