henrik242 opened a new pull request, #26112: URL: https://github.com/apache/camel/pull/26112
Fixes [CAMEL-24626](https://issues.apache.org/jira/browse/CAMEL-24626). Three follow-ups to #26028, listed as known pre-existing issues in that PR's description. None of them was introduced or fixed there. ## 1. A cancelled `BackgroundTask` stays in the `TaskManagerRegistry` (camel-support) A task scheduled through `BackgroundTask.schedule()` adds itself to the `TaskManagerRegistry` from its first run, and only a run of the task removes it again. A caller that cancels the `Future` returned by `schedule()` therefore leaves the entry behind for the life of the `CamelContext`: the task keeps being listed as an internal task, and it keeps its container reachable. `BackgroundTask.cancel(boolean)` now unschedules the task, releases the latch so no queued run does any work, marks the task `Inactive` and removes it from the registry. A run that starts while a cancel is landing undoes its own registration, so the two cannot race a stale entry back in. `schedule()` is used in exactly two places, `camel-sjms` and `camel-master`, and both now cancel through the task. The other thirteen `Tasks.backgroundTask()` users go through the blocking `run()`, which already deregisters on every exit path. ## 2. Lock inversion between the consumer and the cluster view (camel-master) `MasterConsumer` guarded its leadership state with the `BaseService` lock: - `doStop` holds that lock and then needs the write lock of the view, through `CamelClusterView.removeEventListener`. - `AbstractCamelClusterView` dispatches events while holding its own read lock, and the listener of the consumer then needs the `BaseService` lock. The unlocked `isRunAllowed()` fast path added in #26028 covers the common case, but a leadership event that passes that check just before a stop acquires the lock still closes the two orders into a deadlock. The leadership state and the pending start task now have a lock of their own, which `doStop` releases before it touches the view. Nothing that holds it ever waits for the view, so the cycle is gone rather than narrowed. It also decouples leadership handling from the service lifecycle: a leadership event and a start attempt no longer wait for whatever lifecycle operation is in progress. One correction to the description of #26028, which claimed `doStop` waits for the leader pool to terminate while an in-flight attempt waits for the lock. It does not: `ExecutorServiceManager.shutdown()` passes an await timeout of 0, so that path was unnecessary coupling rather than a deadlock. The code comment says what actually happens. ## 3. Exhausted start attempts are not documented (camel-master) `backOffMaxAttempts` defaults to 10 attempts, `backOffDelay` apart (5000 millis). A node that uses up its attempts keeps the leadership and consumes nothing until the leadership changes again. That is the documented intent of the option, and `backOffMaxAttempts=0` already retries for as long as the node is the leader, because the budget builder ignores non-positive values and keeps its unlimited default. Neither the consequence nor the escape hatch was written down, so both are now in the component documentation. ## Testing Two tests in `MasterConsumerLeadershipTest`, both checked against unpatched `main` and failing there: - `testCancellingAPendingStartRemovesTheTaskFromTheRegistry` keeps a start task retrying, loses the leadership, and asserts the task leaves the registry. Before the change it stays. - `testEventDispatchIsNotBlockedByALifecycleOperation` holds the service lock through a suspend that blocks inside the delegate, then asserts a leadership event is still dispatched. Before the change the dispatch waits for the lock, which is the wait that deadlocks against the view. Two tests in `BackgroundTaskTest` cover cancelling a task that is running and cancelling one before its first run: unscheduled, deregistered, `Inactive`, and no further attempt. Green locally: camel-support (119), camel-master (30), the camel-core task tests (34), and `-Psourcecheck` on both changed main modules. The camel-sjms main sources compile, but its tests need an Artemis test-infra artifact that is not installed locally, so CI has to cover that call site. ## Notes for the reviewer - The description of the `backOffMaxAttempts` option itself is deliberately unchanged. It feeds three generated mirrors (the component json, the catalog json and `MasterComponentBuilderFactory`), and the catalog and componentdsl ones cannot be regenerated without a full build, so a hand-edited version would fail the uncommitted-changes check. Worth a small follow-up from someone with a full build, since that description is what tooling and IDE completion show. - Both bugs are present on `camel-4.22.x` as well: the registry leak through camel-sjms predates this work, and the lock inversion came along with the backport of CAMEL-24583. Happy to open a backport PR if you want it there. -- 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]
