Andrea Cosentino created CAMEL-24244:
----------------------------------------
Summary: camel-base-engine:
DefaultStreamCachingStrategy.updateSpool calls lock() instead of unlock() in
finally, blocking other threads forever
Key: CAMEL-24244
URL: https://issues.apache.org/jira/browse/CAMEL-24244
Project: Camel
Issue Type: Bug
Components: camel-core
Reporter: Andrea Cosentino
Assignee: Andrea Cosentino
{{DefaultStreamCachingStrategy.UtilizationStatistics.updateSpool(long)}} calls
{{lock.lock()}} in its {{finally}} block instead of {{lock.unlock()}}, so the
lock is never released.
h3. Code
{{core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultStreamCachingStrategy.java}}:
{noformat}
private final Lock lock = new ReentrantLock(); // line 563
void updateMemory(long size) { // line 572 -- correct
lock.lock();
try {
memoryAverageSize.set(memorySize.addAndGet(size) /
memoryCounter.incrementAndGet());
} finally {
lock.unlock();
}
}
void updateSpool(long size) { // line 581 -- BUG
lock.lock();
try {
spoolAverageSize.set(spoolSize.addAndGet(size) /
spoolCounter.incrementAndGet());
} finally {
lock.lock(); // line 586 -- should be
unlock()
}
}
{noformat}
The sibling {{updateMemory}} immediately above is correct, which makes this
look like a copy-paste slip.
h3. Impact
{{lock}} is a {{ReentrantLock}}, so the calling thread does not deadlock
against itself -- it simply leaves the hold count at 2 and never releases it.
The consequences:
* The hold count grows without bound across invocations on the same thread.
* **Any other thread entering {{updateSpool()}} blocks forever.** With more
than one routing thread performing stream caching, this is a permanent hang of
those threads.
h3. Reachability
{{updateSpool}} is called from {{computeStatistics}} (line 356), which runs
from {{doCache}} (line 316) under:
{noformat}
if (statistics.isStatisticsEnabled()) {
computeStatistics(cache);
}
{noformat}
So the bug is reachable whenever stream-caching statistics are enabled *and* a
stream is spooled to disk (rather than kept in memory). Memory-only caching
takes the {{updateMemory}} path and is unaffected.
h3. Suggested fix
Change line 586 to {{lock.unlock();}}.
A regression test would need two threads calling {{updateSpool}} concurrently
with a timeout, asserting both complete -- the current single-threaded tests
cannot catch it because reentrancy hides the bug from the first thread.
h3. Notes
Found while reviewing PR #24983 / #24985 (CAMEL-24227), which touches this file
for an unrelated {{volatile}} change. Not introduced by that PR -- it is
pre-existing on {{main}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
