Claus Ibsen created CAMEL-24980:
-----------------------------------
Summary: camel-core - RedeliveryErrorHandler: remove dead
SimpleTask code and share failure handling between the tasks
Key: CAMEL-24980
URL: https://issues.apache.org/jira/browse/CAMEL-24980
Project: Camel
Issue Type: Improvement
Components: camel-core
Reporter: Claus Ibsen
RedeliveryErrorHandler (core/camel-core-processor, ~2050 lines) is hard to
maintain. Most of the class is two inner task classes, SimpleTask and
RedeliveryTask, that each carry their own copy of the failure handling logic,
and the copies have started to drift apart. This ticket is about cleaning that
up without changing behaviour.
h3. Findings
*Dead code in SimpleTask*
The CAMEL-22907 fix added about 300 lines to SimpleTask.handlePreviousFailure
and its helpers: exception policy lookup, deliverToFailureProcessor,
shouldHandle / shouldContinue, the 4-arg prepareExchangeAfterFailure and
prepareExchangeForContinue. None of it can run. SimpleTask is only chosen when
there are no exception policies and no dead letter channel (see doStart), so
the policy lookup always returns null and the target processor is always null.
With the old 4-line body put back, all 12 *BridgeErrorHandler* tests still
pass, including the two added by CAMEL-22907.
*Duplicated logic that has drifted*
- handleException: SimpleTask checks for a null exception, RedeliveryTask does
not.
- FAILURE_ROUTE_ID is set three different ways: getRoute().getRouteId(),
getAtRouteId() and ExchangeHelper.captureFailureOrigin().
- The two prepareExchangeForContinue methods behave differently. One removes
EXCEPTION_CAUGHT and sets EXCEPTION_HANDLED, the other keeps EXCEPTION_CAUGHT.
- In each task, deliverToFailureProcessor has a "with failure processor" branch
and a "without" branch that both set the failure endpoint and route id, run
onPrepare and build the log message. That is four near-identical blocks in
total.
*Logging*
- logFailedDelivery takes five positional booleans (shouldRedeliver,
newException, handled, continued, isDeadLetterChannel). isDeadLetterChannel is
never used, and isLogRetryAttempted is checked twice.
*Ordering and task release*
- In deliverToFailureProcessor the finally block schedules the callback before
it logs and releases the task. DefaultReactiveExecutor runs the task
immediately when no worker is active on that thread, so when an async failure
processor finishes on another thread, the exchange can carry on routing before
the "Failed delivery" log reads it. Not reproduced yet.
- When run() catches an unexpected exception, or runSynchronousRedelivery is
interrupted, the callback is fired but taskFactory.release is never called.
- RedeliveryTask.reset() does not clear failureProcessor, so a reused pooled
task can send a later exchange to an earlier exchange's onException route. This
only affects the pooled exchange factory, which is planned for deprecation. It
is a one-line fix to fold into this cleanup.
h3. Proposed work (one PR per step)
# Delete the dead SimpleTask code by going back to the pre-CAMEL-22907 body.
Keep the CAMEL-22907 tests as guards.
# Replace the five booleans with a failed-delivery logger driven by an enum
(retry, exhausted, handled, continued, new exception).
# Move the shared failure preparation into one helper (failure endpoint, route
id, onPrepare, original message/body, handled/continued preparation) that both
tasks use. Collapse the two branches of deliverToFailureProcessor into one path.
# Log and release the task before the callback is scheduled, and release the
task on the error paths.
# Optional: turn the decision part of RedeliveryTask.doRun into a small pure
method that can be unit-tested on its own.
h3. Constraints
- This is on every exchange's hot path. Keep hot methods small so the JIT can
inline them (as the earlier "refactor large methods to help inlining" commit
did), and do not add allocations.
- Within Camel, the only subclasses outside the package are
TransactionErrorHandler (camel-spring) and JtaTransactionErrorHandler. They
only call super.process, so the inner tasks can become private.
RedeliveryErrorHandler itself is public API, so keep its protected methods for
third-party subclasses.
- No behaviour change is intended. The existing error handler tests in
camel-core are the safety net.
_Claude Code on behalf of Claus Ibsen_
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
