Remove DCSS (#4053)

The internal implementation of `JobSupport` no longer uses
the Double-Compare Single-Swap algorithm.
Instead, the signal for the list to stop accepting this or that
kind of elements is provided explicitly.
In addition to simplifying the implementation somewhat,
this change allowed us to more precisely define when child
nodes should stop being accepted into the list, fixing a bug.

Fixes #3893

Additionally, new stress tests are added to ensure the correct
behavior.

Author: dkhalanskyjb

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

@@ -2,22 +2,22 @@
 
 package kotlinx.coroutines.internal
 
-import kotlinx.coroutines.*
-import kotlin.jvm.*
-
 /** @suppress **This is unstable API and it is subject to change.** */
 public expect open class LockFreeLinkedListNode() {
     public val isRemoved: Boolean
     public val nextNode: LockFreeLinkedListNode
     public val prevNode: LockFreeLinkedListNode
+    public fun addLast(node: LockFreeLinkedListNode, permissionsBitmask: Int): Boolean
     public fun addOneIfEmpty(node: LockFreeLinkedListNode): Boolean
-    public inline fun addLastIf(node: LockFreeLinkedListNode, crossinline condition: () -> Boolean): Boolean
     public open fun remove(): Boolean
 
+    /**
+     * 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(forbiddenElementsBit: Int)
 }
 
-internal fun LockFreeLinkedListNode.addLast(node: LockFreeLinkedListNode) = addLastIf(node) { true }
-
 /** @suppress **This is unstable API and it is subject to change.** */
 public expect open class LockFreeLinkedListHead() : LockFreeLinkedListNode {
     public inline fun forEach(block: (LockFreeLinkedListNode) -> Unit)

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

@@ -174,6 +174,20 @@ class JobTest : TestBase() {
         finish(4)
     }
 
+    @Test
+    fun testInvokeOnCancellingFiringOnNormalExit() = runTest {
+        val job = launch {
+            expect(2)
+        }
+        job.invokeOnCompletion(onCancelling = true) {
+            assertNull(it)
+            expect(3)
+        }
+        expect(1)
+        job.join()
+        finish(4)
+    }
+
     @Test
     fun testOverriddenParent() = runTest {
         val parent = Job()

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

@@ -8,18 +8,6 @@ import kotlin.jvm.*
 
 private typealias Node = LockFreeLinkedListNode
 
-@PublishedApi
-internal const val UNDECIDED: Int = 0
-
-@PublishedApi
-internal const val SUCCESS: Int = 1
-
-@PublishedApi
-internal const val FAILURE: Int = 2
-
-@PublishedApi
-internal val CONDITION_FALSE: Any = Symbol("CONDITION_FALSE")
-
 /**
  * Doubly-linked concurrent list node with remove support.
  * Based on paper
@@ -49,37 +37,10 @@ public actual open class LockFreeLinkedListNode {
     private fun removed(): Removed =
         _removedRef.value ?: Removed(this).also { _removedRef.lazySet(it) }
 
-    @PublishedApi
-    internal abstract class CondAddOp(
-        @JvmField val newNode: Node
-    ) : AtomicOp<Node>() {
-        @JvmField var oldNext: Node? = null
-
-        override fun complete(affected: Node, failure: Any?) {
-            val success = failure == null
-            val update = if (success) newNode else oldNext
-            if (update != null && affected._next.compareAndSet( this, update)) {
-                // only the thread the makes this update actually finishes add operation
-                if (success) newNode.finishAdd(oldNext!!)
-            }
-        }
-    }
-
-    @PublishedApi
-    internal inline fun makeCondAddOp(node: Node, crossinline condition: () -> Boolean): CondAddOp =
-        object : CondAddOp(node) {
-            override fun prepare(affected: Node): Any? = if (condition()) null else CONDITION_FALSE
-        }
-
     public actual open val isRemoved: Boolean get() = next is Removed
 
     // LINEARIZABLE. Returns Node | Removed
-    public val next: Any get() {
-        _next.loop { next ->
-            if (next !is OpDescriptor) return next
-            next.perform(this)
-        }
-    }
+    public val next: Any get() = _next.value
 
     // LINEARIZABLE. Returns next non-removed Node
     public actual val nextNode: Node get() =
@@ -117,20 +78,27 @@ public actual open class LockFreeLinkedListNode {
     // ------ addLastXXX ------
 
     /**
-     * Adds last item to this list atomically if the [condition] is true.
+     * Adds last item to this list. Returns `false` if the list is closed.
      */
-    public actual inline fun addLastIf(node: Node, crossinline condition: () -> Boolean): Boolean {
-        val condAdd = makeCondAddOp(node, condition)
+    public actual fun addLast(node: Node, permissionsBitmask: Int): Boolean {
         while (true) { // lock-free loop on prev.next
-            val prev = prevNode // sentinel node is never removed, so prev is always defined
-            when (prev.tryCondAddNext(node, this, condAdd)) {
-                SUCCESS -> return true
-                FAILURE -> return false
+            val currentPrev = prevNode
+            return when {
+                currentPrev is ListClosed ->
+                    currentPrev.forbiddenElementsBitmask and permissionsBitmask == 0 &&
+                        currentPrev.addLast(node, permissionsBitmask)
+                currentPrev.addNext(node, this) -> true
+                else -> continue
             }
         }
     }
 
-    // ------ addXXX util ------
+    /**
+     * Forbids adding new items to this list.
+     */
+    public actual fun close(forbiddenElementsBit: Int) {
+        addLast(ListClosed(forbiddenElementsBit), forbiddenElementsBit)
+    }
 
     /**
      * Given:
@@ -165,17 +133,6 @@ public actual open class LockFreeLinkedListNode {
         return true
     }
 
-    // returns UNDECIDED, SUCCESS or FAILURE
-    @PublishedApi
-    internal fun tryCondAddNext(node: Node, next: Node, condAdd: CondAddOp): Int {
-        node._prev.lazySet(this)
-        node._next.lazySet(next)
-        condAdd.oldNext = next
-        if (!_next.compareAndSet(next, condAdd)) return UNDECIDED
-        // added operation successfully (linearized) -- complete it & fixup the list
-        return if (condAdd.perform(this) == null) SUCCESS else FAILURE
-    }
-
     // ------ removeXXX ------
 
     /**
@@ -273,10 +230,6 @@ public actual open class LockFreeLinkedListNode {
                 }
                 // slow path when we need to help remove operations
                 this.isRemoved -> return null // nothing to do, this node was removed, bail out asap to save time
-                prevNext is OpDescriptor -> { // help & retry
-                    prevNext.perform(prev)
-                    return correctPrev() // retry from scratch
-                }
                 prevNext is Removed -> {
                     if (last !== null) {
                         // newly added (prev) node is already removed, correct last.next around it
@@ -332,3 +285,5 @@ public actual open class LockFreeLinkedListHead : LockFreeLinkedListNode() {
     // optimization: because head is never removed, we don't have to read _next.value to check these:
     override val isRemoved: Boolean get() = false
 }
+
+private class ListClosed(@JvmField val forbiddenElementsBitmask: Int): LockFreeLinkedListNode()

kotlinx-coroutines-core/common/test/JobTest.kt

@@ -14,13 +14,20 @@ public actual open class LockFreeLinkedListNode {
     inline actual val prevNode get() = _prev
     inline actual val isRemoved get() = _removed
 
-    @PublishedApi
-    internal fun addLast(node: Node) {
-        val prev = this._prev
-        node._next = this
-        node._prev = prev
-        prev._next = node
-        this._prev = node
+    public actual 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
+            prev._next = node
+            this._prev = node
+            true
+        }
+    }
+
+    public actual fun close(forbiddenElementsBit: Int) {
+        addLast(ListClosed(forbiddenElementsBit), forbiddenElementsBit)
     }
 
     /*
@@ -30,10 +37,6 @@ public actual open class LockFreeLinkedListNode {
      * invokes handler on remove
      */
     public actual open fun remove(): Boolean {
-        return removeImpl()
-    }
-
-    private fun removeImpl(): Boolean {
         if (_removed) return false
         val prev = this._prev
         val next = this._next
@@ -45,13 +48,7 @@ public actual open class LockFreeLinkedListNode {
 
     public actual fun addOneIfEmpty(node: Node): Boolean {
         if (_next !== this) return false
-        addLast(node)
-        return true
-    }
-
-    public actual inline fun addLastIf(node: Node, crossinline condition: () -> Boolean): Boolean {
-        if (!condition()) return false
-        addLast(node)
+        addLast(node, Int.MIN_VALUE)
         return true
     }
 }
@@ -72,3 +69,5 @@ public actual open class LockFreeLinkedListHead : Node() {
     // just a defensive programming -- makes sure that list head sentinel is never removed
     public actual final override fun remove(): Nothing = throw UnsupportedOperationException()
 }
+
+private class ListClosed(val forbiddenElementsBitmask: Int): LockFreeLinkedListNode()

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

@@ -12,13 +12,13 @@ class LinkedListTest {
     fun testSimpleAddLastRemove() {
         val list = LockFreeLinkedListHead()
         assertContents(list)
-        val n1 = IntNode(1).apply { list.addLast(this) }
+        val n1 = IntNode(1).apply { list.addLast(this, Int.MAX_VALUE) }
         assertContents(list, 1)
-        val n2 = IntNode(2).apply { list.addLast(this) }
+        val n2 = IntNode(2).apply { list.addLast(this, Int.MAX_VALUE) }
         assertContents(list, 1, 2)
-        val n3 = IntNode(3).apply { list.addLast(this) }
+        val n3 = IntNode(3).apply { list.addLast(this, Int.MAX_VALUE) }
         assertContents(list, 1, 2, 3)
-        val n4 = IntNode(4).apply { list.addLast(this) }
+        val n4 = IntNode(4).apply { list.addLast(this, Int.MAX_VALUE) }
         assertContents(list, 1, 2, 3, 4)
         assertTrue(n1.remove())
         assertContents(list, 2, 3, 4)