CAMEL-24626: camel-master - leadership gets its own lock, and cancelled start tasks leave the task registry - #26112
Conversation
henrik242
changed the title
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
…registers a scheduled task A task adds itself to the TaskManagerRegistry from its first run, and only a run of the task removes it again. Cancelling the future returned by schedule() therefore left the entry behind for the life of the context. camel-sjms now cancels through the task instead of the future.
…ot the service lock doStop holds the service lock and needs the write lock of the cluster view to remove the listener, while the view dispatches events holding its read lock and then needs the consumer lock. A leadership event that passes the isRunAllowed fast path just before a stop acquires the lock closes the two orders into a deadlock. The leadership now has a lock of its own, which doStop releases before it touches the view. The pending start task is also cancelled through the task rather than through its future, so it leaves the TaskManagerRegistry with it.
…ted consumer A leader that uses up backOffMaxAttempts consumes nothing until the leadership changes again, and setting the option to 0 retries for as long as the node is the leader.
fc50d20 to
f1257f1
Compare
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 563 tested, 26 compile-only — current: 562 all testedMaveniverse Scalpel detected 589 affected modules (current approach: 562).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thorough work on all three issues. The lock inversion analysis is precise, and the dedicated leadershipLock is the right fix — narrowing the critical section to just the leadership state rather than the whole service lifecycle cleanly breaks the cycle. The BackgroundTask.cancel() addition fills a real gap in the API, and the test coverage is solid.
A few observations on the concurrency details — nothing blocking, but worth a look.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| */ | ||
| public void cancel(boolean mayInterruptIfRunning) { | ||
| // any run that has not started yet becomes a no-op | ||
| latch.countDown(); |
There was a problem hiding this comment.
💡 Observation (low): cancel() sets running.set(false) at the end, but a concurrent runTaskWrapper() that is already past the latch check and executing the supplier will still be running. After cancel() returns, isRunning() returns false while an attempt may still be in progress (with mayInterruptIfRunning=false).
This is probably fine in practice — callers that need to wait for the in-flight attempt to finish would use mayInterruptIfRunning=true or await the future — but it means isRunning() can briefly lie after cancel(false). Worth a comment on the Javadoc noting that cancel(false) does not wait for an in-flight supplier call to complete.
| running.set(true); | ||
| Future<?> future = service.scheduleWithFixedDelay(() -> runTaskWrapper(camelContext, supplier), | ||
| budget.initialDelay(), budget.interval(), TimeUnit.MILLISECONDS); | ||
| scheduledContext.set(camelContext); |
There was a problem hiding this comment.
💡 Observation (low): scheduledContext is set after scheduledFuture (line 174 vs 173), but cancel() reads scheduledContext in deregister() and scheduledFuture in unschedule() independently. There's a theoretical window where cancel() is called between the two set() calls — it would unschedule the future but deregister() would see a null context and skip the registry removal.
The race guard in runTaskWrapper (lines 108-113) catches this: a run that started before the cancel saw the latch will deregister on its next check. And the existing if (latch.getCount() == 0) { unschedule(false); } at line 176 catches the reverse. So the window is covered by defense-in-depth, but it might be slightly cleaner to set both atomically (or at least set scheduledContext first, before scheduledFuture, since cancel() checks the future first).
Looking again — scheduledContext IS set before scheduledFuture at lines 173-174. So if cancel() fires between them, it sees the context (deregisters) but doesn't see the future yet (skips unschedule). Then line 174 publishes the future, and the final latch.getCount() == 0 check at line 176 catches it. 👍 Order is correct.
| // may wait for the view, and the listener bails out before locking once this consumer is stopping | ||
| // note: removeEventListener below needs the write lock of the cluster view, while an event dispatch | ||
| // takes the read lock of the view and then leadershipLock. This thread must not hold leadershipLock | ||
| // here, or the two orders deadlock |
There was a problem hiding this comment.
💡 Observation (low): doStop() acquires leadershipLock, clears state, releases the lock, then calls view.removeEventListener(). Between the lock release and the removeEventListener, a leadership event could arrive, pass the isRunAllowed() fast check (which returns false since super.doStop() ran first), and bail out — correct. But the comment on line 128-131 explaining this ordering is valuable. It might be worth adding that super.doStop() has already run at this point, so isRunAllowed() is the first gate the listener hits.
| // is cancelled no run is coming | ||
| leaderTask.cancel(mayInterruptIfRunning); | ||
| leaderTask = null; |
There was a problem hiding this comment.
💡 Observation (medium): The cancelLeaderTask method nulls leaderTask and leaderTaskFuture but these fields are not volatile. They're always accessed under leadershipLock, so this is safe — but it's worth noting this invariant somewhere since the fields are declared next to the volatile delegatedConsumer and view at lines 64-65, and a future reader might wonder why some fields are volatile and others aren't.
The leaderTask field could arguably be volatile for consistency with the surrounding declarations, but since all accesses are lock-guarded it's unnecessary overhead.
|
|
||
| view.setLeader(true); | ||
| await().atMost(10, TimeUnit.SECONDS).untilAsserted(() -> assertEquals(1, probe.started.get())); | ||
|
|
||
| CountDownLatch suspendEntered = new CountDownLatch(1); | ||
| CountDownLatch suspendGate = new CountDownLatch(1); | ||
| probe.suspendEntered.set(suspendEntered); | ||
| probe.suspendGate.set(suspendGate); | ||
|
|
||
| // suspending holds the service lock of the master consumer for as long as the delegate takes | ||
| MasterConsumer consumer = (MasterConsumer) context.getRoute("master-route").getConsumer(); | ||
| Thread suspender = new Thread(consumer::suspend, "suspend"); | ||
| suspender.start(); | ||
| assertTrue(suspendEntered.await(20, TimeUnit.SECONDS), "The suspend of the delegate should have started"); | ||
|
|
||
| // the cluster view dispatches its events while holding its own lock, and needs that same lock again | ||
| // to remove the listener when the consumer stops. An event that waits here for the service lock of | ||
| // the consumer is what closes that into a deadlock, so the leadership must not be guarded by it | ||
| Thread dispatcher = new Thread(() -> view.setLeader(true), "leadership-taken"); | ||
| dispatcher.start(); | ||
| try { | ||
| dispatcher.join(TimeUnit.SECONDS.toMillis(20)); |
There was a problem hiding this comment.
💡 Nice test. testEventDispatchIsNotBlockedByALifecycleOperation is a well-designed deadlock detection test — holding the service lock via a blocking suspend, then verifying the dispatch thread completes without waiting. The join(20s) timeout with the alive check is the right pattern for this.
|
@henrik242 take a look at the comments from @gnodet |
3 participants
Fixes 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
BackgroundTaskstays in theTaskManagerRegistry(camel-support)A task scheduled through
BackgroundTask.schedule()adds itself to theTaskManagerRegistryfromits first run, and only a run of the task removes it again. A caller that cancels the
Futurereturned by
schedule()therefore leaves the entry behind for the life of theCamelContext: thetask 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 doesany work, marks the task
Inactiveand removes it from the registry. A run that starts while acancel is landing undoes its own registration, so the two cannot race a stale entry back in.
schedule()is used in exactly two places,camel-sjmsandcamel-master, and both now cancelthrough the task. The other thirteen
Tasks.backgroundTask()users go through the blockingrun(),which already deregisters on every exit path.
2. Lock inversion between the consumer and the cluster view (camel-master)
MasterConsumerguarded its leadership state with theBaseServicelock:doStopholds that lock and then needs the write lock of the view, throughCamelClusterView.removeEventListener.AbstractCamelClusterViewdispatches events while holding its own read lock, and the listener ofthe consumer then needs the
BaseServicelock.The unlocked
isRunAllowed()fast path added in #26028 covers the common case, but a leadershipevent 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
doStopreleases 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
doStopwaits for the leader pool toterminate while an in-flight attempt waits for the lock. It does not:
ExecutorServiceManager.shutdown()passes an await timeout of 0, so that path was unnecessarycoupling rather than a deadlock. The code comment says what actually happens.
3. Exhausted start attempts are not documented (camel-master)
backOffMaxAttemptsdefaults to 10 attempts,backOffDelayapart (5000 millis). A node that usesup its attempts keeps the leadership and consumes nothing until the leadership changes again. That
is the documented intent of the option, and
backOffMaxAttempts=0already retries for as long asthe 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 unpatchedmainand failingthere:
testCancellingAPendingStartRemovesTheTaskFromTheRegistrykeeps a start task retrying, loses theleadership, and asserts the task leaves the registry. Before the change it stays.
testEventDispatchIsNotBlockedByALifecycleOperationholds the service lock through a suspend thatblocks 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
BackgroundTaskTestcover cancelling a task that is running and cancelling one beforeits first run: unscheduled, deregistered,
Inactive, and no further attempt.Green locally: camel-support (119), camel-master (30), the camel-core task tests (34), and
-Psourcecheckon both changed main modules. The camel-sjms main sources compile, but its testsneed an Artemis test-infra artifact that is not installed locally, so CI has to cover that call site.
Notes for the reviewer
backOffMaxAttemptsoption itself is deliberately unchanged. It feedsthree 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.
camel-4.22.xas well: the registry leak through camel-sjms predatesthis work, and the lock inversion came along with the backport of CAMEL-24583. Happy to open a
backport PR if you want it there.