allthingssecurity opened a new pull request, #29786: URL: https://github.com/apache/camel/pull/29786
# Description [CAMEL-26882](https://issues.apache.org/jira/browse/CAMEL-26882) Follow-up of #29127 (CAMEL-26464): its description named this double completion in `DefaultProducerCache` as a follow-up, and the review asked for it to be filed. The same rule as CAMEL-26445, CAMEL-26461 and CAMEL-26464 (the callback of an exchange is called exactly once, also when a processor breaks the async contract). `DefaultProducerCache.doInAsyncProducer` (used by `toD`, the routing slip, `to` for a producer that is not optimised, and the async sends of the producer template through `asyncDispatchExchange`) catches an exception thrown by the producer, sets it on the exchange and calls the callback. When the producer had already called the callback before it threw (an `AsyncProcessor` that breaks the contract), the exception was set on an exchange that had already been continued (maybe on another thread), so a send that succeeded failed the exchange (with `toD` the steps after it did not run), and the callback was called a second time. The error handlers ignore a second call since CAMEL-26445, CAMEL-26461 and CAMEL-26464, but the async sends of the producer template released the producer to the pool a second time, so a producer of an endpoint that is not a singleton producer could be handed to two sends at once. When the producer threw before it called the callback, the producer was not relea sed to the pool (only the callback released it, and outside the async sends of the producer template, where `asyncDispatchExchange` called it, nothing called it), so each such send created a new producer. Change, in `DefaultProducerCache`: - the callback of `doInAsyncProducer` is a small class (`SentCallback`, an `AtomicBoolean`, one object per send as the lambda it replaces) that completes the send once: the sent event, the release of the producer, the callback of the caller; a callback of the producer after that is logged at WARN and ignored; - a throw (any `Throwable`, as the prior art) after the callback is logged at WARN and left there (the error handlers of the prior art rethrow it to their caller; here a caller such as `doAsyncSendExchange` would set it on the exchange that already completed): the exchange is not completed again and the exception is not set on it; `doInAsyncProducer` returns the `doneSync` the producer passed (or `false` when the callback ran on another thread). The takeover is inline, not scheduled as in CAMEL-26464: when the send runs nested in a worker of the reactive executor and the callback was only queued, a throw is taken as a throw before the callback; it still completes exactly once; - a throw before the callback completes the send as the callback would (sent event, release, callback) with the exception on the exchange; that includes a `VirtualMachineError`, as in the error handlers of CAMEL-26445 and CAMEL-26461 (the sync adapters of CAMEL-26741 and CAMEL-26866 instead rethrow it after the callback); - `asyncDispatchExchange` no longer completes on its own: its exception goes to `doInAsyncProducer`, its only caller in Camel, which knows whether the callback was called (a subclass that calls this protected method directly now gets the exception thrown); it pairs its sending event with a sent event when the producer throws before the callback (`EventNotifierCallback` emits its sent event once). The cost per send is one compare-and-set, plus a small flag object in `EventNotifierCallback` on the async template path when event notification applies. Tests, new `DefaultProducerCacheProducerThrowsTest` (an `AsyncProducer` of an endpoint that is not a singleton producer, which calls the callback and then throws, or throws before): - `testCallbackThenThrowCompletesOnce`: `doInAsyncProducer` calls the callback once and leaves the exchange without the exception; - `testThrowBeforeCallbackFailsAndReleasesProducer`: two sends fail with the exception, the callback is called once each, and one producer serves both (it is back in the pool); - `testErrorBeforeCallbackFailsAndReleasesProducer`: the same with a `NoClassDefFoundError` (on main an `Error` escaped both catches, the producer was not released); - `testAsyncSendCallbackThenThrowKeepsTheResult`: `asyncSendExchange` keeps the result of the producer; - `testAsyncSendCallbackThenThrowReleasesTheProducerOnce`: after such a template send, two producers acquired at the same time are two instances; - `testAsyncSendThrowBeforeCallbackPairsTheEvents`: a control, every sending event has its sent event; - `testToDCallbackThenThrowRoutesOnce`: with `toD`, the step after it gets the message, once, and the exchange has no exception. Without the change the six tests other than the control fail on main (the callback called twice, the exception set on the completed exchange, the producer in the pool twice, two producers created, the `Error` escaping, the step after `toD` not run). With it the camel-core tests pass (9187 run, 0 failures, 0 errors, 45 skipped); after the last review changes (an `Error` test, a javadoc line) the producer cache, template, `toD`, routing slip and event notifier tests pass (276 run, 0 failures, 4 skipped). Not changed, for a follow-up JIRA I will offer: `SendProcessor.sendUsingProducer` (the optimised `to`) and the sync `DefaultProducerCache.send` also set an exception thrown after the callback on the exchange. With a send hook installed, `doInAsyncProducer` calls the producer through `SendHelper.HookedSend`, which sets such an exception on the exchange (and calls `afterSend` a second time) before it rethrows it; so with a hook this change still completes the send once and releases the producer once, but the exception stays on the exchange. The async template path emits a sending/sent pair from both `doInAsyncProducer` and `EventNotifierCallback`, as on main; when the producer throws before the callback, the sent event of `EventNotifierCallback` now comes before `doInAsyncProducer` sets the exception on the exchange (its own sent event comes after), as only `doInAsyncProducer` knows whether the callback ran. That duplicate pair is for the same follow-up. No documentation or upgrade guide change: only producers that break the async contract are affected. # 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) filed for the change (usually before you start working on it): [CAMEL-26882](https://issues.apache.org/jira/browse/CAMEL-26882). # 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 with the formatter and import-sort plugins and ran the camel-core tests. 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]
