davsclaus commented on code in PR #26112:
URL: https://github.com/apache/camel/pull/26112#discussion_r3949740446
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,57 @@ 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,
and {@link #isAttempting()} keeps
+ * reporting it until it does.
+ *
+ * @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);
+ if (status == Status.Active) {
+ status = Status.Inactive;
+ }
+ completed.set(false);
+ deregister();
+ running.set(false);
+ }
+
Review Comment:
**isRunning() races with an in-progress attempt when
mayInterruptIfRunning=false.**
When cancel(false) is called, the Javadoc correctly says isAttempting()
keeps reporting true until the supplier finishes, but isRunning() is set to
false here immediately, before that supplier call returns.
SimpleMessageListenerContainer (unchanged by this PR) gates on
!recoverTask.isRunning() to decide whether to start a new recovery task (line
237). In practice, doStop() always passes cancel(true) and then nulls
recoverTask, so the gap does not bite the SJMS case today. But as a public API
contract, callers cannot rely on isRunning() to mean no attempt is currently
executing - only isAttempting() can say that when cancel(false) was used.
At minimum, the Javadoc could say: After this method returns, isRunning()
returns false even if an attempt is still in progress; use isAttempting() to
check for that.
##########
core/camel-support/src/main/java/org/apache/camel/support/task/BackgroundTask.java:
##########
@@ -163,22 +171,57 @@ 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,
and {@link #isAttempting()} keeps
+ * reporting it until it does.
+ *
+ * @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);
+ if (status == Status.Active) {
+ status = Status.Inactive;
+ }
+ completed.set(false);
+ deregister();
+ running.set(false);
Review Comment:
**completed.set(false) resets state on an already-completed task.**
If cancel() is called defensively on a task that finished successfully,
completed is reset to false. In the current call sites (all on
actively-retrying tasks) this does not matter, but since cancel() is now public
API it is worth documenting or guarding. Consider completed.compareAndSet(true,
false) or noting in the Javadoc that cancel() is only meaningful on a
still-running task.
--
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]
