From 541a9b6830e6eb978334ebdaacaef2688871b1ce Mon Sep 17 00:00:00 2001 From: Roman Elizarov Date: Tue, 25 Sep 2018 23:23:34 +0300 Subject: [PATCH] Job unwraps all instances of CancellationException * This makes exception handling fully consistent, since CancellationException is considered "normal", never goes to logs and does not cause parent cancellation. * Solves exception transparency of withTimeout/withTimeoutOrNull -- exception throws from inside gets rethrown outside * Makes sure that closed/cancelled producer that throws ClosedSendChannelException is not considered to be failed --- .../src/JobSupport.kt | 4 +-- .../src/Timeout.kt | 4 --- .../test/WithTimeoutOrNullTest.kt | 25 +++++++------ .../test/WithTimeoutTest.kt | 25 +++++++------ .../test/channels/ProduceTest.kt | 2 +- .../test/WithTimeoutJvmTest.kt | 36 ------------------- .../test/WithTimeoutOrNullJvmTest.kt | 22 ------------ 7 files changed, 32 insertions(+), 86 deletions(-) delete mode 100644 core/kotlinx-coroutines-core/test/WithTimeoutJvmTest.kt diff --git a/common/kotlinx-coroutines-core-common/src/JobSupport.kt b/common/kotlinx-coroutines-core-common/src/JobSupport.kt index a8551b73c3..63bc6bb4fb 100644 --- a/common/kotlinx-coroutines-core-common/src/JobSupport.kt +++ b/common/kotlinx-coroutines-core-common/src/JobSupport.kt @@ -247,7 +247,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel * Note that it's only happening when both parent and child throw exception simultaneously. */ var rootCause = exceptions[0] - if (rootCause is JobCancellationException) { + if (rootCause is CancellationException) { val cause = unwrap(rootCause) rootCause = if (cause !== null) { cause @@ -275,7 +275,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel } private tailrec fun unwrap(exception: Throwable): Throwable? = - if (exception is JobCancellationException) { + if (exception is CancellationException) { val cause = exception.cause if (cause !== null) unwrap(cause) else null } else { diff --git a/common/kotlinx-coroutines-core-common/src/Timeout.kt b/common/kotlinx-coroutines-core-common/src/Timeout.kt index 5c9ea070cd..a7ac0e8f44 100644 --- a/common/kotlinx-coroutines-core-common/src/Timeout.kt +++ b/common/kotlinx-coroutines-core-common/src/Timeout.kt @@ -16,8 +16,6 @@ import kotlin.coroutines.experimental.intrinsics.* * * The code that is executing inside the [block] is cancelled on timeout and the active or next invocation of * cancellable suspending function inside the block throws [TimeoutCancellationException]. - * Even if the code in the block suppresses [TimeoutCancellationException], it - * is still thrown by `withTimeout` invocation. * * The sibling function that does not throw exception on timeout is [withTimeoutOrNull]. * Note, that timeout action can be specified for [select] invocation with [onTimeout][SelectBuilder.onTimeout] clause. @@ -40,8 +38,6 @@ public suspend fun withTimeout(timeMillis: Long, block: suspend CoroutineSco * * The code that is executing inside the [block] is cancelled on timeout and the active or next invocation of * cancellable suspending function inside the block throws [TimeoutCancellationException]. - * **NB**: this method is exception-unsafe. Even if the code in the block suppresses [TimeoutCancellationException], this - * invocation of `withTimeoutOrNull` still returns `null` and thrown exception will be ignored. * * The sibling function that throws exception on timeout is [withTimeout]. * Note, that timeout action can be specified for [select] invocation with [onTimeout][SelectBuilder.onTimeout] clause. diff --git a/common/kotlinx-coroutines-core-common/test/WithTimeoutOrNullTest.kt b/common/kotlinx-coroutines-core-common/test/WithTimeoutOrNullTest.kt index aa73f5b3da..a22b0bbe25 100644 --- a/common/kotlinx-coroutines-core-common/test/WithTimeoutOrNullTest.kt +++ b/common/kotlinx-coroutines-core-common/test/WithTimeoutOrNullTest.kt @@ -190,19 +190,24 @@ class WithTimeoutOrNullTest : TestBase() { @Test fun testSuppressExceptionWithAnotherException() = runTest { expect(1) - val value = withTimeoutOrNull(100) { - expect(2) - try { - delay(1000) - } catch (e: CancellationException) { - finish(3) - throw TestException() + try { + withTimeoutOrNull(100) { + expect(2) + try { + delay(1000) + } catch (e: CancellationException) { + expect(3) + throw TestException() + } + expectUnreached() + "OK" } expectUnreached() - "OK" - } + } catch (e: TestException) { + // catches TestException + finish(4) - assertNull(value) + } } private class TestException : Exception() diff --git a/common/kotlinx-coroutines-core-common/test/WithTimeoutTest.kt b/common/kotlinx-coroutines-core-common/test/WithTimeoutTest.kt index 2408f77dee..8f126d557c 100644 --- a/common/kotlinx-coroutines-core-common/test/WithTimeoutTest.kt +++ b/common/kotlinx-coroutines-core-common/test/WithTimeoutTest.kt @@ -147,21 +147,24 @@ class WithTimeoutTest : TestBase() { } @Test - fun testSuppressExceptionWithAnotherException() = runTest(expected = { it is TimeoutCancellationException }) { + fun testSuppressExceptionWithAnotherException() = runTest{ expect(1) - withTimeout(100) { - expect(2) - try { - delay(1000) - } catch (e: CancellationException) { - finish(3) - throw TestException() + try { + withTimeout(100) { + expect(2) + try { + delay(1000) + } catch (e: CancellationException) { + expect(3) + throw TestException() + } + expectUnreached() + "OK" } expectUnreached() - "OK" + } catch (e: TestException) { + finish(4) } - - expectUnreached() } private class TestException : Exception() diff --git a/common/kotlinx-coroutines-core-common/test/channels/ProduceTest.kt b/common/kotlinx-coroutines-core-common/test/channels/ProduceTest.kt index 9b7cc1d342..770077a1c0 100644 --- a/common/kotlinx-coroutines-core-common/test/channels/ProduceTest.kt +++ b/common/kotlinx-coroutines-core-common/test/channels/ProduceTest.kt @@ -38,7 +38,7 @@ class ProduceTest : TestBase() { send(2) // will get cancelled } catch (e: Throwable) { finish(7) - check(e is JobCancellationException && e.job == coroutineContext[Job]) + check(e is ClosedSendChannelException) throw e } expectUnreached() diff --git a/core/kotlinx-coroutines-core/test/WithTimeoutJvmTest.kt b/core/kotlinx-coroutines-core/test/WithTimeoutJvmTest.kt deleted file mode 100644 index 05a16117e2..0000000000 --- a/core/kotlinx-coroutines-core/test/WithTimeoutJvmTest.kt +++ /dev/null @@ -1,36 +0,0 @@ -/* - * Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. - */ - -package kotlinx.coroutines.experimental - -import kotlinx.coroutines.experimental.exceptions.* -import java.io.* -import kotlin.test.* - -/* - * Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license. - */ - -class WithTimeoutJvmTest : TestBase() { - - @Test - fun testSuppressExceptionWithAnotherException() = runTest(expected = - { it is TimeoutCancellationException && checkException(it.suppressed()[0]) }) { - - expect(1) - withTimeout(100) { - expect(2) - try { - delay(1000) - } catch (e: CancellationException) { - finish(3) - throw IOException() - } - expectUnreached() - "OK" - } - - expectUnreached() - } -} \ No newline at end of file diff --git a/core/kotlinx-coroutines-core/test/WithTimeoutOrNullJvmTest.kt b/core/kotlinx-coroutines-core/test/WithTimeoutOrNullJvmTest.kt index af09cd6b99..9bfd3a1602 100644 --- a/core/kotlinx-coroutines-core/test/WithTimeoutOrNullJvmTest.kt +++ b/core/kotlinx-coroutines-core/test/WithTimeoutOrNullJvmTest.kt @@ -8,28 +8,6 @@ import java.io.* import kotlin.test.* class WithTimeoutOrNullJvmTest : TestBase() { - - - @Test - fun testCancellationSuppression() = runTest { - - expect(1) - val value = withTimeoutOrNull(100) { - expect(2) - try { - delay(1000) - } catch (e: CancellationException) { - expect(3) - throw IOException() - } - expectUnreached() - "OK" - } - - assertNull(value) - finish(4) - } - @Test fun testOuterTimeoutFiredBeforeInner() = runTest { val result = withTimeoutOrNull(100) {