allthingssecurity opened a new pull request, #29200: URL: https://github.com/apache/camel/pull/29200
# Description [CAMEL-26575](https://issues.apache.org/jira/browse/CAMEL-26575) Same family as CAMEL-26445, CAMEL-26446, CAMEL-26461 and CAMEL-26464 (an exception that nothing hands to the callback, so the exchange hangs), here in the completion of a multicast. `MulticastTask.doDone` (and `doTimeoutDone`) sets the done flag first (`done.compareAndSet(false, true)`) and then calls `MulticastProcessor.doDone`, which calls `AggregationStrategy.onCompletion(subExchange, original)` before its last statement, `reactiveExecutor.schedule(callback)`. When `onCompletion` throws: - the callback is never scheduled; - the exception goes to the catch of `aggregateCompleted` (or `timeout`), whose `doFailed` / `doTimeoutFailed` finds the task done already and only logs it at DEBUG; - with `stopOnException` the multicast is done from the callback of the failed sub exchange, and the exception goes up to whatever called that callback (the reactive executor logs and drops it). The exchange hangs: a synchronous caller blocks for good, the exchange stays inflight, and a graceful shutdown waits for it. This happens in every mode (sequential, `parallelProcessing`, `synchronous`, `stopOnException`, `timeout`), for the multicast, the split (also `streaming`, also in a `doTry`, whose `doCatch` never runs) and the recipient list. The same strategy throwing from `aggregate` fails the exchange as expected. ```java AggregationStrategy strategy = new AggregationStrategy() { public Exchange aggregate(Exchange oldExchange, Exchange newExchange) { return newExchange; } public void onCompletion(Exchange exchange, Exchange inputExchange) { throw new IllegalStateException("onCompletion"); } }; from("direct:start").multicast(strategy).to("mock:a", "mock:b").end().to("mock:result"); template.send("direct:start", e -> e.getMessage().setBody("Hello")); // never returns ``` The javadoc of `onCompletion` says it must not throw, so this needs user code that breaks that contract, as the four sibling fixes need an `AsyncProcessor` that throws. None of Camel's own strategies throw there. But such a strategy should fail the exchange, as when `aggregate` throws, not hang it silently (the only trace is a DEBUG log). ## The fix `MulticastProcessor.doDone` catches an exception from `onCompletion`, does the rest of its work and schedules the callback as before. The exchange fails with that exception. It is set after the results are copied, because `ExchangeHelper.copyResults` would overwrite it, and the exchange is not marked as exhausted (no error handler has seen this failure), so the error handler of the route handles it as when the strategy throws from `aggregate`: redelivery, `onException`, the dead letter channel. An exchange that has failed already (such as with `stopOnException`) keeps its exception, which tells where it failed, and gets the new one as suppressed; its exhausted flag is left as it was. `doDone` runs once per exchange (the done flag guards it), so the callback is still scheduled exactly once. Nothing changes on the success path. ## Behaviour changes - The hang: a route whose strategy throws from `onCompletion` now fails with that exception (its error handler, `onException` and a `doCatch` see it) instead of never completing. - One case did not hang: a `synchronous()` split whose expression throws, with a strategy whose `onCompletion` throws as well. On main the exception from `onCompletion` replaced the one of the expression; now the exchange keeps the exception of the expression, with the one from `onCompletion` as suppressed (so a `doCatch` for the expression's exception type catches it). `splitExpressionThrows` pins it. No route that completes successfully changes, so there is no upgrade guide entry. It merges cleanly with the PRs of CAMEL-26445, CAMEL-26446, CAMEL-26461 and CAMEL-26464, which change other files. Backport: camel-4.18.x has the same bug (there the catch of `aggregate` calls `doDone`, which does nothing once the task is done, so not even the DEBUG log). The change merges cleanly there (checked with `git merge-tree`; the tests were not run on 4.18.x, where `timeout()` still uses `tryLock`). Not part of this: the aggregator (`AggregateProcessor.onSubmitCompletion`) also calls `onCompletion` after it counts the group as in progress; by reading only, a throw there could leave the count up, so a stop with `forceCompletionOnStop` would wait. A separate follow-up if confirmed. ## Tests New `MulticastOnCompletionThrowsTest` (camel-core, `processor`, JUnit assertions as the sibling tests). Sends are bounded with `asyncSend(...).get(10, SECONDS)` and there are no sleeps. - multicast, multicast `parallelProcessing`, `synchronous`, `stopOnException`, `timeout` (the second output waits on a latch, so the timeout completes it), split, split `streaming`, recipient list. Each route with the throwing strategy is compared with the same route whose strategy does not throw: it must fail with the exception and complete as many exchanges (an `EventNotifier` count, which also counts the copies), so the exchange continues exactly once. With `stopOnException` the exchange keeps the exception of the output, with the exception from `onCompletion` suppressed; - a split in a `doTry`: the `doCatch` runs once; - the error handler of the route: a split under `onException(...).handled(true)` (the `onException` handles it once) and under a redelivering error handler (redelivered twice, 6 parts sent), with the control of a strategy that throws from `aggregate` (the same, also on main); - `aggregate` and `onCompletion` both throw, and the expression of a split throws: the exchange keeps the first exception, with the one from `onCompletion` as suppressed. On main 13 of the 14 tests fail with a `TimeoutException` (the hang); the control of the strategy that throws from `aggregate` passes on main, as do the routes whose strategy does not throw. With the fix all 14 pass, also 20 times in one JVM (280/280) and with the four sibling fixes merged. Without the exhausted change (the first commit alone) the two error-handler tests fail. `mvn test` on camel-core `org/apache/camel/processor/**`: 2922 tests, 0 failures, 0 errors, 17 skipped (with the formatter and impsort of the build: no change). _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
