gnodet-bot commented on code in PR #28756:
URL: https://github.com/apache/camel/pull/28756#discussion_r4429652114
##########
core/camel-core-processor/src/main/java/org/apache/camel/processor/FinallyProcessor.java:
##########
@@ -60,8 +61,24 @@ public boolean process(final Exchange exchange, final
AsyncCallback callback) {
}
}
+ // a route called from the doTry (such as with direct) can have
handled a failure with its error handler (dead
+ // letter channel or a handled onException), which marks the exchange
as handled by the error handler and as
+ // failure handled. The route then does not continue after the doTry,
but the doFinally block must still run all
+ // its steps (a pipeline stops before its second step when the
exchange is handled by the error handler), and a
+ // failure in a route that the block calls must be handled by that
route's error handler as usual (which does not
+ // handle a new failure of an exchange that is failure handled). So
the marks are cleared while the block runs,
+ // and set again afterwards (as the onCompletion does)
+ final ExchangeExtension ext = exchange.getExchangeExtension();
+ final boolean errorHandlerHandled = ext.isErrorHandlerHandledSet() &&
ext.isErrorHandlerHandled();
Review Comment:
`OnCompletionProcessor` captures this as `Boolean errorhandlerHandled =
ext.getErrorHandlerHandled()` (nullable, preserving null/false/true). Here you
use a derived primitive: true only when both set AND true. That's correct for
`doFinally` — you only need to act when the error handler genuinely handled the
failure — but it's a deliberate departure worth a one-liner comment so future
readers don't wonder whether the distinction was overlooked.
##########
core/camel-core-processor/src/main/java/org/apache/camel/processor/FinallyProcessor.java:
##########
@@ -114,16 +131,32 @@ private static final class FinallyAsyncCallback
implements AsyncCallback {
private final Exchange exchange;
private final AsyncCallback callback;
private final Exception exception;
+ private final boolean errorHandlerHandled;
+ private final boolean failureHandled;
- FinallyAsyncCallback(Exchange exchange, AsyncCallback callback,
Exception exception) {
+ FinallyAsyncCallback(Exchange exchange, AsyncCallback callback,
Exception exception,
+ boolean errorHandlerHandled, boolean
failureHandled) {
this.exchange = exchange;
this.callback = callback;
this.exception = exception;
+ this.errorHandlerHandled = errorHandlerHandled;
+ this.failureHandled = failureHandled;
}
@Override
public void done(boolean doneSync) {
try {
+ if (errorHandlerHandled) {
+ // the doFinally block has run, so the route does not
continue after the doTry, as before
+ // (unless the block marked it itself, such as a route it
called that failed)
+ final ExchangeExtension ext =
exchange.getExchangeExtension();
+ if (!ext.isErrorHandlerHandledSet()) {
+ ext.setErrorHandlerHandled(true);
+ }
+ if (failureHandled) {
+ ext.setFailureHandled(true);
+ }
Review Comment:
`OnCompletionProcessor` always restores `failureHandled` to its original
value (can clear it to false) in its `else` branch. Here, it's one-directional:
`setFailureHandled(true)` is only called when the original was true; if the
`doFinally` block itself sets `failureHandled=true` on a fresh exchange
(original = false), that state leaks out. In practice
`errorHandlerHandled=true` with `failureHandled=false` doesn't arise in Camel's
error handling flow, so this is safe — but a short comment explaining the
intentional asymmetry would help future maintainers.
--
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]
