gnodet commented on code in PR #26172:
URL: https://github.com/apache/camel/pull/26172#discussion_r3951921812
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,61 @@ public Future<?> schedule(CamelContext camelContext,
BooleanSupplier supplier) {
running.set(true);
Future<?> future = service.scheduleWithFixedDelay(() ->
runTaskWrapper(camelContext, supplier),
budget.initialDelay(), budget.interval(),
TimeUnit.MILLISECONDS);
+ scheduledContext.set(camelContext);
scheduledFuture.set(future);
if (latch.getCount() == 0) {
// the task already finished before the future was published, so
it could not unschedule itself
- unschedule();
+ unschedule(false);
}
return future;
}
+ /**
+ * Cancels a task scheduled with {@link #schedule(CamelContext,
BooleanSupplier)} that is no longer needed, and
+ * removes it from the {@link TaskManagerRegistry}. A scheduled task
deregisters itself from one of its runs, which
+ * is not going to happen once the schedule is cancelled, so cancelling
the returned {@link Future} directly leaves
+ * the task behind in the registry.
+ * <p/>
+ * This does not wait for an attempt that is already running: with {@code
mayInterruptIfRunning} false, a supplier
+ * call that is in progress runs to completion after this method returns.
{@link #isRunning()} answers for the
+ * schedule and turns false here even then, so {@link #isAttempting()} is
the one to ask whether an attempt is still
+ * in flight.
+ * <p/>
+ * A task that already completed, failed or exhausted its budget keeps the
outcome of its last run. Only the
+ * schedule of a task that is still {@link Status#Active} is cancelled,
which turns it {@link Status#Inactive}.
+ *
+ * @param mayInterruptIfRunning whether the thread of an attempt that is
currently running should be interrupted
+ */
+ public void cancel(boolean mayInterruptIfRunning) {
+ // any run that has not started yet becomes a no-op
+ latch.countDown();
+ unschedule(mayInterruptIfRunning);
Review Comment:
**Medium — data race on `status` field**
`AbstractTask.status` is a plain (non-volatile) field. `cancel()` reads and
writes it on the caller thread while `runTaskWrapper()` on the executor thread
may be writing `status = Status.Exhausted / Completed / Failed` at the same
time. The `latch.countDown()` above gates future invocations of
`runTaskWrapper()` but does not prevent a run that is already past the latch
check from writing `status` concurrently.
The Javadoc promises 'a task that already completed keeps the outcome of its
last run,' but this unsynchronised read-modify-write can silently overwrite
`Completed` with `Inactive`.
Cleanest fix: declare `status` `volatile` in `AbstractTask`. It already has
concurrent readers via JMX/management consoles.
##########
components/camel-master/src/main/java/org/apache/camel/component/master/MasterConsumer.java:
##########
@@ -282,12 +296,12 @@ private boolean startDelegatedConsumer(BackgroundTask
task, AtomicReference<Cons
cancelLeaderTask(false);
return true; // no more attempts
} finally {
- lock.unlock();
+ leadershipLock.unlock();
}
}
private void onLeadershipLost() {
- lock.lock();
+ leadershipLock.lock();
Review Comment:
**Low — undocumented locking constraint on `onLeadershipLost()`**
The `doStop()` comment correctly explains why `leadershipLock` must be
released before `removeEventListener`. The symmetric invariant on
`onLeadershipLost()` is missing: this method holds `leadershipLock` and calls
`ServiceHelper.stopAndShutdownServices(delegatedConsumer, delegatedEndpoint)`.
If a delegated consumer's `doStop()` ever touches the cluster view (e.g., to
deregister its own listener), a three-way cycle forms: view-read-lock →
leadershipLock → delegatedConsumer-lock → view-write-lock.
Not a bug in current implementations, but the invariant should be documented
alongside the existing comment on `doStop()` to guard future callers.
--
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]
