Skip to content

Commit c581112

Browse files
dkhalanskyjbdkhalanskyjb
committed
Try to replicate the old behavior
1 parent b6da0f1 commit c581112

File tree

3 files changed

+57
-37
lines changed

3 files changed

+57
-37
lines changed

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

+52-32
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,9 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
127127
// Used by the IDEA debugger via reflection and must be kept binary-compatible, see KTIJ-24102
128128
private val _state = atomic<Any?>(if (active) EMPTY_ACTIVE else EMPTY_NEW)
129129

130+
/** `true` means that the Job is cancelling and shouldn't accept [invokeOnCompletion] with `onCancelling = true` */
131+
private val onCancellingHandlersNotAccepted = atomic(false)
132+
130133
private val _parentHandle = atomic<ChildHandle?>(null)
131134
internal var parentHandle: ChildHandle?
132135
get() = _parentHandle.value
@@ -235,6 +238,8 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
235238
if (!wasCancelling) onCancelling(finalException)
236239
onCompletionInternal(finalState)
237240
// Then CAS to completed state -> it must succeed
241+
// forbid any new children
242+
onCancellingHandlersNotAccepted.value = true
238243
val casSuccess = _state.compareAndSet(state, finalState.boxIncomplete())
239244
assert { casSuccess }
240245
// And process all post-completion actions
@@ -326,7 +331,7 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
326331
}
327332

328333
private fun notifyCancelling(list: NodeList, cause: Throwable) {
329-
// first cancel our own children
334+
// then cancel our own children
330335
onCancelling(cause)
331336
notifyHandlers<JobCancellingNode>(list, cause)
332337
// then cancel parent
@@ -359,8 +364,10 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
359364
return parent.childCancelled(cause) || isCancellation
360365
}
361366

362-
private fun NodeList.notifyCompletion(cause: Throwable?) =
367+
private fun NodeList.notifyCompletion(cause: Throwable?) {
368+
close()
363369
notifyHandlers<JobNode>(this, cause)
370+
}
364371

365372
private inline fun <reified T: JobNode> notifyHandlers(list: NodeList, cause: Throwable?) {
366373
var exception: Throwable? = null
@@ -458,52 +465,63 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
458465
// for user-defined handlers it allocates a JobNode object that we might not need, but this is Ok.
459466
val node: JobNode = makeNode(handler, onCancelling)
460467
loopOnState { state ->
461-
when (state) {
462-
is Empty -> { // EMPTY_X state -- no completion handlers
468+
when {
469+
state !is Incomplete || onCancelling && onCancellingHandlersNotAccepted.value -> {
470+
if (invokeImmediately) {
471+
val exception = when (state) {
472+
is CompletedExceptionally -> state.cause
473+
is Finishing -> state.rootCause
474+
else -> null
475+
}
476+
// :KLUDGE: We have to invoke a handler in platform-specific way via `invokeIt` extension,
477+
// because we play type tricks on Kotlin/JS and handler is not necessarily a function there
478+
handler.invokeIt(exception)
479+
}
480+
return NonDisposableHandle
481+
}
482+
state is Empty -> { // EMPTY_X state -- no completion handlers
463483
if (state.isActive) {
464484
// try move to SINGLE state
465485
if (_state.compareAndSet(state, node)) return node
466486
} else
467487
promoteEmptyToNodeList(state) // that way we can add listener for non-active coroutine
468488
}
469-
is Incomplete -> {
489+
else -> {
490+
// is Incomplete
470491
val list = state.list
471492
if (list == null) { // SINGLE/SINGLE+
472493
promoteSingleToNodeList(state as JobNode)
473494
} else {
474-
var rootCause: Throwable? = null
475-
var handle: DisposableHandle = NonDisposableHandle
476-
if (onCancelling && state is Finishing) {
477-
synchronized(state) {
478-
// check if we are installing cancellation handler on job that is being cancelled
479-
rootCause = state.rootCause // != null if cancelling job
480-
// We add node to the list in two cases --- either the job is not being cancelled
481-
// or we are adding a child to a coroutine that is not completing yet
482-
if (rootCause == null || handler.isHandlerOf<ChildHandleNode>() && !state.isCompleting) {
483-
// Note: add node the list while holding lock on state (make sure it cannot change)
484-
if (!addLastAtomic(state, list, node)) return@loopOnState // retry
485-
// just return node if we don't have to invoke handler (not cancelling yet)
486-
if (rootCause == null) return node
487-
// otherwise handler is invoked immediately out of the synchronized section & handle returned
488-
handle = node
495+
if (onCancelling) {
496+
if (state is Finishing) {
497+
val rootCause: Throwable?
498+
val handle: DisposableHandle
499+
synchronized(state) {
500+
// check if we are installing cancellation handler on job that is being cancelled
501+
rootCause = state.rootCause // != null if cancelling job
502+
// We add node to the list in two cases --- either the job is not being cancelled
503+
// or we are adding a child to a coroutine that is not completing yet
504+
if (rootCause == null || handler.isHandlerOf<ChildHandleNode>() && !state.isCompleting) {
505+
// Note: add node the list while holding lock on state (make sure it cannot change)
506+
if (!list.addLastIf(node) { !onCancellingHandlersNotAccepted.value }) return@loopOnState // retry
507+
// just return node if we don't have to invoke handler (not cancelling yet)
508+
if (rootCause == null) return node
509+
// otherwise handler is invoked immediately out of the synchronized section & handle returned
510+
handle = node
511+
} else {
512+
handle = NonDisposableHandle
513+
}
489514
}
515+
// Note: attachChild uses invokeImmediately, so it gets invoked when adding to cancelled job
516+
if (invokeImmediately) handler.invokeIt(rootCause)
517+
return handle
490518
}
491-
}
492-
if (rootCause != null) {
493-
// Note: attachChild uses invokeImmediately, so it gets invoked when adding to cancelled job
494-
if (invokeImmediately) handler.invokeIt(rootCause)
495-
return handle
519+
if (list.addLastIf(node) { !onCancellingHandlersNotAccepted.value }) return node
496520
} else {
497-
if (addLastAtomic(state, list, node)) return node
521+
if (list.addLast(node)) return node
498522
}
499523
}
500524
}
501-
else -> { // is complete
502-
// :KLUDGE: We have to invoke a handler in platform-specific way via `invokeIt` extension,
503-
// because we play type tricks on Kotlin/JS and handler is not necessarily a function there
504-
if (invokeImmediately) handler.invokeIt((state as? CompletedExceptionally)?.cause)
505-
return NonDisposableHandle
506-
}
507525
}
508526
}
509527
}
@@ -880,6 +898,8 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
880898
val finishing = state as? Finishing ?: Finishing(list, false, null)
881899
// must synchronize updates to finishing state
882900
var notifyRootCause: Throwable? = null
901+
// forbid any new children
902+
onCancellingHandlersNotAccepted.value = true
883903
synchronized(finishing) {
884904
// check if this state is already completing
885905
if (finishing.isCompleting) return COMPLETING_ALREADY

‎kotlinx-coroutines-core/concurrent/src/internal/LockFreeLinkedList.kt

+4-4
Original file line numberDiff line numberDiff line change
@@ -125,15 +125,15 @@ public actual open class LockFreeLinkedListNode {
125125
public actual fun addLast(node: Node): Boolean {
126126
while (true) { // lock-free loop on prev.next
127127
val currentPrev = prevNode
128-
if (currentPrev is CLOSED) return false
128+
if (currentPrev is LIST_CLOSED) return false
129129
if (currentPrev.addNext(node, this)) return true
130130
}
131131
}
132132

133133
/**
134134
* Forbids adding new items to this list.
135135
*/
136-
public actual fun close() { addLast(CLOSED()) }
136+
public actual fun close() { addLast(LIST_CLOSED()) }
137137

138138
/**
139139
* Adds last item to this list atomically if the [condition] is true.
@@ -142,7 +142,7 @@ public actual open class LockFreeLinkedListNode {
142142
val condAdd = makeCondAddOp(node, condition)
143143
while (true) { // lock-free loop on prev.next
144144
val prev = prevNode // sentinel node is never removed, so prev is always defined
145-
if (prev is CLOSED) return false
145+
if (prev is LIST_CLOSED) return false
146146
when (prev.tryCondAddNext(node, this, condAdd)) {
147147
SUCCESS -> return true
148148
FAILURE -> return false
@@ -372,4 +372,4 @@ public actual open class LockFreeLinkedListHead : LockFreeLinkedListNode() {
372372
}
373373

374374
// not private due to what seems to be a compiler bug
375-
internal class CLOSED: LockFreeLinkedListNode()
375+
internal class LIST_CLOSED: LockFreeLinkedListNode()

‎kotlinx-coroutines-core/jvm/test/MemoryFootprintTest.kt

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import kotlin.test.*
1212
class MemoryFootprintTest : TestBase(true) {
1313

1414
@Test
15-
fun testJobLayout() = assertLayout(Job().javaClass, 24)
15+
fun testJobLayout() = assertLayout(Job().javaClass, 32)
1616

1717
@Test
1818
fun testCancellableContinuationFootprint() = assertLayout(CancellableContinuationImpl::class.java, 48)

0 commit comments

Comments
 (0)