Andrea Cosentino created CAMEL-24286:
----------------------------------------
Summary: camel-support: BackgroundTask supplier exception hangs an
unlimited-duration run() and leaks the task registration
Key: CAMEL-24286
URL: https://issues.apache.org/jira/browse/CAMEL-24286
Project: Camel
Issue Type: Bug
Components: camel-core
Reporter: Andrea Cosentino
Assignee: Andrea Cosentino
Follow-up defect from the ForegroundTask -> BackgroundTask reconnection-loop
migration (CAMEL-24272 / CAMEL-24278, both merged).
h3. Problem
{{BackgroundTask.runTaskWrapper}} rethrows a supplier exception on the
scheduler thread without releasing the completion latch or unregistering the
task:
{code:java}
try {
if (doRun(supplier)) {
...
latch.countDown();
}
} catch (Exception e) { // BackgroundTask.java ~131-134 on main
status = Status.Failed;
cause = e;
throw e; // rethrown onto the ScheduledExecutorService
thread
}
{code}
When the supplier throws a non-{{TaskRunFailureException}}:
* the exception is rethrown onto the {{scheduleWithFixedDelay}} worker, which
silently suppresses all further executions of that task (standard
{{ScheduledExecutorService}} contract);
* {{latch.countDown()}} is never reached, and {{registry.removeTask(this)}} is
never called on this path;
* for a task built with {{withUnlimitedDuration()}}, {{waitForTaskCompletion}}
calls {{latch.await()}} with no timeout, so *the calling thread blocks forever*
and the task *leaks in the TaskManagerRegistry*.
{{ReentrantLock}} is not involved here, but note {{latch.await()}} in the
unlimited case is likewise unrecoverable once the only countdown site is
skipped.
The interrupt path has a related smaller gap: in {{waitForTaskCompletion}},
{{registry.removeTask(this)}} sits inside the {{try}} (~line 205), while the
{{finally}} only resets {{elapsed}}/{{running}}. An {{InterruptedException}}
from {{latch.await()}} therefore also leaves the task registered.
h3. Reachability on main (post CAMEL-24272)
Both of these now route an *unlimited-duration* reconnection through
{{BackgroundTask.run()}}:
* *camel-ftp SFTP* — {{SftpOperations.java:142}} uses
{{withUnlimitedDuration()}}; {{tryConnect}} catches only {{JSchException}}
({{:209}}, {{:221}}). A non-JSch {{RuntimeException}} on the connect path (e.g.
an invalid {{filenameEncoding}} reaching {{Charset.forName}}) escapes into the
throw path above -> hang + registry leak.
* *camel-mongodb-gridfs* — {{GridFsConsumer.java:122}}
{{withUnlimitedDuration()}}, {{:131}} {{task.run(...)}}. In
{{processCollection}} the inner {{catch (Exception) { // ignore }}} ({{:184}})
wraps *only* {{getProcessor().process(exchange)}}; the cursor acquisition
{{getGridFSFileMongoCursor(...)}} ({{:143}}) and the pre-process
{{findOneAndUpdate(...)}} ({{:155}}) are *outside* any catch. A transient
{{MongoException}} there escapes -> same hang + leak.
Bounded tasks ({{withMaxDuration}}, e.g. Infinispan) do not hang because
{{latch.await(maxDuration)}} times out -- but they still fail slow and lose the
real cause. Suppliers that catch-all and return {{false}} (e.g. FTP
{{tryConnect}}) are unaffected.
h3. Suggested fix
Treat a thrown supplier exception as a terminal failure that still releases the
caller:
* in the {{catch}} block, set {{completed.set(false)}}, unregister when
{{!registeredByRun}}, and call {{latch.countDown()}} (do not rely on rethrowing
onto the scheduler thread, which serves no purpose there);
* move {{registry.removeTask(this)}} in {{waitForTaskCompletion}} into the
{{finally}} so the interrupt path also unregisters.
A regression test can build an unlimited-duration {{BackgroundTask}} whose
supplier throws on the first run and assert (with a bounded await, daemon
thread) that {{run()}} returns and the registry ends empty -- mirroring the
shape used in CAMEL-24244's {{StreamCachingStrategySpoolStatisticsTest}}.
h3. Notes
Found while reviewing the task-manager PR cluster (#25164 merged, #25169
closed). Not attributable to a single PR -- it is the residual
foreground->background porting gap now reachable on main. Raising for the
owners of CAMEL-2427x to assess, since some of the rethrow behaviour may be
intentional for programming errors; the latch-release/unregister omission is
the part that looks unintended.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
