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 breaks contract of cancel #2791

Closed
jachor opened this issue Jun 25, 2021 · 0 comments
Closed

CoroutineScope.future breaks contract of cancel #2791

jachor opened this issue Jun 25, 2021 · 0 comments

Comments

@jachor
Copy link

@jachor jachor commented Jun 25, 2021

This issue is tricky as CoroutineScope.future ktdoc mentions this behavior:

Note that the error and cancellation semantics of future are subtly different than asListenableFuture’s.
In particular, any exception that happens in the coroutine after returned future is successfully cancelled
will be passed to the CoroutineExceptionHandler from the context. See ListenableFutureCoroutine for details.

I'd like to argue here it is not a good one and actually it means interface of ListenableFuture is incorrectly implemented by CoroutineScope.future -- ListenableFuture contract has cancel() method and it doesn't mention it can sometimes crash whole app. Due to that it fails "Liskov substitution principle" -- one can not use this specific impl of ListenableFuture instead of other ListenableFuture implementations.

To substantiate my observation this trivial code already sporadically will crash due to future behavior:

  @Test
  fun cancellation_withFailureReportingRace_crashesCoroutineScope(): Unit = runBlocking(Dispatchers.IO) {
    val innerFuture = SettableFuture.create<Unit>()
    val outerFuture = future { innerFuture.await() }

    launch { innerFuture.setException(IOException()) }
    launch { outerFuture.cancel(false) }

    // NOTE: This will sporadically crash, I'd expect the failure to be only present
    // in 'outterFuture'/'innerFuture'. I think unhandled exception in CoroutineScope
    // should not happen here.
  }

I'm aware of #2774. I decided to create new issue as it is not about cancel/cancel race, although making cancel atomic might fix both. I don't think making cancel atomic will be fully possible due to constructs such as NonCancellable, but it would fix some issues.

@qwwdfsad qwwdfsad added the bug label Jun 28, 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