Skip to content

Commit 91ecee8

Browse files
qwwdfsadelizarov
qwwdfsad
authored and
elizarov
committed
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
1 parent c9afb67 commit 91ecee8

File tree

22 files changed

+137
-37
lines changed

22 files changed

+137
-37
lines changed

‎binary-compatibility-validator/reference-public-api/kotlinx-coroutines-core.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,8 @@ public final class kotlinx/coroutines/experimental/CoroutineExceptionHandler$Key
166166
public final class kotlinx/coroutines/experimental/CoroutineExceptionHandlerKt {
167167
public static final fun CoroutineExceptionHandler (Lkotlin/jvm/functions/Function2;)Lkotlinx/coroutines/experimental/CoroutineExceptionHandler;
168168
public static final fun handleCoroutineException (Lkotlin/coroutines/experimental/CoroutineContext;Ljava/lang/Throwable;)V
169+
public static final fun handleCoroutineException (Lkotlin/coroutines/experimental/CoroutineContext;Ljava/lang/Throwable;Lkotlinx/coroutines/experimental/Job;)V
170+
public static synthetic fun handleCoroutineException$default (Lkotlin/coroutines/experimental/CoroutineContext;Ljava/lang/Throwable;Lkotlinx/coroutines/experimental/Job;ILjava/lang/Object;)V
169171
}
170172

171173
public final class kotlinx/coroutines/experimental/CoroutineName : kotlin/coroutines/experimental/AbstractCoroutineContextElement {

‎common/kotlinx-coroutines-core-common/src/AbstractCoroutine.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ public abstract class AbstractCoroutine<in T>(
111111
}
112112

113113
internal final override fun handleOnCompletionException(exception: Throwable) {
114-
handleCoroutineException(parentContext, exception)
114+
handleCoroutineException(parentContext, exception, this)
115115
}
116116

117117
internal override fun nameString(): String {

‎common/kotlinx-coroutines-core-common/src/Annotations.common.kt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ package kotlinx.coroutines.experimental.internalAnnotations
99
@Target(AnnotationTarget.FILE, AnnotationTarget.FUNCTION)
1010
internal expect annotation class JvmName(val name: String)
1111

12+
@Target(AnnotationTarget.FUNCTION, AnnotationTarget.CONSTRUCTOR)
13+
internal expect annotation class JvmOverloads()
14+
1215
@Target(AnnotationTarget.FILE)
1316
internal expect annotation class JvmMultifileClass()
1417

‎common/kotlinx-coroutines-core-common/src/Builders.common.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ private open class StandaloneCoroutine(
159159
override fun hasOnFinishingHandler(update: Any?) = update is CompletedExceptionally
160160

161161
override fun handleJobException(exception: Throwable) {
162-
handleCoroutineException(parentContext, exception)
162+
handleCoroutineException(parentContext, exception, this)
163163
}
164164

165165
override fun onFinishingInternal(update: Any?) {

‎common/kotlinx-coroutines-core-common/src/CoroutineExceptionHandler.kt‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
package kotlinx.coroutines.experimental
66

7+
import kotlinx.coroutines.experimental.internalAnnotations.*
78
import kotlin.coroutines.experimental.*
89

910
internal expect fun handleCoroutineExceptionImpl(context: CoroutineContext, exception: Throwable)
@@ -15,12 +16,13 @@ internal expect fun handleCoroutineExceptionImpl(context: CoroutineContext, exce
1516
* If current exception is [CancellationException], it's ignored: [CancellationException] is a normal way to cancel
1617
* coroutine.
1718
*
18-
* If there is a [Job] in the context, then [Job.cancel] is invoked.
19+
* If there is a [Job] in the context and it's not a [caller], then [Job.cancel] is invoked.
1920
* If invocation returned `true`, method terminates: now [Job] is responsible for handling an exception.
2021
* Otherwise, If there is [CoroutineExceptionHandler] in the context, it is used.
2122
* Otherwise all instances of [CoroutineExceptionHandler] found via [ServiceLoader] and [Thread.uncaughtExceptionHandler] are invoked
2223
*/
23-
public fun handleCoroutineException(context: CoroutineContext, exception: Throwable) {
24+
@JvmOverloads // binary compatibility
25+
public fun handleCoroutineException(context: CoroutineContext, exception: Throwable, caller: Job? = null) {
2426
// if exception handling fails, make sure the original exception is not lost
2527
try {
2628

@@ -31,7 +33,8 @@ public fun handleCoroutineException(context: CoroutineContext, exception: Throwa
3133

3234
// If parent is successfully cancelled, we're done, it is now its responsibility to handle the exception
3335
val parent = context[Job]
34-
if (parent != null && parent.cancel(exception)) {
36+
// E.g. actor registers itself in the context, in that case we should invoke handler
37+
if (parent !== null && parent !== caller && parent.cancel(exception)) {
3538
return
3639
}
3740

‎common/kotlinx-coroutines-core-common/src/JobSupport.kt‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,6 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
162162

163163
val update = coerceProposedUpdate(expect, proposedUpdate)
164164
if (!tryFinalizeState(expect, update)) return false
165-
if (update is CompletedExceptionally) handleJobException(update.cause)
166165
completeStateFinalization(expect, update, mode)
167166
return true
168167
}
@@ -194,7 +193,6 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
194193
}
195194

196195
val update = Cancelled(this, finalException ?: expect.cancelled!!.cause)
197-
handleJobException(update.cause)
198196
// This CAS never fails: we're in the state when no jobs can be attached, because state is already sealed
199197
if (!tryFinalizeState(expect, update)) {
200198
val error = AssertionError("Unexpected state: ${_state.value}, expected: $expect, update: $update")
@@ -238,7 +236,8 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
238236
}
239237
}
240238

241-
val seenExceptions = HashSet<Throwable>() // TODO it should be identity set
239+
// TODO it should be identity set and optimized for small footprints
240+
val seenExceptions = HashSet<Throwable>(suppressed.size)
242241
suppressed.forEach {
243242
val unwrapped = unwrap(it)
244243
if (unwrapped !== null && unwrapped !== rootCause) {
@@ -269,6 +268,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
269268
private fun tryFinalizeState(expect: Incomplete, update: Any?): Boolean {
270269
require(update !is Incomplete) // only incomplete -> completed transition is allowed
271270
if (!_state.compareAndSet(expect, update)) return false
271+
if (update is CompletedExceptionally) handleJobException(update.cause)
272272
// Unregister from parent job
273273
parentHandle?.let {
274274
it.dispose() // volatile read parentHandle _after_ state was updated
@@ -614,12 +614,26 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
614614
return true
615615
}
616616

617-
// We either successfully added an exception or caller should handle it itself
618-
return cause.let { state.addException(it) }
617+
/*
618+
* If we failed to add an exception, then `seal` was successfully called
619+
* and `seal` is called only after state update => retry is liveness-safe
620+
*/
621+
if (state.addException(cause)) {
622+
return true
623+
} else {
624+
return@loopOnState
625+
}
619626
}
620627

621628
if (tryMakeCancelling(state, state.list, cause)) return true
622629
}
630+
631+
/*
632+
* Filter out duplicates due to race in the following pattern:
633+
* T1: parent -> completion sequence
634+
* T2: child -> set final state -> signal with final exception to the parent
635+
*/
636+
is CompletedExceptionally -> return state.cause === cause
623637
else -> { // is inactive
624638
return false
625639
}
@@ -694,6 +708,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
694708
val cancelled = (state as? Finishing)?.cancelled ?: (proposedUpdate as? Cancelled)
695709
val completing = Finishing(list, cancelled, true)
696710
if (_state.compareAndSet(state, completing)) {
711+
(state as? Finishing)?.transferExceptions(completing)
697712
if (state !is Finishing) onFinishingInternal(proposedUpdate)
698713
if (child != null && tryWaitForChild(child, proposedUpdate))
699714
return COMPLETING_WAITING_CHILDREN
@@ -802,8 +817,8 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
802817
internal open fun onFinishingInternal(update: Any?) {}
803818

804819
/**
805-
* Method which is invoked once Job becomes `Cancelled`. It's guaranteed that at the moment
806-
* of invocation the job and all its children are complete
820+
* Method which is invoked once Job becomes [Cancelled] or [CompletedExceptionally].
821+
* It's guaranteed that at the moment of invocation the job and all its children are complete
807822
*/
808823
internal open fun handleJobException(exception: Throwable) {}
809824

@@ -875,6 +890,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
875890
}
876891
}
877892

893+
// guarded by `synchronized(this)`
878894
fun addLocked(exception: Throwable) {
879895
// Cannot be null at this point here
880896
when (_exceptionsHolder) {
@@ -901,6 +917,18 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
901917
_exceptionsHolder = null
902918
}
903919

920+
fun transferExceptions(to: Finishing) {
921+
synchronized(this) {
922+
synchronized(to) {
923+
val holder = _exceptionsHolder
924+
when(holder) {
925+
is Throwable -> to.addLocked(holder)
926+
is List<*> -> holder.forEach { to.addLocked(it as Throwable) }
927+
}
928+
seal()
929+
}
930+
}
931+
}
904932
}
905933

906934
private val Incomplete.isCancelling: Boolean

‎common/kotlinx-coroutines-core-common/src/channels/Broadcast.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ private open class BroadcastCoroutine<E>(
101101
else -> _channel.close(cause) // producer coroutine has completed -- close channel
102102
}
103103
if (!processed && cause != null)
104-
handleCoroutineException(context, cause)
104+
handleCoroutineException(context, cause, this)
105105
}
106106
}
107107

‎common/kotlinx-coroutines-core-common/src/channels/Produce.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,6 @@ private class ProducerCoroutine<E>(
124124
else -> _channel.close(cause) // producer coroutine has completed -- close channel
125125
}
126126
if (!processed && cause != null)
127-
handleCoroutineException(context, cause)
127+
handleCoroutineException(context, cause, this)
128128
}
129129
}

‎core/kotlinx-coroutines-core/src/channels/Actor.kt‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -160,12 +160,10 @@ private open class ActorCoroutine<E>(
160160
) : ChannelCoroutine<E>(parentContext, channel, active), ActorScope<E>, ActorJob<E> {
161161
override fun onCancellation(cause: Throwable?) {
162162
_channel.cancel(cause)
163-
// Always propagate the exception, don't wait for actor senders
164-
if (cause != null) handleCoroutineException(context, cause)
165163
}
166164

167165
override fun handleJobException(exception: Throwable) {
168-
handleCoroutineException(context, exception)
166+
handleCoroutineException(context, exception, this)
169167
}
170168
}
171169

‎core/kotlinx-coroutines-core/src/Annotations.kt‎ renamed to ‎core/kotlinx-coroutines-core/src/internalAnnotations/Annotations.kt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ package kotlinx.coroutines.experimental.internalAnnotations
77
@Suppress("ACTUAL_WITHOUT_EXPECT")
88
internal actual typealias JvmName = kotlin.jvm.JvmName
99

10+
@Suppress("ACTUAL_WITHOUT_EXPECT")
11+
internal actual typealias JvmOverloads = kotlin.jvm.JvmOverloads
12+
1013
@Suppress("ACTUAL_WITHOUT_EXPECT")
1114
internal actual typealias JvmMultifileClass = kotlin.jvm.JvmMultifileClass
1215

0 commit comments

Comments
 (0)