allthingssecurity commented on code in PR #26938:
URL: https://github.com/apache/camel/pull/26938#discussion_r4129056286
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultErrorRegistry.java:
##########
@@ -191,16 +199,18 @@ private void capture(Exchange exchange, boolean handled) {
endpointUri, toNode, stepId, fromEndpointUri, routeUptime,
elapsed,
threadName, data, exception, handled, messageHistory);
- // deduplicate by exchange ID:
+ // deduplicate the same failure of an exchange (the same exception, or
one that wraps the other), as it is
+ // reported by both a correlated copy and the original exchange. Other
failures of the same exchange (the
+ // failures of the parts of a split, a failure after a doCatch, a
failure in onCompletion) are kept.
// - correlated copy (inner): has more specific node info (e.g.,
throwException inside circuit breaker),
- // so it replaces any existing entry for the same original exchange
+ // so it replaces an existing entry of the same failure
// - original exchange (outer): if already captured from a correlated
copy, skip it
// since the copy has more specific info about where the error
actually occurred
if (correlationId != null) {
- entries.removeIf(e -> exchangeId.equals(e.getExchangeId()));
+ entries.removeIf(e -> exchangeId.equals(e.getExchangeId()) &&
isSameFailure(e.getException(), exception));
} else {
for (BacklogErrorEventMessage e : entries) {
- if (exchangeId.equals(e.getExchangeId())) {
+ if (exchangeId.equals(e.getExchangeId()) &&
isSameFailure(e.getException(), exception)) {
Review Comment:
This "same failure" check also matches a `doCatch` that rethrows a new
exception wrapping the caught one, which is a common pattern. The catch's
`ExchangeFailureHandledEvent` records the caught exception (not handled,
correct with fix 1). Then the `ExchangeFailedEvent` with the new exception
returns here, because the existing entry is its cause. On this branch:
```java
.doTry().throwException(new IllegalArgumentException("inner")).id("t1")
.doCatch(IllegalArgumentException.class)
.process(e -> { throw new IllegalStateException("wrapped",
e.getProperty(Exchange.EXCEPTION_CAUGHT, Exception.class)); })
```
The caller gets `IllegalStateException: wrapped`, but the registry has a
single entry: `IllegalArgumentException: inner`, node `t1`, not handled.
Without the cause (as in `testDoCatchThatThrowsAgainIsNotRecordedAsHandled`)
both are recorded. The skip is meant for an original exchange reporting what a
correlated copy already recorded. Could it apply only when the existing entry
came from a correlated copy, for example with a flag set on the entry in the
`correlationId != null` branch?
_Review by 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]
