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

CoroutineScope.future {} behaves differently to FutureTask when cancelled #2774

Closed
yorickhenning opened this issue Jun 18, 2021 · 0 comments
Closed

Comments

@yorickhenning
Copy link
Contributor

@yorickhenning yorickhenning commented Jun 18, 2021

I think we've found another cancellation corner case in CoroutineScope.future {}.

In a race between asynchronous cancellation from Future.cancel() and cancellation from failure of the executing Job, ListenableFutureCoroutine.onCancelled() may execute after the Future has already been completed as cancelled.

When onCancelled() runs after the Future is complete, it invokes the CoroutineExceptionHandler and escalates the failure:

    override fun onCancelled(cause: Throwable, handled: Boolean) {
        if (!future.completeExceptionallyOrCancel(cause) && !handled) {
            // prevents loss of exception that was not handled by parent & could not be set to JobListenableFuture
            handleCoroutineException(context, cause)
        }
    }

This is different from how FutureTask treats failure after cancellation.

I haven't minimized a test case, but my reading is:

  1. Future.cancel() is called on the returned Future
  2. The cancelling thread of control executes JobListenableFuture.cancel(), succeeds in cancelling the Future, then calls jobToCancel.cancel()
  3. Concurrently, the thread of control executing a continuation in that Job completes in failure, which also calls cancel() on that same Job
  4. If the thread of control executing the continuation wins the race, onCancelled() gets called with a failure, fails to complete the already-complete-as-cancelled Future with that failure, and so invokes the CoroutineExceptionHandler

In contrast, FutureTask uses CAS from NEW -> COMPLETING when the submitted function finishes for any reason. If that CAS fails, the completion is dropped, whether or not the completion is a failure. Cancellation makes the same CAS away from NEW, and there's no else case if completion of the running function with an Exception doesn't win the race.

Since future {} calls the CoroutineExceptionHandler, errors that would be ignored in a function run with an Executor instead execute a failure handler. So at the moment, under cancellation, futures from myCoroutineScope.future(::myFunc) are incompatible with those returned from myExecutor.submit(::myFunc).

I think to maintain compatibility, future {} would have to attempt to complete its Future, but return from the stack and drop the completion if the future is already complete, for any reason?

Incidentally, I think the documentation on ListenableFutureCoroutine is out of date. That class is only used by future {}, but the docstring refers to its use in asListenableFuture {}.

@qwwdfsad qwwdfsad added the bug label Jun 21, 2021
vadimsemenov added a commit to vadimsemenov/kotlinx.coroutines that referenced this issue Jul 23, 2021
…oroutineExceptionHandler.

See  Kotlin#2774 and Kotlin#2791 for more context.

This change makes `future` coroutine builder consistent with `java.util.concurrent.FutureTask` which also drops exceptions that happens after successful cancellation.
yorickhenning pushed a commit to yorickhenning/kotlinx.coroutines that referenced this issue Oct 14, 2021
…oroutineExceptionHandler (Kotlin#2840)

This change makes `future` coroutine builder consistent with `java.util.concurrent.FutureTask` which also drops exceptions that happen after successful cancellation.

Fixes Kotlin#2774
Fixes Kotlin#2791
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants