[
https://issues.apache.org/jira/browse/JCS-248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110270#comment-18110270
]
Thomas Buffagni commented on JCS-248:
-------------------------------------
Thanks for the additional work on the thread-pool lifecycle. The latest changes
appear to address the reference-count underflow, the phantom acquisition on
pool-creation failure, and the lock being held during
{{{}awaitTermination(){}}}.
While reviewing the updated implementation, I noticed two remaining concurrency
scenarios that may be worth considering. I may, of course, be missing an
intended lifecycle constraint.
*1. Pool acquisition while global disposal is in progress*
{{ThreadPoolManager.dispose()}} processes {{pools}} and {{schedulerPools}}
under separate synchronized blocks, but the manager does not appear to enter a
global {{disposing}} or {{disposed}} state.
A concurrent thread may therefore acquire or create a pool while global
disposal is in progress—for example, after the ordinary-pool section has
completed, or between the ordinary and scheduler sections. Such a pool might
not be visited by the current {{dispose()}} invocation and could remain alive
after shutdown completes.
Would it be useful for the manager to reject new acquisitions once global
disposal begins, or otherwise coordinate acquisition and global disposal
through a common lifecycle state?
*2. Possible overlap between executor generations*
During an individual pool disposal, the existing executor is removed from the
map and shutdown is initiated. The map lock is then released before termination
is confirmed.
At that point, another thread can create a replacement executor under the same
name while tasks from the previous executor may still be terminating:
Â
{{P1 removed and shutdown initiated
P2 created under the same name
P1 still finishing tasks while P2 starts processing new tasks}}
For pools used to process ordered events, this temporary overlap could weaken
ordering guarantees across the lifecycle transition.
Would it be worth either preventing recreation until the previous generation
has terminated, or explicitly documenting that ordering is guaranteed only
within a single executor generation?
These points do not diminish the improvements in the latest commit; I wanted to
mention them in case they are useful for further lifecycle hardening or
regression tests.
> 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)
