allthingssecurity commented on PR #29525:
URL: https://github.com/apache/camel/pull/29525#issuecomment-6763728251
Thanks @orpiske, that is a fair concern, and the `VirtualMachineError` part
(`OutOfMemoryError`, `StackOverflowError`, `InternalError`) is the sensitive
one. Some context first, then a proposal.
What happens on main today when a plain `Processor` throws an `Error` that
is not a `LinkageError`: `DelegateSyncProcessor` calls `callback.done(true)` in
`finally`, so the routing engine carries on to the next step as if the
processor had succeeded, and only then does the `Error` escape into the
reactive executor. For an application with strict reliability needs, that is
the risky outcome: after an `OutOfMemoryError` in a processor, the route can
still reach its end and the consumer can commit or acknowledge the message,
although the step never completed. Camel already catches `Throwable` on several
other paths, for the same reason (for example `CamelInternalProcessor` and
`RedeliveryErrorHandler`).
So I would not like to keep "the route continues", but I agree the error
handler should not be invited to handle a JVM-level failure. Proposal:
- `VirtualMachineError` (`OutOfMemoryError`, `StackOverflowError`,
`InternalError`, ...): the exchange fails and the route does not continue, but
the failure is not given to the error handler. No `onException`, `doCatch`,
redelivery or dead letter channel, so nothing in the application "handles" it,
and the consumer does not commit. The `Error` is then thrown on to the calling
thread as today, so the thread's uncaught exception handler and any JVM-level
handling still see it.
- Other `Error`s (`AssertionError`, `LinkageError`,
`ExceptionInInitializerError`, ...): the exchange fails with it and the error
handler sees it, as in this PR. That is what `LinkageError` already did here,
and what tests asserting inside a processor need.
- Docs: an explicit section in the 4.24 upgrade guide and the error handling
page, with a small table of which `Error`s go where and what the consumer then
does. I'll also fold in Claus's two notes on the current paragraph ("unless the
error handler handles it", and `request`/`asyncSend` next to `send`).
Would that address your concern? Or would you rather leave
`VirtualMachineError` exactly as today (thrown on after the callback) and limit
this PR to the other `Error`s? I'll hold the change until you say which you
prefer.
_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]
