allthingssecurity commented on PR #28598:
URL: https://github.com/apache/camel/pull/28598#issuecomment-6464127079
@davsclaus thanks for the review. All points are in 305d56d57e01:
- **Delay (blocker):** a positive value from `beforeSend` now waits with
`TimeSource.sleep` on the thread that sends, for every exchange, and then calls
the producer on that same thread, as a slow endpoint would. Nothing goes to the
`ErrorHandlerRedeliveryTask` pool any more. When the wait is interrupted, the
producer is not called. The send fails with the `InterruptedException` and the
thread keeps its interrupted status. Like any `InterruptedException` on an
exchange, this marks the exchange as interrupted, so Camel stops routing it.
That is the same result as when the synchronous wait of
`RedeliveryErrorHandler` is interrupted. If a use case shows up, an async delay
can be added later as an opt-in; chaos does not need one.
- **Low 1:** without a hook, the recipient list gives a recipient's error
handler the producer itself again, so nothing changes. With a hook, the error
handler that has the wrapped producer is cached under its own key, and the
wrapper implements `ShutdownableService` and passes `shutdown` on. One finding
while I wrote the test: on main, the recipient's producer is not shut down
through that error handler either. `MulticastProcessor` keeps those error
handlers in a `Map`, and `ServiceHelper.stopAndShutdownServices` skips a `Map`.
A counting producer is stopped once and never shut down, on main and on the
branch, both on context stop and on route removal. So instead of a route test,
there are two tests:
- `SendHelperTest#testSendHookProducerPassesTheLifecycleOn`: the wrapper
passes start, stop and shutdown on.
- `SendHookTest#testRecipientListWithoutHookCallsTheProducerItself`:
without a hook, the error handler calls the producer directly; a hook installed
later is used, and after it is removed the producer is called directly again.
If you think those cached error handlers should be stopped, I can open a
JIRA.
- **Low 2:** the `SendHook` javadoc now says it covers the sends that fire
`ExchangeSendingEvent`, and that code which acquires a producer and calls it
itself does not run it. `ExtendedCamelContext` says the same.
- **Nit 3:** I removed the `notify` parameter of `SendHelper.send`.
`EventNotifierProducer` was its only caller in the repo.
- **Nit 4:** other negative values are reserved: the send fails with an
`IllegalArgumentException` and the producer is not called. I chose to reject
them rather than warn, so that giving a value a meaning later cannot change a
hook that works today.
- **Nit 5:** the thread-name test now asserts that the producer runs on the
thread that sent.
I updated the PR description to match: delays, the recipient list and the
test results.
--
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]
