[
https://issues.apache.org/jira/browse/JCS-248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108356#comment-18108356
]
Thomas Buffagni commented on JCS-248:
-------------------------------------
Hi!
I reviewed the current implementation on {{main}} after the JCS-248 fix and I
think there are a few lifecycle/concurrency scenarios worth checking.
The main concern is that executor disposal is currently performed {*}by pool
name{*}, rather than by executor identity.
h3. 1. Possible ABA / wrong-generation shutdown
{{ElementEventQueue}} keeps its own {{queueProcessor}} reference, but on
dispose it calls:
{{ThreadPoolManager.getInstance().disposeExecutorService(POOL_NAME);}}
{{disposeExecutorService()}} then removes whatever executor is currently
registered under that name.
This can produce the following sequence:
{{Q1 -> gets P1
Q2 -> gets P1
Q2.dispose()
removes P1
shuts down P1
Q3 is created
creates and registers P2
Q1.dispose()
removes P2
shuts down P2}}
In this case an old queue instance ({{{}Q1{}}}) can shut down a {*}new executor
generation ({{{}P2{}}}) that it never used{*}.
A possible improvement would be an identity-aware dispose, for example:
{{disposeExecutorService(poolName, expectedExecutor)}}
using something equivalent to:
{{pools.remove(poolName, expectedExecutor);}}
before shutting it down.
----
h3. 2. {{ElementEventQueue}} instances now appear to share one JVM-wide executor
All {{ElementEventQueue}} instances currently use the same fixed pool name:
{{"ElementEventQueue"}}
This looks like a semantic change compared with the previous implementation,
where each queue created its own executor.
With the current implementation:
{{Q1 ----\
> shared executor
Q2 ----/}}
disposing {{Q1}} may therefore shut down the executor that {{Q2}} is still
using.
It may also introduce head-of-line blocking between otherwise independent
element event queues.
It would be useful to confirm whether this sharing is intentional. If not, the
pool should probably be instance-scoped, or its lifecycle should be
reference-counted.
----
h3. 3. Race between {{addElementEvent()}} and {{dispose()}}
There also seems to be a check-then-act race:
{{if (!destroyed.get())
\{
queueProcessor.execute(...);
}}}
can race with:
{{destroyed.compareAndSet(false, true);
disposeExecutorService(...);}}
Possible interleaving:
{{T1: destroyed == false
T2: destroyed = true
T2: executor shutdown
T1: executor.execute(...)}}
which may result in a {{{}RejectedExecutionException{}}}.
The {{AtomicBoolean}} guarantees visibility, but it does not make the
{{destroyed}} check and {{execute()}} atomic with respect to disposal.
----
h3. 4. Similar ownership issue may exist for {{CacheEventQueue}}
{{CacheEventQueue}} now obtains the executor from {{ThreadPoolManager}} using a
name based on the cache name.
If multiple queue instances can exist for the same cache name, they may now
share the same executor.
That would mean that destroying one queue could potentially shut down the
executor of another queue associated with the same cache.
If queues are listener-specific, including the listener identity in the pool
identity, or otherwise keeping the executor instance-scoped, may be safer.
----
h3. 5. Shared {{PooledCacheEventQueue}} ownership
{{PooledCacheEventQueue}} explicitly supports sharing a pool between multiple
queues, but an individual queue can call:
{{disposeExecutorService(poolName, wait);}}
This means a single consumer can destroy a shared resource.
Conceptually, shared executors probably need either:
* lifecycle ownership by {{ThreadPoolManager}} only, or
* reference counting / acquire-release semantics.
Otherwise the first queue being destroyed may terminate the executor while
other queues still depend on it.
----
h3. 6. Concurrent remove/recreate may allow two executor generations to overlap
{{getExecutorService()}} uses {{{}computeIfAbsent(){}}}, while disposal removes
the executor and subsequently shuts it down.
A concurrent thread can therefore create a replacement executor after the map
entry is removed but while the previous executor is still processing queued
tasks.
For ordered event processing, this could temporarily result in:
{{P1 -> still draining old tasks
P2 -> already processing new tasks}}
which may violate ordering assumptions across the lifecycle transition.
----
There are also two smaller points:
* {{Duration.toSeconds()}} loses sub-second timeout precision; {{toNanos()}}
or {{toMillis()}} would avoid turning e.g. 500 ms into zero.
* {{InterruptedException}} during {{awaitTermination()}} should probably
restore the interrupt status with:
{{Thread.currentThread().interrupt();}}
The JCS-248 fix itself clearly improves the leaked-thread problem, but I think
the *dispose-by-name / executor-generation issue and multi-instance
{{ElementEventQueue}} ownership* are worth testing explicitly.
Two regression tests that may expose the issue would be:
{{1. create Q1 + Q2
2. dispose Q1
3. verify Q2 can still process events}}
and:
{{1. Q1 and Q2 use P1
2. dispose Q2 -> P1 removed/shutdown
3. create Q3 -> P2
4. dispose old Q1
5. verify Q3/P2 is still alive}}
The second one in particular would verify that an old instance cannot
accidentally dispose a newer executor registered under the same pool name.
> 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)
