Fix a spinlock in invokeOnCompletion with onCancelling = true

Before this change, when children were prohibited from adding
themselves to the list, cancellation handlers were also prohibited
from doing so. This was plain incorrect, because a list that's
closed for new children could still get cancelled later, and also
because being closed for new children happened before advancing the
state to the final one.

Author: dkhalanskyjb

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

@@ -353,7 +353,7 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
     }
 
     private fun NodeList.notifyCompletion(cause: Throwable?) {
-        close(LIST_MAX_PERMISSION)
+        close(LIST_ON_COMPLETION_PERMISSION)
         notifyHandlers<JobNode>(this, cause)
     }
 
@@ -466,13 +466,13 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
             if (onCancelling) {
                 val rootCause = (state as? Finishing)?.let { synchronized(it) { it.rootCause } }
                 if (rootCause == null) {
-                    list.addLast(node, LIST_CANCELLATION_PERMISSION)
+                    list.addLast(node, LIST_CANCELLATION_PERMISSION or LIST_ON_COMPLETION_PERMISSION)
                 } else {
                     if (invokeImmediately) handler.invoke(rootCause)
                     return NonDisposableHandle
                 }
             } else {
-                list.addLast(node, LIST_MAX_PERMISSION)
+                list.addLast(node, LIST_ON_COMPLETION_PERMISSION)
             }
         }
         when {
@@ -988,14 +988,20 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
         val node = ChildHandleNode(child).also { it.job = this }
         val added = tryPutNodeIntoList(node) { _, list ->
             // First, try to add a child along the cancellation handlers
-            val addedBeforeCancellation = list.addLast(node, LIST_CANCELLATION_PERMISSION)
+            val addedBeforeCancellation = list.addLast(
+                node,
+                LIST_ON_COMPLETION_PERMISSION or LIST_CHILD_PERMISSION or LIST_CANCELLATION_PERMISSION
+            )
             if (addedBeforeCancellation) {
                 // The child managed to be added before the parent started to cancel or complete. Success.
                 true
             } else {
                 // Either cancellation or completion already happened, the child was not added.
                 // Now we need to try adding it for completion.
-                val addedBeforeCompletion = list.addLast(node, LIST_CHILD_PERMISSION)
+                val addedBeforeCompletion = list.addLast(
+                    node,
+                    LIST_CHILD_PERMISSION or LIST_ON_COMPLETION_PERMISSION
+                )
                 // Whether or not we managed to add the child before the parent completed, we need to investigate:
                 // why didn't we manage to add it before cancellation?
                 // If it's because cancellation happened in the meantime, we need to notify the child.
@@ -1353,9 +1359,10 @@ private val SEALED = Symbol("SEALED")
 private val EMPTY_NEW = Empty(false)
 private val EMPTY_ACTIVE = Empty(true)
 
-private const val LIST_MAX_PERMISSION = Int.MAX_VALUE
-private const val LIST_CHILD_PERMISSION = 1
-private const val LIST_CANCELLATION_PERMISSION = 0
+// bit mask
+private const val LIST_ON_COMPLETION_PERMISSION = 1
+private const val LIST_CHILD_PERMISSION = 2
+private const val LIST_CANCELLATION_PERMISSION = 4
 
 private class Empty(override val isActive: Boolean) : Incomplete {
     override val list: NodeList? get() = null

kotlinx-coroutines-core/common/src/internal/LockFreeLinkedList.common.kt

@@ -7,14 +7,15 @@ public expect open class LockFreeLinkedListNode() {
     public val isRemoved: Boolean
     public val nextNode: LockFreeLinkedListNode
     public val prevNode: LockFreeLinkedListNode
-    public fun addLast(node: LockFreeLinkedListNode, clearanceLevel: Int): Boolean
+    public fun addLast(node: LockFreeLinkedListNode, permissionsBitmask: Int): Boolean
     public fun addOneIfEmpty(node: LockFreeLinkedListNode): Boolean
     public open fun remove(): Boolean
 
     /**
-     * Closes the list for [maxForbidden] and all numbers below.
+     * Closes the list for anything that requests the permission [forbiddenElementsBit].
+     * Only a single permission can be forbidden at a time, but this isn't checked.
      */
-    public fun close(maxForbidden: Int)
+    public fun close(forbiddenElementsBit: Int)
 
 }
 

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

@@ -80,12 +80,13 @@ public actual open class LockFreeLinkedListNode {
     /**
      * Adds last item to this list. Returns `false` if the list is closed.
      */
-    public actual fun addLast(node: Node, clearanceLevel: Int): Boolean {
+    public actual fun addLast(node: Node, permissionsBitmask: Int): Boolean {
         while (true) { // lock-free loop on prev.next
             val currentPrev = prevNode
             return when {
                 currentPrev is ListClosed ->
-                    currentPrev.maxForbidden < clearanceLevel && currentPrev.addLast(node, clearanceLevel)
+                    currentPrev.forbiddenElementsBitmask and permissionsBitmask == 0 &&
+                        currentPrev.addLast(node, permissionsBitmask)
                 currentPrev.addNext(node, this) -> true
                 else -> continue
             }
@@ -95,7 +96,9 @@ public actual open class LockFreeLinkedListNode {
     /**
      * Forbids adding new items to this list.
      */
-    public actual fun close(maxForbidden: Int) { addLast(ListClosed(maxForbidden), maxForbidden) }
+    public actual fun close(forbiddenElementsBit: Int) {
+        addLast(ListClosed(forbiddenElementsBit), forbiddenElementsBit)
+    }
 
     /**
      * Given:
@@ -285,4 +288,4 @@ public actual open class LockFreeLinkedListHead : LockFreeLinkedListNode() {
     override val isRemoved: Boolean get() = false
 }
 
-private class ListClosed(val maxForbidden: Int): LockFreeLinkedListNode()
+private class ListClosed(val forbiddenElementsBitmask: Int): LockFreeLinkedListNode()

kotlinx-coroutines-core/jsAndWasmShared/src/internal/LinkedList.kt

@@ -22,8 +22,9 @@ public open class LinkedListNode : DisposableHandle {
     public inline val prevNode get() = _prev
     public inline val isRemoved get() = _removed
 
-    public fun addLast(node: Node, clearanceLevel: Int): Boolean = when (val prev = this._prev) {
-        is ListClosed -> prev.maxForbidden < clearanceLevel && prev.addLast(node, clearanceLevel)
+    public fun addLast(node: Node, permissionsBitmask: Int): Boolean = when (val prev = this._prev) {
+        is ListClosed ->
+            prev.forbiddenElementsBitmask and permissionsBitmask == 0 && prev.addLast(node, permissionsBitmask)
         else -> {
             node._next = this
             node._prev = prev
@@ -33,8 +34,8 @@ public open class LinkedListNode : DisposableHandle {
         }
     }
 
-    public fun close(maxForbidden: Int) {
-        addLast(ListClosed(maxForbidden), maxForbidden)
+    public fun close(forbiddenElementsBit: Int) {
+        addLast(ListClosed(forbiddenElementsBit), forbiddenElementsBit)
     }
 
     /*
@@ -83,4 +84,4 @@ public open class LinkedListHead : LinkedListNode() {
     public final override fun remove(): Nothing = throw UnsupportedOperationException()
 }
 
-private class ListClosed(val maxForbidden: Int): LinkedListNode()
+private class ListClosed(val forbiddenElementsBitmask: Int): LinkedListNode()