diff --git a/integration/kotlinx-coroutines-guava/src/ListenableFuture.kt b/integration/kotlinx-coroutines-guava/src/ListenableFuture.kt index 8f11e0a916..d214cc6b1a 100644 --- a/integration/kotlinx-coroutines-guava/src/ListenableFuture.kt +++ b/integration/kotlinx-coroutines-guava/src/ListenableFuture.kt @@ -14,7 +14,7 @@ import kotlin.coroutines.* /** * Starts [block] in a new coroutine and returns a [ListenableFuture] pointing to its result. * - * The coroutine is immediately started. Passing [CoroutineStart.LAZY] to [start] throws + * The coroutine is started immediately. Passing [CoroutineStart.LAZY] to [start] throws * [IllegalArgumentException], because Futures don't have a way to start lazily. * * When the created coroutine [isCompleted][Job.isCompleted], it will try to @@ -35,10 +35,12 @@ import kotlin.coroutines.* * See [newCoroutineContext][CoroutineScope.newCoroutineContext] for a description of debugging * facilities. * - * 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. + * Note that the error and cancellation semantics of [future] are _different_ than [async]'s. + * In contrast to [Deferred], [Future] doesn't have an intermediate `Cancelling` state. If + * the returned `Future` is successfully cancelled, and `block` throws afterward, the thrown + * error is dropped, and getting the `Future`'s value will throw a `CancellationException` with + * no cause. This is to match the specification and behavior of + * `java.util.concurrent.FutureTask`. * * @param context added overlaying [CoroutineScope.coroutineContext] to form the new context. * @param start coroutine start option. The default value is [CoroutineStart.DEFAULT]. @@ -241,8 +243,8 @@ public suspend fun ListenableFuture.await(): T { return suspendCancellableCoroutine { cont: CancellableContinuation -> addListener( - ToContinuation(this, cont), - MoreExecutors.directExecutor()) + ToContinuation(this, cont), + MoreExecutors.directExecutor()) cont.invokeOnCancellation { cancel(false) } @@ -284,16 +286,13 @@ private class ToContinuation( * By documented contract, a [Future] has been cancelled if * and only if its `isCancelled()` method returns true. * - * Any error that occurs after successfully cancelling a [ListenableFuture] will be passed - * to the [CoroutineExceptionHandler] from the context. The contract of [Future] does not permit - * it to return an error after it is successfully cancelled. - * - * By calling [asListenableFuture] on a [Deferred], any error that occurs after successfully - * cancelling the [ListenableFuture] representation of the [Deferred] will _not_ be passed to - * the [CoroutineExceptionHandler]. Cancelling a [Deferred] places that [Deferred] in the - * cancelling/cancelled states defined by [Job], which _can_ show the error. It's assumed that - * the [Deferred] pointing to the task will be used to observe any error outcome occurring after - * cancellation. + * Any error that occurs after successfully cancelling a [ListenableFuture] is lost. + * The contract of [Future] does not permit it to return an error after it is successfully cancelled. + * On the other hand, we can't report an unhandled exception to [CoroutineExceptionHandler], + * otherwise [Future.cancel] can lead to an app crash which arguably is a contract violation. + * In contrast to [Future] which can't change its outcome after a successful cancellation, + * cancelling a [Deferred] places that [Deferred] in the cancelling/cancelled states defined by [Job], + * which _can_ show the error. * * This may be counterintuitive, but it maintains the error and cancellation contracts of both * the [Deferred] and [ListenableFuture] types, while permitting both kinds of promise to point @@ -312,10 +311,14 @@ private class ListenableFutureCoroutine( } 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) - } + // Note: if future was cancelled in a race with a cancellation of this + // coroutine, and the future was successfully cancelled first, the cause of coroutine + // cancellation is dropped in this promise. A Future can only be completed once. + // + // This is consistent with FutureTask behaviour. A race between a Future.cancel() and + // a FutureTask.setException() for the same Future will similarly drop the + // cause of a failure-after-cancellation. + future.completeExceptionallyOrCancel(cause) } } diff --git a/integration/kotlinx-coroutines-guava/test/ListenableFutureTest.kt b/integration/kotlinx-coroutines-guava/test/ListenableFutureTest.kt index 69ba193071..135de84ec4 100644 --- a/integration/kotlinx-coroutines-guava/test/ListenableFutureTest.kt +++ b/integration/kotlinx-coroutines-guava/test/ListenableFutureTest.kt @@ -555,11 +555,7 @@ class ListenableFutureTest : TestBase() { } @Test - fun testUnhandledExceptionOnExternalCancellation() = runTest( - unhandled = listOf( - { it -> it is TestException } // exception is unhandled because there is no parent - ) - ) { + fun testUnhandledExceptionOnExternalCancellation() = runTest { expect(1) // No parent here (NonCancellable), so nowhere to propagate exception val result = future(NonCancellable + Dispatchers.Unconfined) { @@ -567,7 +563,7 @@ class ListenableFutureTest : TestBase() { delay(Long.MAX_VALUE) } finally { expect(2) - throw TestException() // this exception cannot be handled + throw TestException() // this exception cannot be handled and is set to be lost. } } result.cancel(true) @@ -775,4 +771,39 @@ class ListenableFutureTest : TestBase() { assertEquals(count, completed.get()) } } + + @Test + fun stressTestFutureDoesNotReportToCoroutineExceptionHandler() = runTest { + repeat(1000) { + supervisorScope { // Don't propagate failures in children to parent and other children. + val innerFuture = SettableFuture.create() + val outerFuture = async { innerFuture.await() } + + withContext(Dispatchers.Default) { + launch { innerFuture.setException(TestException("can be lost")) } + launch { outerFuture.cancel() } + // nothing should be reported to CoroutineExceptionHandler, otherwise `Future.cancel` contract violation. + } + } + } + } + + @Test + fun futurePropagatesExceptionToParentAfterCancellation() = runTest { + val latch = CompletableDeferred() + val parent = Job() + val scope = CoroutineScope(parent) + val exception = TestException("propagated to parent") + val future = scope.future { + withContext(NonCancellable) { + latch.await() + throw exception + } + } + future.cancel(true) + latch.complete(true) + parent.join() + assertTrue(parent.isCancelled) + assertEquals(exception, parent.getCancellationException().cause) + } }