allthingssecurity commented on PR #29665: URL: https://github.com/apache/camel/pull/29665#issuecomment-6819884054
Thanks for the review. Changes are in ecd605e16c76. 1. Agreed: the catch around the await manager in `process` (:354-360) was not reached by any new test. New test `synchronousSplitFailsTheExchangeWithAnErrorOfItsAggregationStrategy`: a synchronous split whose `AggregationStrategy` throws an `AssertionError` in `aggregate`, with `onException(AssertionError.class).handled(true)`. It expects `mock:part` to get "a" only, `mock:handled` 1 and `mock:after` 0. The `Error` leaves `MulticastTask.aggregateCompleted` and the reactive worker (both catch only `Exception`), then the await manager, and lands in that catch. With the catch put back to `catch (Exception e)`, the test fails (`mock:handled` gets 0; the callback has already run, and the `Error` is thrown on). With it, the test passes. Description, upgrade guide entry and `error-handler.adoc` now name this case. It applies only with `synchronous=true` and without `parallelProcessing`. With `parallelProcessing`, `aggregate` runs on a pool thread and the `Error` is lost in its `Future`, so that case stays with the follow-up. On this path `doDone` of the split is still skipped, as before; the follow-up draft (MulticastTask catches) says so too, with the cases left for it: parallel, timeout, the default asynchronous mode, and the transacted leftovers. 2. You read it right. The task goes to `submit(...)`, so a `VirtualMachineError` rethrown from `run()` ends up in a `Future` nobody reads. The `finally` has already called `callback.done(false)` with no exception on the exchange, so the exchange completes as if the call had succeeded, and the `Error` is silently dropped. That was also the case before this PR for every `Error` there. For a VME I kept the behaviour (no exchange failure, thrown on, as in `DelegateSyncProcessor`), but it is now logged at ERROR first, with the operation and the exchange id, so an OOM there is at least visible. Switching to `execute(...)` would make the pool thread die visibly, but it changes the threading of every API component, so I did not do it here. New test `testVirtualMachineErrorCompletesTheExchangeAsBefore` pins the unchanged completion; it does not assert the log line. 3. #29668 merged at 14:30 today, so this PR is the second. `error-handler.adoc` (:188) now also names a transformer or validator of the input or output type of a route (`inputType`, `outputType`) and a plain `Processor` of a transacted exchange in a step without an error handler of its own. The two upgrade-guide sections cannot be folded here without a rebase onto main, because #29668's section is not on this branch, and that would need a force-push. Instead, the stacked follow-up for the `MulticastTask` catches will be opened on main after this one and touches the same guide entry, so it will fold #29668's section and this one into one 4.24 entry. If you would rather have it here, I can rebase and force-push once you say so. 4. Yes. With `-Psanitizer`, `SiblingProcessorsThrowErrorTest` (including the new test) and `AbstractApiProducerTest` pass: 16 run, 0 failures, no violation reported. As you expected, the `StackOverflowError` cases call the processors directly, and on the route paths `process` does not throw and calls the callback once. 5. Done: `assertInstanceOf` at those three places, and the processors started in the test are stopped in a `finally` (`ServiceHelper.stopService`). Tests: the new test fails with the `process` catch reverted and passes with it. camel-core tests around the change (`SiblingProcessorsThrowErrorTest`, `AbstractApiProducerTest`, the multicast, split, recipient list, error, transform and `support/component` tests) ran 698 with 0 failures. The two changed test classes ran 320/320 over 20 runs in one JVM. The description is updated. _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]
