allthingssecurity opened a new pull request, #29041: URL: https://github.com/apache/camel/pull/29041
# Description [CAMEL-26461](https://issues.apache.org/jira/browse/CAMEL-26461) Follow-up of CAMEL-26430 (#28785), CAMEL-26445 (`SimpleTask`) and CAMEL-26446 (the first delivery of `RedeliveryTask`): the same hang in a route with `noErrorHandler()`, which does not use `RedeliveryErrorHandler`. With `errorHandler(noErrorHandler())`, each step is wrapped in `NoErrorHandler`, whose `process(exchange, callback)` called `output.process(exchange, callback)` with no try/catch. When the 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, `DefaultReactiveExecutor` logs it (`Error executing reactive work due to ... This exception is ignored.`) and drops it, and the callback is never called: the caller waits forever, with or without an asynchronous step before it. A plain `Processor` that throws does not hang (its converter catches it). ```java from("direct:start").errorHandler(noErrorHandler()) .process(new AsyncProcessorSupport() { @Override public boolean process(Exchange exchange, AsyncCallback callback) { throw new IllegalStateException("thrown"); // instead of exchange.setException(...) + callback.done(true) } }) .to("mock:result"); template.send("direct:start", e -> e.getMessage().setBody("Hello")); // never returns ``` ## The fix `NoErrorHandler.process` catches an exception thrown by the processor, with the same exactly-once rule as CAMEL-26430, CAMEL-26445 and CAMEL-26446: - thrown before the callback was called: the exchange is failed with it (exception set, callback called with `false`) through the reactive executor, after the work already queued there, so a callback that the processor handed to queued work still comes first (such as the reply of a `direct` route with the default error handler, whose steps each schedule the next one there; a called route with `noErrorHandler()` loses to the takeover, even with one step, and its reply is then ignored with a WARN); `process` returns `false`; - thrown after the callback was called: the callback continued the exchange, so the exception is rethrown to the caller, as before (the reactive executor logs it; for a transacted exchange, or a step called through the synchronous API, the caller fails the exchange, so a transaction rolls back, as on main). The merged CAMEL-26430 does the same on redelivery, and CAMEL-26445 in the `SimpleTask`. (CAMEL-26446 logs it instead, because there the catch of `RedeliveryTask.run()` would complete the exchange a second time.) - exactly once: `NoErrorHandler` already created a callback object per call (an anonymous class); it is now a small named class (`Continuation`) with a `volatile int` updated with an `AtomicIntegerFieldUpdater`, so nothing more is allocated per step; the cost per step is one uncontended `compareAndSet` when the callback comes (a rough probe, a 20-step route of empty `noErrorHandler()` steps, main and the branch alternating, load around 11: about 5 ns per step, all from the `compareAndSet`). The callback and the takeover each `compareAndSet` it; the loser logs one WARN and returns. A second call of the callback is ignored the same way. Why here and not in `CamelInternalProcessor` or `Pipeline`, where a catch would also cover the steps of `doTry` and `onException` (follow-up CAMEL-26464): the other three fixes are in the error handlers, `NoErrorHandler` already creates a callback per call so the flag costs nothing more, while those layers run every step of every route with pooled tasks. Where it applies: every step of a route with `errorHandler(noErrorHandler())` (also a context-level `noErrorHandler()`, YAML `errorHandler: none` / Pipes, kamelets and rest contract-first routes that use it; no kamelet-specific code), and the producers of a recipient list, which wraps each one in a clone of the route's error handler: with `noErrorHandler()`, a recipient whose processor throws hung there too, and now fails (without `stopOnException`, the next recipient still runs). The catch takes `Exception`, as the other three fixes do. The WARNs name the processor, not the exchange id: the id is created on first use, and on these paths the exchange may be in use on another thread. Not changed (on main as well): an `AsyncProcessor` that throws inside a `doTry` block or an `onException` still hangs (the steps of these blocks have no error handler); follow-up CAMEL-26464. ## Behaviour changes - The hang: no route can depend on it. No upgrade guide entry. - A processor that hands its callback to another thread, or to a `direct` route with `noErrorHandler()`, and then throws: the exchange now fails with the thrown exception right after the work already queued on the reactive executor, and the callback, when it comes later, is ignored with a WARN (before, the exchange waited for that callback, or completed with the route's reply). A route with `noErrorHandler()` cannot tell such a callback from one that never comes; the other thread may still write to the exchange. The same as in the three other fixes; it needs a processor that breaks the `AsyncProcessor` contract. - A callback called twice: the second call is ignored with a WARN (before, it continued the exchange a second time). Backport: `NoErrorHandler.java` is the same on camel-4.22.x and camel-4.18.x, so the production change cherry-picks; it belongs with a backport of CAMEL-26430 if that is done. It merges in any order with the CAMEL-26445 and CAMEL-26446 PRs. ## Tests New `NoErrorHandlerProcessorThrowsTest` (camel-core, `processor.errorhandler`, the tests of the `SimpleTask` fix with `errorHandler(noErrorHandler())` on the route, JUnit assertions, 15 tests; sends that hang on main are bounded with `asyncSend(...).get(10, SECONDS)`, no sleeps): - `noErrorHandler()`, after an asynchronous step (`delay(10).asyncDelayed()`): the exchange fails with the thrown exception; - the next exchange after a takeover is processed; - the late callback from another thread is ignored, the exchange is done once (`EventNotifier` count); 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 (this one passes on main too: there the callback always wins); - a recipient list with a recipient whose producer throws: it fails the exchange at the end of the list and the next recipient still gets the message; with `stopOnException`, the next recipient does not, and the exchange fails with the wrapped exception; - controls (pass on main too): a transacted exchange (it fails with the thrown exception on main as well), a transacted exchange whose processor calls the callback and then throws (fails with that exception, as on main), a plain `Processor` that throws, the callback called before the throw, the callback queued on the reactive executor before the throw, the exchange handed to a `direct` route before the throw. Without the fix (main's `NoErrorHandler` compiled over the branch), 8 of the 15 fail: 7 time out (also both recipient-list tests), and the double callback completes the exchange twice. Mutants: the takeover run inside the catch instead of through the reactive executor fails the 2 controls with a queued callback; no `compareAndSet` in the callback fails 3 (late callback, double callback, stress test); logging a throw after the callback instead of rethrowing it fails the transacted callback-then-throw control. - The new class 20 times in one JVM: 300 tests, 0 failures. - camel-core `processor.errorhandler.**`, `processor.onexception.**` and the doTry/doCatch/doFinally tests, with the new class: 346, 0 failures, 1 skipped. - The 57 camel-core test classes that use `noErrorHandler` (242 tests, in plain JVMs, on the final commit): 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-26461) filed for the change (usually before you start working on it): [CAMEL-26461](https://issues.apache.org/jira/browse/CAMEL-26461). # 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]
