Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repeating scheduled task does not get cancelled if cancel is called from within the task itself #163

Closed
dmlloyd opened this issue Mar 27, 2024 · 0 comments · Fixed by #164

Comments

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 27, 2024

According to the documentation of Future#cancel:

Attempts to cancel execution of this task. This method has no effect if the task is already completed or cancelled, or could not be cancelled for some other reason. Otherwise, if this task has not started when cancel is called, this task should never run. If the task has already started, then the mayInterruptIfRunning parameter determines whether the thread executing this task (when known by the implementation) is interrupted in an attempt to stop the task.

The return value from this method does not necessarily indicate whether the task is now cancelled; use isCancelled.

However, the ScheduledFuture class does not elaborate on this behavior when the task represents a repeating task. Instead, in ScheduledExecutorService, the doc for scheduleAtFixedRate says:

The sequence of task executions continues indefinitely until one of the following exceptional completions occur:

  • The task is explicitly cancelled via the returned future.
  • The executor terminates, also resulting in task cancellation.
  • An execution of the task throws an exception. In this case calling get on the returned future will throw ExecutionException, holding the exception as its cause.

Subsequent executions are suppressed. Subsequent calls to isDone() on the returned future will return true.

The implementation in FutureTask does cancel the task if it is running but only if the mayInterruptIfRunning flag is given as true. In this case, if the caller is the first thread to request interruption of the task, then true is returned from cancel(true) for that caller only (subsequent attempts will return false). This is contrary to the Javadoc for that method, which says:

Returns: false if the task could not be cancelled, typically because it has already completed; true otherwise. If two or more threads cause a task to be cancelled, then at least one of them returns true. Implementations may provide stronger guarantees.

The implication here is that true is returned if the task could be cancelled. Not "was cancelled" or "might be cancelled" or "is cancellable", but "could be cancelled". A logical and semantic disaster.

Again referring to the JDK implementation, we can infer that the correct specification of the method must be closer to "Returns true if the task was successfully requested to cancel".

Bringing it back to planet Earth, the current scheduling implementation of EQE will return false if a scheduled task is requested to be cancelled while the task is running. Currently, setting the mayInterruptIfRunning flag will interrupt the task, but the task will still complete successfully.

The best possible way to modify the EQE behavior to match the JDK is probably to add a "pending cancel" state. In this state, the task is cancelled as soon as its execution completes. Transitioning a task to this state will cause cancel(xx) to return true. The isCancelled method would also return true when in this state.

dmlloyd added a commit to dmlloyd/jboss-threads that referenced this issue Mar 27, 2024
Prevent a problem where a repeating task cannot be cancelled if it is already running. Fixes jbossas#163.
dmlloyd added a commit that referenced this issue Mar 27, 2024
Prevent a problem where a repeating task cannot be cancelled if it is already running. Fixes #163.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant