davsclaus commented on code in PR #25239:
URL: https://github.com/apache/camel/pull/25239#discussion_r3701568437
##########
components/camel-reactive-executor-tomcat/src/main/java/org/apache/camel/reactive/tomcat/TomcatReactiveExecutor.java:
##########
@@ -48,68 +43,16 @@ public class TomcatReactiveExecutor extends ServiceSupport
implements ReactiveEx
private static final Logger LOG =
LoggerFactory.getLogger(TomcatReactiveExecutor.class);
- private static final int MAX_TRACKING_SIZE = 1000;
-
- // to keep track of threads in use
- private final ConcurrentMap<Thread, Field> threads = new
ConcurrentHashMap<>();
- private final TrackingThreadLocal workers = new TrackingThreadLocal();
-
// use for statistics so we have insights at runtime
private boolean statisticsEnabled;
private final AtomicInteger createdWorkers = new AtomicInteger();
private final LongAdder runningWorkers = new LongAdder();
private final LongAdder pendingTasks = new LongAdder();
- /**
- * ThreadLocal which keep tracks of all the threads that are using it, to
ensure we can remove all these threads
- * when Camel is shutting down to not keep stale ThreadLocal which can
have some application servers report this as
- * a potential thread-leak (such as Apache Tomcat).
- */
- private final class TrackingThreadLocal extends ThreadLocal<Worker> {
-
- @Override
- protected Worker initialValue() {
- try {
- // fail-safe to store max
- if (threads.size() < MAX_TRACKING_SIZE) {
- Thread t = Thread.currentThread();
- Field f = Thread.class.getDeclaredField("threadLocals");
- threads.putIfAbsent(t, f);
- }
- } catch (Exception e) {
- // ignore
- }
- int number = createdWorkers.incrementAndGet();
- return new Worker(number, TomcatReactiveExecutor.this);
- }
-
- @Override
- public String toString() {
- return "CamelTomcatReactiveWorker";
- }
- }
-
- private void clearWorkers() {
- int size = threads.size();
- threads.forEach((t, f) -> {
- try {
- Object map = ReflectionHelper.getField(f, t);
- if (map != null) {
- Method m = ReflectionHelper.findMethod(map.getClass(),
"remove", ThreadLocal.class);
- if (m != null) {
- ObjectHelper.invokeMethodSafe(m, map, this);
- }
- }
- } catch (Exception e) {
- // ignore
- }
- });
- threads.clear();
-
- if (size > 0) {
- LOG.info("Cleared {} ThreadLocals", size);
- }
- }
+ private final ContextValue<Worker> workers =
ContextValue.newThreadLocal("CamelTomcatReactiveWorker", () -> {
+ int number = createdWorkers.incrementAndGet();
+ return new Worker(number, TomcatReactiveExecutor.this);
+ });
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
