allthingssecurity commented on PR #26870:
URL: https://github.com/apache/camel/pull/26870#issuecomment-5855380496
@davsclaus addressed in 2e6598a95 (new commit on top, no force-push).
(1) The leak is real, but I fixed it where it happens instead of resetting
on start. What I checked (DefaultCamelContext, parallel onCompletion, forced
shutdown):
- After the forced `context.stop()`, the old `OnCompletionProcessor` keeps
`getPendingExchangesSize() == 1`: the one queued task that `shutdownNow`
dropped.
- After `context.start()`, `getProcessor("oc")` is a new
`OnCompletionProcessor` instance with 0 pending, because the routes are created
again. So the "every later graceful shutdown waits the full timeout" case did
not happen after a context restart.
- The instance is kept across a forced `stopRoute("r", 1, SECONDS)` +
`startRoute("r")`, but there the pool is not shut down, and the 3 running tasks
later uncount themselves. When I reset the counter by reflection right after
`startRoute` to simulate a reset in `doStart`, it ended at -3 once those tasks
finished. A negative count would let a later graceful shutdown stop waiting too
early.
So `doShutdown` now subtracts the tasks that `shutdownNow` returns (queued
tasks that will never run) from `taskCount`. Running tasks are interrupted and
uncount themselves in their `finally`. The count is then exact after a forced
shutdown, and nothing needs resetting.
New test `OnCompletionParallelProcessingForcedShutdownTest`: a pool of one
thread, one onCompletion running and one queued, and a graceful shutdown that
times out after 1 s. Afterwards no task may be pending, and the queued one must
not have run. Negative control: without the subtraction it fails with
`expected: <0> but was: <1>`; with it, it passes.
WireTapProcessor: I did not change it in this PR. This branch predates
#26851, so its `WireTapProcessor` still counts from `run()`. Queued tasks are
not counted there, and subtracting the dropped tasks would make the count
negative. On `main` (with #26851) the Wire Tap has the same leak and needs the
same subtraction. I can do that in a small follow-up PR on top of `main`, or
rebase this PR onto `main` and add it here. Which do you prefer?
(2) Added a `camel-core` section to the 4.23 upgrade guide. It says that
stopping or suspending a route, or stopping CamelContext, now waits for the
running and queued parallel onCompletion tasks, and that an onCompletion that
synchronously stops its own route now waits until the shutdown timeout. It
suggests stopping the route asynchronously instead (for example Control Bus
`async=true`). I checked the self-stop case: with a 2 s timeout, `stopRoute`
from the parallel onCompletion took 2003 ms and ended with a timeout, and the
route was then Stopped.
Tests: `*OnCompletion*,*Shutdown*,*WireTap*` in camel-core: 153 tests, 0
failures (2 skipped). The branch still merges cleanly with `main` (`git
merge-tree`).
_Claude Code on behalf of allthingssecurity_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
