gnodet-bot commented on code in PR #26812: URL: https://github.com/apache/camel/pull/26812#discussion_r4087174151
########## core/camel-core/src/test/java/org/apache/camel/processor/onexception/OnExceptionChangedExceptionOnRedeliveryTest.java: ########## @@ -0,0 +1,141 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.processor.onexception; + +import java.io.IOException; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.camel.CamelExecutionException; +import org.apache.camel.ContextTestSupport; +import org.apache.camel.Exchange; +import org.apache.camel.builder.RouteBuilder; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * When the exception changes between redelivery attempts, the exception policy (onException) for the current exception + * is used, and not the one matched by a previous attempt (CAMEL-24981). + */ +public class OnExceptionChangedExceptionOnRedeliveryTest extends ContextTestSupport { + + private final AtomicInteger attempts = new AtomicInteger(); + + @Override + @BeforeEach + public void setUp() throws Exception { + attempts.set(0); + super.setUp(); + } + + @Test + public void testNoPolicyForNewExceptionGoesToDeadLetter() throws Exception { + getMockEndpoint("mock:io").expectedMessageCount(0); + getMockEndpoint("mock:iae").expectedMessageCount(0); + getMockEndpoint("mock:dead").expectedMessageCount(1); + getMockEndpoint("mock:dead").message(0).exchangeProperty(Exchange.EXCEPTION_CAUGHT) + .isInstanceOf(IllegalStateException.class); + + template.sendBody("direct:dlc", "Hello"); + + assertMockEndpointsSatisfied(); + assertEquals(2, attempts.get()); + } + + @Test + public void testNoPolicyForNewExceptionIsNotHandled() throws Exception { + getMockEndpoint("mock:io").expectedMessageCount(0); + + CamelExecutionException e = assertThrows(CamelExecutionException.class, + () -> template.sendBody("direct:default", "Hello")); + assertInstanceOf(IllegalStateException.class, e.getCause()); + + assertMockEndpointsSatisfied(); + assertEquals(2, attempts.get()); + } + + @Test + public void testPolicyForNewExceptionIsUsed() throws Exception { + getMockEndpoint("mock:io").expectedMessageCount(0); + getMockEndpoint("mock:iae").expectedMessageCount(1); + getMockEndpoint("mock:dead").expectedMessageCount(0); + + template.sendBody("direct:iae", "Hello"); + + assertMockEndpointsSatisfied(); + // 1 attempt with IOException, then 2 more as the IllegalArgumentException policy allows 2 redeliveries + assertEquals(3, attempts.get()); Review Comment: ⚠️ **Misleading comment — the IAE policy only gets 1 effective retry, not 2.** The `redeliveryCounter` is not reset when the exception changes. After the IOException attempt, the counter is already at 1. When IAE fires, the counter increments to 2 and `shouldRedeliver(2 ≤ 2)` passes — giving one more try. Then counter = 3, `shouldRedeliver(3 ≤ 2)` fails, exhausted. So IAE effectively gets **1 additional delivery attempt** against its 2-redelivery allowance, not 2. The `assertEquals(3, attempts.get())` assertion is correct, but the comment misrepresents why. ```suggestion // 1 attempt with IOException (counter → 1), then 1 more with IllegalArgumentException // (counter → 2, which is ≤ maxRedeliveries(2), then counter → 3 > 2 → exhausted). // The redelivery counter is not reset when the exception type changes. assertEquals(3, attempts.get()); ``` ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc: ########## @@ -73,6 +73,17 @@ Prior to Camel 4.23 the property was only set when there was no fallback and was so a fallback that tested it for `null` must now test for `true` or `false` instead. `CamelCircuitBreakerResponseShortCircuited` is unchanged and remains `true` whenever the fallback runs, whatever the cause. +=== Error handler - onException when the exception changes during redelivery + +When a redelivery attempt fails with a different exception than the previous attempt, the error handler now uses +the `onException` that matches the new exception. If no `onException` matches it, the error handler's own settings +apply, for example moving the message to the dead letter channel. + +Prior to Camel 4.23 the error handler kept using the `onException` matched by the earlier exception, including its +`handled`, `continued`, redelivery and `onRedelivery` settings. So a new exception with no `onException` of its own +could be routed and handled by the earlier exception's `onException`, and was not seen by the caller or the dead +letter channel. Review Comment: 📝 **Missing: the redelivery counter carries over when the exception changes.** The upgrade guide correctly describes the policy-matching change, but omits a related behavioral detail: when the exception changes mid-redelivery, the accumulated `redeliveryCounter` is **not** reset. The new exception policy's `maximumRedeliveries` applies against that cumulative counter. Practical impact: a route with `onException(IOException.class).maximumRedeliveries(5)` that fails 4 times before the exception changes to `IllegalStateException` with `maximumRedeliveries(3)` will have the new policy immediately exhausted (counter = 5 > 3) on the next delivery attempt. Users relying on per-exception retry budgets after the fix should be aware of this. Suggest adding a paragraph: ``` The redelivery counter is not reset when the exception type changes. The new exception policy's `maximumRedeliveries` limit applies against the cumulative attempt count across all exception types in the same delivery sequence. ``` -- 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]
