allthingssecurity opened a new pull request, #29127: URL: https://github.com/apache/camel/pull/29127
# Description [CAMEL-26464](https://issues.apache.org/jira/browse/CAMEL-26464) Follow-up of CAMEL-26430 (#28785), CAMEL-26445 (`SimpleTask`), CAMEL-26446 (the first delivery of `RedeliveryTask`) and CAMEL-26461 (`NoErrorHandler`): the same hang in the steps that have no error handler at all. The steps of a `doTry`, `doCatch`, `doFinally` and `onException` block have no error handler, so their channel (`DefaultChannel`, a `CamelInternalProcessor`) calls the processor of the step directly. When that processor is an `AsyncProcessor` that throws from `process(exchange, callback)` instead of setting the exception on the exchange, the exception goes up to the reactive work that runs the step (the task of `Pipeline`, or `TryProcessor`), `DefaultReactiveExecutor` logs and drops it (`Error executing reactive work due to ... This exception is ignored.`), and the callback is never called: the exchange hangs, and the `doCatch` never runs. The same processor setting the exception on the exchange is caught by the `doCatch`; a plain `Processor` that throws is caught too (its converter catches it). ```java from("direct:start") .doTry() .process(asyncProcessorThatThrows) // throws instead of exchange.setException(...) + callback.done(true) .to("mock:result") .doCatch(IllegalStateException.class) .to("mock:caught") .end(); template.send("direct:start", e -> e.getMessage().setBody("Hello")); // never returns ``` ## The fix `CamelInternalProcessor.processNonTransacted` catches an exception thrown by the processor when the channel has no error handler (`catchProcessorThrows`, set by `DefaultChannel.doStart` when `errorHandler == null`, next to where it picks the processor). The rule is the one of the four earlier fixes: - thrown before the callback was called: the exchange is failed with it through the reactive executor, after the work already queued there (so a callback that the processor handed to queued work still comes first): the exception is set on the exchange, the after advices run, then the unit of work's after processing (in the order the callback chain runs them, such as clearing the MDC), and the original callback is scheduled. `process` returns `false`; - thrown after the callback was called: the callback continued the exchange, so the exception is rethrown to the caller, as before (as CAMEL-26430, CAMEL-26445 and CAMEL-26461 do; CAMEL-26446 logs it, because there the catch would complete the exchange a second time). That caller is the reactive executor, which logs it, or a caller that calls the step through the synchronous API, such as a circuit breaker, which then fails; - exactly once: the after task (`AsyncAfterTask`, already the callback object of every step, pooled with pooled exchanges) gets one `volatile int awaited`: `n` while the callback of call `n` is still to come, `-n` once that call is settled. The callback and the takeover each `compareAndSet` it from `n` to `-n`, and the loser logs one WARN and returns. After a takeover the task is not released to the pool, so a late callback finds no call awaited instead of a task that another exchange reuses. A second call of the callback is ignored the same way. Transacted exchanges are not changed: `processTransacted` calls the step synchronously and already sets a thrown exception on the exchange. ## Cost on the hot path The channel runs every step of every route, so: - nothing is allocated per step on the success path, and nothing grows: the call number is one `int` field of the existing after task, which fits in its padding (the task stays at 32 bytes, measured with the field offsets on JDK 21 with the default layout; with `-XX:+UseCompactObjectHeaders` it would grow from 24 to 32 bytes; without pooled exchanges the task is created per step as before, with them it is reused); the flag is a field of the channel; - a step whose channel has the error handler (nearly every step) pays a read of a plain `boolean` field of the channel in `prepare`, `done` and before the call; its try/catch has no cost unless something is thrown. When CAMEL-26445, CAMEL-26446 and CAMEL-26461 are merged, such a step pays its `compareAndSet` in its error handler (CAMEL-26446 on the first delivery of a redelivering error handler), and this change adds none; - a channel without an error handler pays one ordered write (`lazySet`) in `prepare` and one uncontended `compareAndSet` in `done`, on a field the same thread has just written. These are the steps of the blocks above and the channels of those EIPs themselves (`doTry`, `doCatch`, `doFinally`, `onException`, `circuitBreaker`), the `multicast` step (without `shareUnitOfWork`) and each output of a multicast, the steps of a circuit breaker, and steps with `inheritErrorHandler(false)`. The multicast wraps each output channel in a clone of the route's error handler, so once CAMEL-26445 is merged such an output pays two `compareAndSet`s, one in each. A rough probe (a route of 20 empty `process` steps inside a `doTry`, and one of 20 such steps with the default error handler; 100,000 exchanges per round, the last two of 6 rounds, three JVMs each, main and the branch alternating; the load of the machine was 19 to 22 from other work, so the numbers are noisy): the `doTry` route took 1329 to 1354 ns per exchange on main and 1350 to 1387 ns on the branch, and the route with the error handler 1777 to 1826 ns and 1802 to 1853 ns: both about 30 to 35 ns more on average, which at this load is within the noise (the route with the error handler only reads a boolean). The probe cannot resolve a per-step cost of the order of 1 to 2 ns. Where it also applies: the outputs of a multicast, around whose channel the multicast clones the route's error handler. A throwing output hung on main, or, when the route's error handler redelivers, failed the exchange at once (the clone of the error handler on main does not handle a throw on the first attempt); it now fails as when it sets the exception, and is redelivered as any other failure (the next output still runs). Steps nested in another EIP inside these blocks (such as a split inside a `doTry`) are covered too. Why here and not in `Pipeline` or `TryProcessor`: a block with one step has no `Pipeline` (`Pipeline.newInstance` returns the step itself), and the steps run from several callers (`Pipeline`, `TryProcessor`, `CatchProcessor`, `FinallyProcessor`, the failure processor of `RedeliveryErrorHandler`); the channel is the one place every step goes through, and it already has a callback object per step. Not part of this change: in `DefaultProducerCache`, `asyncDispatchExchange` and the catch in `doInAsyncProducer` call `callback.done(true)` also when the producer had already called the callback before it threw, so such an exchange is completed twice (found in review; a follow-up JIRA, CAMEL-ZZZZZ, covers it). ## Behaviour changes - The hang: no route can depend on it. - A processor that hands its callback to another thread and then throws: the exchange now fails with the thrown exception right after the work already queued on the reactive executor (the `doCatch` catches it), and the callback, when it comes later, is ignored with a WARN. Before, the exception was dropped and that callback continued the exchange as a success (a route that completed on main now takes its `doCatch`). The step cannot tell such a callback from one that never comes. The same as in the four earlier fixes; it needs a processor that breaks the `AsyncProcessor` contract. - A callback called twice in such a step: the second call is ignored with a WARN (before, it continued the exchange a second time). With pooled exchanges, a second call that comes after the task was reused by another exchange cannot be told apart (the same limitation as the `SimpleTask` fix). - A multicast output with a redelivering error handler that throws: now redelivered (before, it failed the exchange at once). Upgrade guide 4.24: "camel-core - an AsyncProcessor that throws in a step without an error handler fails the exchange", only for what users notice (a handed-off callback plus a throw now takes the `doCatch`, a second callback is ignored with a WARN, the multicast output is redelivered); there is no option for the old behaviour. Backport: `CamelInternalProcessor` and `DefaultChannel` differ on camel-4.18.x (the step hook is not there), so a backport needs a small manual port; it belongs with backports of the four earlier fixes if those are done. It merges cleanly in any order with the CAMEL-26445, CAMEL-26446 and CAMEL-26461 PRs (no common files except the upgrade guide, in a different place). ## Tests - Rebased on main, where CAMEL-26445, CAMEL-26446 and CAMEL-26461 are now merged: camel-core with the error-handler, `doTry`, `onException`, multicast and circuit breaker tests (`*ProcessorThrows*`, `*DoTry*`, `*NoErrorHandler*`, `*OnException*`, `Multicast*`, `*CircuitBreaker*`): 596 tests, 0 failures, 8 skipped, including the 16 of `NoErrorHandlerProcessorThrowsTest` (CAMEL-26461) and the 40 of this change. New `StepWithoutErrorHandlerProcessorThrowsTest` (camel-core, `processor`, JUnit assertions as the sibling tests, 20 tests; sends that hang on main are bounded with `asyncSend(...).get(10, SECONDS)`, no sleeps), and `StepWithoutErrorHandlerProcessorThrowsPooledTest` (the same with pooled exchanges, so the after tasks are reused): - `doTry`, `doTry` with only a `doFinally`, a step of `doCatch`, a step of `doFinally`, `onException` with and without `handled(true)`, an output of a multicast, and an output of a multicast in a route that redelivers: the route with the throwing processor ends exactly as the same route with a processor that sets the exception on the exchange (the exception, the caught exception, the mocks reached, the calls), with as many exchanges done (`EventNotifier` count, which also counts the copies of the multicast); - after an asynchronous step (`delay(10).asyncDelayed()`), and with MDC logging (the unit of work wraps the callback); - the next exchange after a takeover is processed; - the late callback from another thread is ignored and the exchange is done once; it does not continue the next exchange either; - the callback called twice: done once; - the callback and the throw at the same time (500 exchanges, each callback called at once on a pool of two threads): each exchange done once, either completed or caught; - controls (pass on main too): a transacted exchange, a plain `Processor` that throws, the callback called before the throw, the same through the synchronous API (the step's channel called with `process(exchange)`, as a circuit breaker does: the exception reaches the caller), the callback queued on the reactive executor before the throw. Without the fix (main's `CamelInternalProcessor` and `DefaultChannel` compiled over the branch), 14 of the 20 fail: 12 time out, the double callback completes the exchange twice, and the redelivered multicast output fails the exchange at once. The 5 controls and the race test pass on main too. Mutants (each fails at least one test): the takeover run inside the catch instead of through the reactive executor fails the control with a queued callback; no `compareAndSet` in the callback fails 5 (the stress test, the double callback, and the three controls where the callback comes first); taking over also when the callback came first, instead of rethrowing, fails the synchronous-caller control; releasing the after task to the pool after a takeover fails `callbackCalledAfterTheProcessorThrewDoesNotContinueTheNextExchange` (and the stress test) with pooled exchanges. - Both new classes 20 times in one JVM: 800 tests, 0 failures. - camel-core `org.apache.camel.processor.**` (with the new classes): 2948 tests, 0 failures, 17 skipped. - camel-core `issues.**` and `impl.**`: 1224 tests, 7 failures, all in `RouteChecksumTest`; `component.**`: 1649 tests, 8 failures in `BrowseRemoveRouteTest`, `SedaConfigureTest`, `FilePollEnrichDynamicFileNameTest` and `FileProducerChecksumOfWrittenFileTest`; `model`, `builder`, `support`, `converter`, `language` and the root package: 2451 tests, 11 failures in `SimpleSyntaxHintsTest`, `SimpleTest` (3 hint tests), `RouteWatcherReloadChangedRoutesTest` and `ValidatorExternalResourceTest`. Each of these classes fails the same way with main's `CamelInternalProcessor` and `DefaultChannel` (they test component and bean code that my local build takes from older jars, outside the modules I built). - The tests of the CAMEL-26445, CAMEL-26446 and CAMEL-26461 branches and these, with the four changes merged together: 119 tests, 0 failures. - Formatter and import-sort: no changes. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL-26464) filed for the change (usually before you start working on it): [CAMEL-26464](https://issues.apache.org/jira/browse/CAMEL-26464). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built the core modules and ran the tests above, including the formatter and import-sort plugins. No generated file changes. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _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]
