[
https://issues.apache.org/jira/browse/JCS-248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108844#comment-18108844
]
Thomas Buffagni commented on JCS-248:
-------------------------------------
Thanks for the follow-up work on JCS-248. I reviewed the second lifecycle
change and I think it addresses the main ownership issue much more cleanly,
especially for {{ElementEventQueue}} and shared pools.
While looking at the new reference-counting logic, I noticed a few edge cases
that may be worth checking. I may of course be missing some intended lifecycle
constraints.
*1. Possible reference-count underflow*
In {{{}disposeExecutorService(){}}}, {{poolUseCounts.computeIfAbsent()}}
creates a counter with value {{0}} even if no corresponding acquisition exists,
and it is then immediately decremented.
So a sequence such as:
{{dispose("X") -> count becomes -1
get("X") -> count becomes 0, executor is created
dispose("X") -> count becomes -1}}
would appear to leave the executor alive rather than shutting it down.
A similar situation might occur if a component performs its local dispose after
the global {{ThreadPoolManager.dispose()}} has already removed the counters.
Would it be safer for the release path to use {{poolUseCounts.get()}} and
simply ignore/log an unmatched release instead of creating a new counter?
*2. Reference count is incremented before executor creation succeeds*
{{getExecutorService()}} increments the use count before {{computeIfAbsent()}}
creates the executor.
If pool creation throws for any reason, the caller never receives an executor,
but the reference count has already been incremented. A subsequent successful
acquisition could therefore inherit a phantom reference and require an
additional release before the pool is actually stopped.
It might be safer to increment the reference count only after the executor has
been successfully obtained.
*3. {{getExecutorService()}} now effectively has acquire/release semantics*
With reference counting, every call to {{getExecutorService()}} now represents
an acquisition that must eventually be balanced by
{{{}disposeExecutorService(){}}}.
That makes sense for shared pools, but it is slightly different from the
previous "getter" semantics and may be worth documenting explicitly, or perhaps
even expressing through acquire/release terminology, to avoid accidental
unbalanced calls in future code.
One additional concurrency detail: {{disposeExecutorService()}} is synchronized
and performs {{awaitTermination()}} while holding the {{ThreadPoolManager}}
monitor. If a terminating worker needs to obtain another pool, it could block
waiting for the same monitor while the disposing thread is waiting for that
worker to terminate. The timeout prevents a permanent deadlock, but it could
still block the whole pool manager until the timeout expires.
None of these points changes the fact that the second patch looks like a
significant improvement over the first lifecycle implementation. I just wanted
to highlight these scenarios in case they are useful for additional tests or
hardening.
> ElementEventQueue.dispose() does not shut down its owned executor, leaking
> threads across web application redeployments
> -----------------------------------------------------------------------------------------------------------------------
>
> Key: JCS-248
> URL: https://issues.apache.org/jira/browse/JCS-248
> Project: Commons JCS
> Issue Type: Bug
> Components: Composite Cache
> Affects Versions: jcs-3.2.1, jcs-4.0
> Environment: Apache Commons JCS 3.2.1; Apache Tomcat 11.0.24; Eclipse
> Temurin JDK 25; Spring web application packaged as a WAR; local
> non-distributed cache.
> Reporter: Thomas Buffagni
> Assignee: Thomas Vandahl
> Priority: Major
> Labels: thread-leak,, tomcat,, webapp-lifecycle
> Fix For: jcs-4.0
>
>
> *How the issue was discovered*
> The issue was discovered while running a Tomcat web-application lifecycle
> benchmark. The benchmark repeatedly performs the following sequence:
> 1. Deploy the Spring WAR.
> 2. Initialize and exercise the local JCS cache.
> 3. Stop the Spring application context and invoke JCS.shutdown().
> 4. Undeploy the WAR from Tomcat.
> 5. Deploy it again and repeat the sequence.
> During WAR undeployment, Tomcat reported that threads created by the web
> application had not been stopped. The threads named in the warnings were
> JCS-ElementEventQueue-* workers.
> Inspection of the JVM after undeployment confirmed that two
> JCS-ElementEventQueue-* worker threads remained alive even though the
> application had been stopped and JCS.shutdown() had been invoked.
> Repeating the deploy/undeploy cycle caused additional worker threads to
> accumulate and produced further Tomcat thread-leak warnings. After five
> lifecycle cycles, ten warnings had been recorded and the number of live
> threads showed an estimated growth of 2.3 threads per cycle.
> This undeployment behavior led to the inspection of
> ElementEventQueue.dispose() and to the identification of the executor
> lifecycle problem described below.
> *Problem*
> ElementEventQueue creates its own executor by calling:
> ThreadPoolManager.getInstance().createPool(...)
> The returned executor is not registered in the ThreadPoolManager internal
> pool maps. Consequently, ThreadPoolManager.dispose() cannot shut it down.
> ElementEventQueue.dispose() sets the destroyed flag, but the
> queueProcessor.shutdownNow() call is commented out. When JCS is used inside a
> Tomcat web application, the executor threads survive application undeployment.
> The issue was reproduced at runtime with JCS 3.2.1. Code inspection confirms
> that the same lifecycle problem is present in the current JCS 4.0.0-SNAPSHOT
> source.
> *Steps to reproduce*
> 1. Deploy a Spring WAR that configures and uses a local JCS cache.
> 2. Execute a workload that creates the ElementEventQueue workers.
> 3. Invoke JCS.shutdown() while stopping the Spring application context.
> 4. Undeploy the WAR from Tomcat.
> 5. Deploy the WAR again and repeat the lifecycle cycle.
> 6. Inspect the Tomcat logs and live JVM threads after each undeployment.
> *Actual result*
> Two additional JCS-ElementEventQueue-* threads remain alive after each
> application lifecycle cycle.
> In a five-cycle deploy/workload/undeploy test with JCS 3.2.1, the unpatched
> implementation produced:
> - 10 Tomcat thread-leak warnings
> - final thread counts of 39, 41, 43, 46, and 48
> - an estimated thread-count slope of +2.3 threads per cycle
> *Expected result*
> ElementEventQueue.dispose() should terminate the executor owned by the queue.
> No JCS-ElementEventQueue-* worker should remain alive after JCS shutdown and
> WAR undeployment.
> *Root cause*
> ElementEventQueue obtains a newly created and unregistered executor from
> ThreadPoolManager.createPool(). Because the queue owns this executor, it must
> also terminate it explicitly.
> *Proposed fix*
> Call queueProcessor.shutdownNow() during the first execution of
> ElementEventQueue.dispose().
> A regression test verifies that:
> - the worker thread is running before disposal
> - the worker terminates after disposal
> - repeated calls to dispose() remain safe
> *Validation*
> The patched JCS 3.2.1 JAR was built from source, packaged inside the test
> WAR, and tested through five complete Tomcat deploy/workload/undeploy cycles.
> *Results after the patch:*
> - 0 Tomcat thread-leak warnings
> - final thread counts of 38, 38, 37, 37, and 38
> - an estimated thread-count slope of -0.1 threads per cycle
> The corresponding focused regression test also passes against the JCS 4
> source tree.
> A pull request containing the fix and regression test will be submitted after
> this issue provides the JCS issue identifier.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
