[jira] [Commented] (CAMEL-24244) camel-base-engine: DefaultStreamCachingStrategy.updateSpool calls lock() instead of unlock() in finally, blocking other threads forever

["Andrea Cosentino (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CAMEL-24244?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18098773#comment-18098773 ] 

Andrea Cosentino commented on CAMEL-24244:
------------------------------------------

Additional impact found while writing the regression test: the leaked lock also deadlocks *CamelContext shutdown*.

{{DefaultStreamCachingStrategy.doStop()}} calls {{statistics.reset()}}, which acquires the same lock:

{noformat}
public void reset() {
    lock.lock();
    try { ... } finally { lock.unlock(); }
}
{noformat}

So once any thread has called {{updateSpool()}} and left the lock permanently held, the thread stopping the context blocks forever in {{reset()}}. The context never finishes stopping.

Verified empirically: a test that spools from two threads against the unfixed code wedged the surefire fork until it hit its 50-minute timeout, rather than simply failing. Note {{ReentrantLock.lock()}} is uninterruptible, so neither {{shutdownNow()}} nor thread interruption can recover the blocked thread.

This broadens the impact from "other threads updating spool statistics block" to "the application cannot shut down cleanly once spool statistics have been updated".

> 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
>            Priority: Major
>
> {{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)