Exception handling update:
Add additional argument to handleCoroutineException to avoid cycling cancellation
Fix multiple races in JobSupport
Improve case with multiple suppression of the same exception
diff --git a/binary-compatibility-validator/reference-public-api/kotlinx-coroutines-core.txt b/binary-compatibility-validator/reference-public-api/kotlinx-coroutines-core.txt
index b22ce4e..248b432 100644
--- a/binary-compatibility-validator/reference-public-api/kotlinx-coroutines-core.txt
+++ b/binary-compatibility-validator/reference-public-api/kotlinx-coroutines-core.txt
@@ -166,6 +166,8 @@
public final class kotlinx/coroutines/experimental/CoroutineExceptionHandlerKt {
public static final fun CoroutineExceptionHandler (Lkotlin/jvm/functions/Function2;)Lkotlinx/coroutines/experimental/CoroutineExceptionHandler;
public static final fun handleCoroutineException (Lkotlin/coroutines/experimental/CoroutineContext;Ljava/lang/Throwable;)V
+ public static final fun handleCoroutineException (Lkotlin/coroutines/experimental/CoroutineContext;Ljava/lang/Throwable;Lkotlinx/coroutines/experimental/Job;)V
+ public static synthetic fun handleCoroutineException$default (Lkotlin/coroutines/experimental/CoroutineContext;Ljava/lang/Throwable;Lkotlinx/coroutines/experimental/Job;ILjava/lang/Object;)V
}
public final class kotlinx/coroutines/experimental/CoroutineName : kotlin/coroutines/experimental/AbstractCoroutineContextElement {
diff --git a/common/kotlinx-coroutines-core-common/src/AbstractCoroutine.kt b/common/kotlinx-coroutines-core-common/src/AbstractCoroutine.kt
index 9a9a7d5..6d7874b 100644
--- a/common/kotlinx-coroutines-core-common/src/AbstractCoroutine.kt
+++ b/common/kotlinx-coroutines-core-common/src/AbstractCoroutine.kt
@@ -111,7 +111,7 @@
}
internal final override fun handleOnCompletionException(exception: Throwable) {
- handleCoroutineException(parentContext, exception)
+ handleCoroutineException(parentContext, exception, this)
}
internal override fun nameString(): String {
diff --git a/common/kotlinx-coroutines-core-common/src/Annotations.common.kt b/common/kotlinx-coroutines-core-common/src/Annotations.common.kt
index 8b2a65a..ae92d68 100644
--- a/common/kotlinx-coroutines-core-common/src/Annotations.common.kt
+++ b/common/kotlinx-coroutines-core-common/src/Annotations.common.kt
@@ -9,6 +9,9 @@
@Target(AnnotationTarget.FILE, AnnotationTarget.FUNCTION)
internal expect annotation class JvmName(val name: String)
+@Target(AnnotationTarget.FUNCTION, AnnotationTarget.CONSTRUCTOR)
+internal expect annotation class JvmOverloads()
+
@Target(AnnotationTarget.FILE)
internal expect annotation class JvmMultifileClass()
diff --git a/common/kotlinx-coroutines-core-common/src/Builders.common.kt b/common/kotlinx-coroutines-core-common/src/Builders.common.kt
index c599639..b515979 100644
--- a/common/kotlinx-coroutines-core-common/src/Builders.common.kt
+++ b/common/kotlinx-coroutines-core-common/src/Builders.common.kt
@@ -159,7 +159,7 @@
override fun hasOnFinishingHandler(update: Any?) = update is CompletedExceptionally
override fun handleJobException(exception: Throwable) {
- handleCoroutineException(parentContext, exception)
+ handleCoroutineException(parentContext, exception, this)
}
override fun onFinishingInternal(update: Any?) {
diff --git a/common/kotlinx-coroutines-core-common/src/CoroutineExceptionHandler.kt b/common/kotlinx-coroutines-core-common/src/CoroutineExceptionHandler.kt
index 6e93b77..93c7183 100644
--- a/common/kotlinx-coroutines-core-common/src/CoroutineExceptionHandler.kt
+++ b/common/kotlinx-coroutines-core-common/src/CoroutineExceptionHandler.kt
@@ -4,6 +4,7 @@
package kotlinx.coroutines.experimental
+import kotlinx.coroutines.experimental.internalAnnotations.*
import kotlin.coroutines.experimental.*
internal expect fun handleCoroutineExceptionImpl(context: CoroutineContext, exception: Throwable)
@@ -15,12 +16,13 @@
* If current exception is [CancellationException], it's ignored: [CancellationException] is a normal way to cancel
* coroutine.
*
- * If there is a [Job] in the context, then [Job.cancel] is invoked.
+ * If there is a [Job] in the context and it's not a [caller], then [Job.cancel] is invoked.
* If invocation returned `true`, method terminates: now [Job] is responsible for handling an exception.
* Otherwise, If there is [CoroutineExceptionHandler] in the context, it is used.
* Otherwise all instances of [CoroutineExceptionHandler] found via [ServiceLoader] and [Thread.uncaughtExceptionHandler] are invoked
*/
-public fun handleCoroutineException(context: CoroutineContext, exception: Throwable) {
+@JvmOverloads // binary compatibility
+public fun handleCoroutineException(context: CoroutineContext, exception: Throwable, caller: Job? = null) {
// if exception handling fails, make sure the original exception is not lost
try {
@@ -31,7 +33,8 @@
// If parent is successfully cancelled, we're done, it is now its responsibility to handle the exception
val parent = context[Job]
- if (parent != null && parent.cancel(exception)) {
+ // E.g. actor registers itself in the context, in that case we should invoke handler
+ if (parent !== null && parent !== caller && parent.cancel(exception)) {
return
}
diff --git a/common/kotlinx-coroutines-core-common/src/JobSupport.kt b/common/kotlinx-coroutines-core-common/src/JobSupport.kt
index 33cc4d3..5756e4e 100644
--- a/common/kotlinx-coroutines-core-common/src/JobSupport.kt
+++ b/common/kotlinx-coroutines-core-common/src/JobSupport.kt
@@ -162,7 +162,6 @@
val update = coerceProposedUpdate(expect, proposedUpdate)
if (!tryFinalizeState(expect, update)) return false
- if (update is CompletedExceptionally) handleJobException(update.cause)
completeStateFinalization(expect, update, mode)
return true
}
@@ -194,7 +193,6 @@
}
val update = Cancelled(this, finalException ?: expect.cancelled!!.cause)
- handleJobException(update.cause)
// This CAS never fails: we're in the state when no jobs can be attached, because state is already sealed
if (!tryFinalizeState(expect, update)) {
val error = AssertionError("Unexpected state: ${_state.value}, expected: $expect, update: $update")
@@ -238,7 +236,8 @@
}
}
- val seenExceptions = HashSet<Throwable>() // TODO it should be identity set
+ // TODO it should be identity set and optimized for small footprints
+ val seenExceptions = HashSet<Throwable>(suppressed.size)
suppressed.forEach {
val unwrapped = unwrap(it)
if (unwrapped !== null && unwrapped !== rootCause) {
@@ -269,6 +268,7 @@
private fun tryFinalizeState(expect: Incomplete, update: Any?): Boolean {
require(update !is Incomplete) // only incomplete -> completed transition is allowed
if (!_state.compareAndSet(expect, update)) return false
+ if (update is CompletedExceptionally) handleJobException(update.cause)
// Unregister from parent job
parentHandle?.let {
it.dispose() // volatile read parentHandle _after_ state was updated
@@ -614,12 +614,26 @@
return true
}
- // We either successfully added an exception or caller should handle it itself
- return cause.let { state.addException(it) }
+ /*
+ * If we failed to add an exception, then `seal` was successfully called
+ * and `seal` is called only after state update => retry is liveness-safe
+ */
+ if (state.addException(cause)) {
+ return true
+ } else {
+ return@loopOnState
+ }
}
if (tryMakeCancelling(state, state.list, cause)) return true
}
+
+ /*
+ * Filter out duplicates due to race in the following pattern:
+ * T1: parent -> completion sequence
+ * T2: child -> set final state -> signal with final exception to the parent
+ */
+ is CompletedExceptionally -> return state.cause === cause
else -> { // is inactive
return false
}
@@ -694,6 +708,7 @@
val cancelled = (state as? Finishing)?.cancelled ?: (proposedUpdate as? Cancelled)
val completing = Finishing(list, cancelled, true)
if (_state.compareAndSet(state, completing)) {
+ (state as? Finishing)?.transferExceptions(completing)
if (state !is Finishing) onFinishingInternal(proposedUpdate)
if (child != null && tryWaitForChild(child, proposedUpdate))
return COMPLETING_WAITING_CHILDREN
@@ -802,8 +817,8 @@
internal open fun onFinishingInternal(update: Any?) {}
/**
- * Method which is invoked once Job becomes `Cancelled`. It's guaranteed that at the moment
- * of invocation the job and all its children are complete
+ * Method which is invoked once Job becomes [Cancelled] or [CompletedExceptionally].
+ * It's guaranteed that at the moment of invocation the job and all its children are complete
*/
internal open fun handleJobException(exception: Throwable) {}
@@ -875,6 +890,7 @@
}
}
+ // guarded by `synchronized(this)`
fun addLocked(exception: Throwable) {
// Cannot be null at this point here
when (_exceptionsHolder) {
@@ -901,6 +917,18 @@
_exceptionsHolder = null
}
+ fun transferExceptions(to: Finishing) {
+ synchronized(this) {
+ synchronized(to) {
+ val holder = _exceptionsHolder
+ when(holder) {
+ is Throwable -> to.addLocked(holder)
+ is List<*> -> holder.forEach { to.addLocked(it as Throwable) }
+ }
+ seal()
+ }
+ }
+ }
}
private val Incomplete.isCancelling: Boolean
diff --git a/common/kotlinx-coroutines-core-common/src/channels/Broadcast.kt b/common/kotlinx-coroutines-core-common/src/channels/Broadcast.kt
index 403c084..f55b521 100644
--- a/common/kotlinx-coroutines-core-common/src/channels/Broadcast.kt
+++ b/common/kotlinx-coroutines-core-common/src/channels/Broadcast.kt
@@ -101,7 +101,7 @@
else -> _channel.close(cause) // producer coroutine has completed -- close channel
}
if (!processed && cause != null)
- handleCoroutineException(context, cause)
+ handleCoroutineException(context, cause, this)
}
}
diff --git a/common/kotlinx-coroutines-core-common/src/channels/Produce.kt b/common/kotlinx-coroutines-core-common/src/channels/Produce.kt
index 5ad8f56..4e9510c 100644
--- a/common/kotlinx-coroutines-core-common/src/channels/Produce.kt
+++ b/common/kotlinx-coroutines-core-common/src/channels/Produce.kt
@@ -124,6 +124,6 @@
else -> _channel.close(cause) // producer coroutine has completed -- close channel
}
if (!processed && cause != null)
- handleCoroutineException(context, cause)
+ handleCoroutineException(context, cause, this)
}
}
diff --git a/core/kotlinx-coroutines-core/src/channels/Actor.kt b/core/kotlinx-coroutines-core/src/channels/Actor.kt
index bc9bb46..2139469 100644
--- a/core/kotlinx-coroutines-core/src/channels/Actor.kt
+++ b/core/kotlinx-coroutines-core/src/channels/Actor.kt
@@ -160,12 +160,10 @@
) : ChannelCoroutine<E>(parentContext, channel, active), ActorScope<E>, ActorJob<E> {
override fun onCancellation(cause: Throwable?) {
_channel.cancel(cause)
- // Always propagate the exception, don't wait for actor senders
- if (cause != null) handleCoroutineException(context, cause)
}
override fun handleJobException(exception: Throwable) {
- handleCoroutineException(context, exception)
+ handleCoroutineException(context, exception, this)
}
}
diff --git a/core/kotlinx-coroutines-core/src/Annotations.kt b/core/kotlinx-coroutines-core/src/internalAnnotations/Annotations.kt
similarity index 85%
rename from core/kotlinx-coroutines-core/src/Annotations.kt
rename to core/kotlinx-coroutines-core/src/internalAnnotations/Annotations.kt
index 3faf712..c5c5cfc 100644
--- a/core/kotlinx-coroutines-core/src/Annotations.kt
+++ b/core/kotlinx-coroutines-core/src/internalAnnotations/Annotations.kt
@@ -8,6 +8,9 @@
internal actual typealias JvmName = kotlin.jvm.JvmName
@Suppress("ACTUAL_WITHOUT_EXPECT")
+internal actual typealias JvmOverloads = kotlin.jvm.JvmOverloads
+
+@Suppress("ACTUAL_WITHOUT_EXPECT")
internal actual typealias JvmMultifileClass = kotlin.jvm.JvmMultifileClass
@Suppress("ACTUAL_WITHOUT_EXPECT")
diff --git a/core/kotlinx-coroutines-core/test/channels/ActorTest.kt b/core/kotlinx-coroutines-core/test/channels/ActorTest.kt
index d9853e1..9546fc3 100644
--- a/core/kotlinx-coroutines-core/test/channels/ActorTest.kt
+++ b/core/kotlinx-coroutines-core/test/channels/ActorTest.kt
@@ -150,4 +150,24 @@
parent.join()
finish(2)
}
+
+ @Test
+ fun testChildJob() = runTest {
+ val parent = Job()
+ actor<Int>(context = coroutineContext, parent = parent) {
+ launch(coroutineContext) {
+ try {
+ delay(Long.MAX_VALUE)
+ } finally {
+ expect(1)
+ }
+ }
+ }
+
+ yield()
+ yield()
+ parent.cancel()
+ parent.join()
+ finish(2)
+ }
}
diff --git a/core/kotlinx-coroutines-core/test/exceptions/Exceptions.kt b/core/kotlinx-coroutines-core/test/exceptions/Exceptions.kt
index 10642ae..ba8428f 100644
--- a/core/kotlinx-coroutines-core/test/exceptions/Exceptions.kt
+++ b/core/kotlinx-coroutines-core/test/exceptions/Exceptions.kt
@@ -1,3 +1,7 @@
+/*
+ * Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
package kotlinx.coroutines.experimental.exceptions
import kotlinx.coroutines.experimental.*
@@ -38,7 +42,8 @@
}
fun getException(): Throwable {
- assert(unhandled.size == 1) { "Expected one unhandled exception, but have ${unhandled.size}: $unhandled" }
+ val size = unhandled.size
+ assert(size == 1) { "Expected one unhandled exception, but have $size: $unhandled" }
return unhandled[0]
}
}
diff --git a/core/kotlinx-coroutines-core/test/exceptions/JobBasicCancellationTest.kt b/core/kotlinx-coroutines-core/test/exceptions/JobBasicCancellationTest.kt
index 5706a59..4f82015 100644
--- a/core/kotlinx-coroutines-core/test/exceptions/JobBasicCancellationTest.kt
+++ b/core/kotlinx-coroutines-core/test/exceptions/JobBasicCancellationTest.kt
@@ -1,3 +1,7 @@
+/*
+ * Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
package kotlinx.coroutines.experimental.exceptions
import kotlinx.coroutines.experimental.*
@@ -9,7 +13,7 @@
/*
* Basic checks that check that cancellation more or less works,
* parent is not cancelled on child cancellation and launch {}, Job(), async {} and
- * CompletableDeferred are indistinguishable
+ * CompletableDeferred behave properly
*/
class JobBasicCancellationTest : TestBase() {
@@ -42,15 +46,14 @@
expect(2)
assertTrue(child.cancel())
- try {
- child.join()
- } catch (e: Exception) {
- throw e
- }
+ child.join()
+ yield()
expect(4)
}
parent.join()
+ assertTrue(parent.isCompleted)
+ assertFalse(parent.isCancelled)
finish(5)
}
@@ -92,6 +95,28 @@
}
@Test
+ fun testNestedAsyncFailure() = runTest {
+ val deferred = async(coroutineContext) {
+ val nested = async(coroutineContext) {
+ expect(3)
+ throw IOException()
+ }
+
+ expect(2)
+ yield()
+ expect(4)
+ nested.await()
+ }
+
+ expect(1)
+ try {
+ deferred.await()
+ } catch (e: IOException) {
+ finish(5)
+ }
+ }
+
+ @Test
fun testCancelJobImpl() = runTest {
val parent = launch(coroutineContext) {
expect(1)
diff --git a/core/kotlinx-coroutines-core/test/exceptions/JobExceptionHandlingTest.kt b/core/kotlinx-coroutines-core/test/exceptions/JobExceptionHandlingTest.kt
index 0c28ed0..665a72b 100644
--- a/core/kotlinx-coroutines-core/test/exceptions/JobExceptionHandlingTest.kt
+++ b/core/kotlinx-coroutines-core/test/exceptions/JobExceptionHandlingTest.kt
@@ -1,3 +1,7 @@
+/*
+ * Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
package kotlinx.coroutines.experimental.exceptions
import kotlinx.coroutines.experimental.*
diff --git a/core/kotlinx-coroutines-core/test/exceptions/JobExceptionsStressTest.kt b/core/kotlinx-coroutines-core/test/exceptions/JobExceptionsStressTest.kt
index 2a60c8d..d55f973 100644
--- a/core/kotlinx-coroutines-core/test/exceptions/JobExceptionsStressTest.kt
+++ b/core/kotlinx-coroutines-core/test/exceptions/JobExceptionsStressTest.kt
@@ -2,10 +2,9 @@
* Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/
-package exceptions
+package kotlinx.coroutines.experimental.exceptions
import kotlinx.coroutines.experimental.*
-import kotlinx.coroutines.experimental.exceptions.*
import org.junit.*
import org.junit.Test
import java.io.*
@@ -69,7 +68,7 @@
"Failed to remove ${throwable::class} from $suppressedExceptions")
}
- assertTrue(classes.isEmpty())
+ assertTrue(classes.isEmpty(), "Expected all exception to be present, but following exceptions are missing: $classes")
}
}
-}
\ No newline at end of file
+}
diff --git a/core/kotlinx-coroutines-core/test/exceptions/JobNestedExceptionsTest.kt b/core/kotlinx-coroutines-core/test/exceptions/JobNestedExceptionsTest.kt
index bbafb0c..ea66dd0 100644
--- a/core/kotlinx-coroutines-core/test/exceptions/JobNestedExceptionsTest.kt
+++ b/core/kotlinx-coroutines-core/test/exceptions/JobNestedExceptionsTest.kt
@@ -1,3 +1,7 @@
+/*
+ * Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
package kotlinx.coroutines.experimental.exceptions
import kotlinx.coroutines.experimental.*
@@ -109,8 +113,7 @@
finish(5)
}
- assertEquals(2, exceptions.size)
- assertTrue(exceptions[0] === exceptions[1]) // Sad part about sharing exceptions :(
+ assertEquals(1, exceptions.size)
val exception = exceptions[0]
val suppressed = exception.suppressed()
checkException<IOException>(suppressed[0])
diff --git a/core/kotlinx-coroutines-core/test/exceptions/SuppresionTests.kt b/core/kotlinx-coroutines-core/test/exceptions/SuppresionTests.kt
index 24efb48..51498e2 100644
--- a/core/kotlinx-coroutines-core/test/exceptions/SuppresionTests.kt
+++ b/core/kotlinx-coroutines-core/test/exceptions/SuppresionTests.kt
@@ -2,10 +2,11 @@
* Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/
-package exceptions
+package kotlinx.coroutines.experimental.exceptions
import kotlinx.coroutines.experimental.*
import kotlinx.coroutines.experimental.exceptions.*
+import kotlinx.coroutines.experimental.selects.*
import java.io.*
import kotlin.test.*
diff --git a/js/kotlinx-coroutines-core-js/src/Annotations.kt b/js/kotlinx-coroutines-core-js/src/Annotations.kt
index 4f6595a..8004645 100644
--- a/js/kotlinx-coroutines-core-js/src/Annotations.kt
+++ b/js/kotlinx-coroutines-core-js/src/Annotations.kt
@@ -7,6 +7,9 @@
@Target(AnnotationTarget.FILE, AnnotationTarget.FUNCTION)
internal actual annotation class JvmName(actual val name: String)
+@Target(AnnotationTarget.CONSTRUCTOR, AnnotationTarget.FUNCTION)
+internal actual annotation class JvmOverloads
+
@Target(AnnotationTarget.FILE)
internal actual annotation class JvmMultifileClass
diff --git a/native/kotlinx-coroutines-core-native/src/Annotations.kt b/native/kotlinx-coroutines-core-native/src/Annotations.kt
index 0f3306c..09223fd 100644
--- a/native/kotlinx-coroutines-core-native/src/Annotations.kt
+++ b/native/kotlinx-coroutines-core-native/src/Annotations.kt
@@ -7,6 +7,9 @@
@Target(AnnotationTarget.FILE, AnnotationTarget.FUNCTION)
internal actual annotation class JvmName(actual val name: String)
+@Target(AnnotationTarget.CONSTRUCTOR, AnnotationTarget.FUNCTION)
+internal actual annotation class JvmOverloads
+
@Target(AnnotationTarget.FILE)
internal actual annotation class JvmMultifileClass
diff --git a/reactive/kotlinx-coroutines-reactive/src/Publish.kt b/reactive/kotlinx-coroutines-reactive/src/Publish.kt
index aa1a4a5..66c6675 100644
--- a/reactive/kotlinx-coroutines-reactive/src/Publish.kt
+++ b/reactive/kotlinx-coroutines-reactive/src/Publish.kt
@@ -121,7 +121,7 @@
} catch (e: Throwable) {
try {
if (!cancel(e))
- handleCoroutineException(context, e)
+ handleCoroutineException(context, e, this)
} finally {
doLockedSignalCompleted()
}
@@ -162,7 +162,7 @@
else
subscriber.onComplete()
} catch (e: Throwable) {
- handleCoroutineException(context, e)
+ handleCoroutineException(context, e, this)
}
}
} finally {
diff --git a/reactive/kotlinx-coroutines-rx1/src/RxObservable.kt b/reactive/kotlinx-coroutines-rx1/src/RxObservable.kt
index cbe75d3..7a19152 100644
--- a/reactive/kotlinx-coroutines-rx1/src/RxObservable.kt
+++ b/reactive/kotlinx-coroutines-rx1/src/RxObservable.kt
@@ -122,7 +122,7 @@
} catch (e: Throwable) {
try {
if (!cancel(e))
- handleCoroutineException(context, e)
+ handleCoroutineException(context, e, this)
} finally {
doLockedSignalCompleted()
}
@@ -163,7 +163,7 @@
else
subscriber.onCompleted()
} catch (e: Throwable) {
- handleCoroutineException(context, e)
+ handleCoroutineException(context, e, this)
}
}
} finally {
diff --git a/reactive/kotlinx-coroutines-rx2/src/RxObservable.kt b/reactive/kotlinx-coroutines-rx2/src/RxObservable.kt
index 82eeca8..dab014f 100644
--- a/reactive/kotlinx-coroutines-rx2/src/RxObservable.kt
+++ b/reactive/kotlinx-coroutines-rx2/src/RxObservable.kt
@@ -123,7 +123,7 @@
} catch (e: Throwable) {
try {
if (!cancel(e))
- handleCoroutineException(context, e)
+ handleCoroutineException(context, e, this)
} finally {
doLockedSignalCompleted()
}
@@ -153,7 +153,7 @@
else
subscriber.onComplete()
} catch (e: Throwable) {
- handleCoroutineException(context, e)
+ handleCoroutineException(context, e, this)
}
}
} finally {