Review fixes

Author: Nikita Koval

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

@@ -1268,10 +1268,8 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
     ) : JobNode() {
         override fun invoke(cause: Throwable?) {
             val state = this@JobSupport.state
-            if (state !is Incomplete) {
-                val result = if (state is CompletedExceptionally) state else state.unboxState()
-                select.trySelect(this@JobSupport, result)
-            }
+            val result = if (state is CompletedExceptionally) state else state.unboxState()
+            select.trySelect(this@JobSupport, result)
         }
     }
 }

kotlinx-coroutines-core/common/src/selects/OnTimeout.kt

@@ -7,6 +7,29 @@ package kotlinx.coroutines.selects
 import kotlinx.coroutines.*
 import kotlin.time.*
 
+/**
+ * Clause that selects the given [block] after a specified timeout passes.
+ * If timeout is negative or zero, [block] is selected immediately.
+ *
+ * **Note: This is an experimental api.** It may be replaced with light-weight timer/timeout channels in the future.
+ *
+ * @param timeMillis timeout time in milliseconds.
+ */
+@ExperimentalCoroutinesApi
+@Suppress("EXTENSION_SHADOWED_BY_MEMBER")
+public fun <R> SelectBuilder<R>.onTimeout(timeMillis: Long, block: suspend () -> R): Unit =
+    OnTimeout(timeMillis).selectClause.invoke(block)
+
+/**
+ * Clause that selects the given [block] after the specified [timeout] passes.
+ * If timeout is negative or zero, [block] is selected immediately.
+ *
+ * **Note: This is an experimental api.** It may be replaced with light-weight timer/timeout channels in the future.
+ */
+@ExperimentalCoroutinesApi
+public fun <R> SelectBuilder<R>.onTimeout(timeout: Duration, block: suspend () -> R): Unit =
+    onTimeout(timeout.toDelayMillis(), block)
+
 /**
  * We implement [SelectBuilder.onTimeout] as a clause, so each invocation creates
  * an instance of [OnTimeout] that specifies the registration part according to
@@ -15,6 +38,12 @@ import kotlin.time.*
 private class OnTimeout(
     private val timeMillis: Long
 ) {
+    val selectClause: SelectClause0
+        get() = SelectClause0Impl(
+            clauseObject = this@OnTimeout,
+            regFunc = OnTimeout::register as RegistrationFunction
+        )
+
     private fun register(select: SelectInstance<*>, ignoredParam: Any?) {
         // Should this clause complete immediately?
         if (timeMillis <= 0) {
@@ -31,33 +60,4 @@ private class OnTimeout(
         // Do not forget to clean-up when this `select` is completed or cancelled.
         select.disposeOnCompletion(disposableHandle)
     }
-
-    val selectClause: SelectClause0
-        get() = SelectClause0Impl(
-            clauseObject = this@OnTimeout,
-            regFunc = OnTimeout::register as RegistrationFunction
-        )
 }
-
-/**
- * Clause that selects the given [block] after a specified timeout passes.
- * If timeout is negative or zero, [block] is selected immediately.
- *
- * **Note: This is an experimental api.** It may be replaced with light-weight timer/timeout channels in the future.
- *
- * @param timeMillis timeout time in milliseconds.
- */
-@ExperimentalCoroutinesApi
-@Suppress("EXTENSION_SHADOWED_BY_MEMBER")
-public fun <R> SelectBuilder<R>.onTimeout(timeMillis: Long, block: suspend () -> R): Unit =
-    OnTimeout(timeMillis).selectClause.invoke(block)
-
-/**
- * Clause that selects the given [block] after the specified [timeout] passes.
- * If timeout is negative or zero, [block] is selected immediately.
- *
- * **Note: This is an experimental api.** It may be replaced with light-weight timer/timeout channels in the future.
- */
-@ExperimentalCoroutinesApi
-public fun <R> SelectBuilder<R>.onTimeout(timeout: Duration, block: suspend () -> R): Unit =
-    onTimeout(timeout.toDelayMillis(), block)

kotlinx-coroutines-core/common/src/selects/Select.kt

@@ -150,14 +150,14 @@ public typealias ProcessResultFunction = (clauseObject: Any, param: Any?, clause
  * cancellation while dispatching. Unfortunately, we cannot pass this function only in [SelectInstance.trySelect],
  * as [SelectInstance.selectInRegistrationPhase] can be called when the coroutine is already cancelled.
  */
+@InternalCoroutinesApi
 public typealias OnCancellationConstructor = (select: SelectInstance<*>, param: Any?, internalResult: Any?) -> (Throwable) -> Unit
 
 /**
  * Clause for [select] expression without additional parameters that does not select any value.
  */
 public interface SelectClause0 : SelectClause
-@InternalCoroutinesApi
-public class SelectClause0Impl(
+internal class SelectClause0Impl(
     override val clauseObject: Any,
     override val regFunc: RegistrationFunction,
     override val onCancellationConstructor: OnCancellationConstructor? = null
@@ -171,8 +171,7 @@ private val DUMMY_PROCESS_RESULT_FUNCTION: ProcessResultFunction = { _, _, _ ->
  * Clause for [select] expression without additional parameters that selects value of type [Q].
  */
 public interface SelectClause1<out Q> : SelectClause
-@InternalCoroutinesApi
-public class SelectClause1Impl<Q>(
+internal class SelectClause1Impl<Q>(
     override val clauseObject: Any,
     override val regFunc: RegistrationFunction,
     override val processResFunc: ProcessResultFunction,
@@ -183,8 +182,7 @@ public class SelectClause1Impl<Q>(
  * Clause for [select] expression with additional parameter of type [P] that selects value of type [Q].
  */
 public interface SelectClause2<in P, out Q> : SelectClause
-@InternalCoroutinesApi
-public class SelectClause2Impl<P, Q>(
+internal class SelectClause2Impl<P, Q>(
     override val clauseObject: Any,
     override val regFunc: RegistrationFunction,
     override val processResFunc: ProcessResultFunction,
@@ -348,7 +346,7 @@ internal open class SelectImplementation<R> constructor(
             it === STATE_REG || it is List<*>
         }
     /**
-     * Returns `true` if this `select` is already selected or cancelled;
+     * Returns `true` if this `select` is already selected;
      * thus, other parties are bound to fail when making a rendezvous with it.
      */
     private val isSelected
@@ -437,8 +435,9 @@ internal open class SelectImplementation<R> constructor(
      * updates the state to this clause reference.
      */
     protected fun ClauseData<R>.register(reregister: Boolean = false) {
+        assert { state.value !is Cancelled }
         // Is there already selected clause?
-        if (state.value.let { it is ClauseData<*> || it is Cancelled }) return
+        if (state.value.let { it is ClauseData<*> }) return
         // For new clauses, check that there does not exist
         // another clause with the same object.
         if (!reregister) checkClauseObject(clauseObject)
@@ -459,13 +458,7 @@ internal open class SelectImplementation<R> constructor(
         } else {
             // This clause has been selected!
             // Update the state correspondingly.
-            state.update {
-                if (it is Cancelled) {
-                    createOnCancellationAction(this@SelectImplementation, internalResult)?.invoke(it.cause)
-                    return
-                }
-                this
-            }
+            state.value = this
         }
     }
 
@@ -529,8 +522,6 @@ internal open class SelectImplementation<R> constructor(
                     cont.resume(Unit, curState.createOnCancellationAction(this, internalResult))
                     return@sc
                 }
-                // Cancelled
-                curState is Cancelled -> return@sc
                 // This `select` cannot be in any other state.
                 else -> error("unexpected state: $curState")
             }
@@ -787,6 +778,8 @@ private val NO_RESULT = Symbol("NO_RESULT")
 // We use this marker parameter objects to distinguish
 // SelectClause[0,1,2] and invoke the user-specified block correctly.
 @SharedImmutable
+@JvmField
 internal val PARAM_CLAUSE_0 = Symbol("PARAM_CLAUSE_0")
 @SharedImmutable
-internal val PARAM_CLAUSE_1 = Symbol("PARAM_CLAUSE_1")
+@JvmField
+internal val PARAM_CLAUSE_1 = Symbol("PARAM_CLAUSE_1")

kotlinx-coroutines-core/common/src/selects/SelectOld.kt

@@ -102,4 +102,4 @@ private fun <T> CancellableContinuation<T>.resumeUndispatched(result: T) {
     } else {
         resume(result)
     }
-}
+}

kotlinx-coroutines-core/common/src/selects/SelectUnbiased.kt

@@ -65,4 +65,4 @@ internal open class UnbiasedSelectImplementation<R>(context: CoroutineContext) :
     } finally {
         clausesToRegister.clear()
     }
-}
+}

kotlinx-coroutines-core/common/src/sync/Semaphore.kt

@@ -305,7 +305,7 @@ internal open class SemaphoreImpl(private val permits: Int, acquiredPermits: Int
         // If the cell already has PERMIT from tryResumeNextFromQueue, try to grab it
         if (segment.cas(i, PERMIT, TAKEN)) { // took permit thus eliminating acquire/release pair
             /// This continuation is not yet published, but still can be cancelled via outer job
-            when(waiter) {
+            when (waiter) {
                 is CancellableContinuation<*> -> {
                     waiter as CancellableContinuation<Unit>
                     waiter.resume(Unit, onCancellationRelease)

kotlinx-coroutines-core/common/test/selects/SelectOldTest.kt

@@ -121,4 +121,4 @@ class SelectOldTest : TestBase() {
         finish(4)
         assertEquals("OK", res)
     }
-}
+}

kotlinx-coroutines-core/concurrent/test/selects/SelectMutexStressTest.kt

@@ -30,4 +30,4 @@ class SelectMutexStressTest : TestBase() {
         assertTrue(mutex.isLocked)
         finish(n + 2)
     }
-}
+}

kotlinx-coroutines-core/jvm/test/MutexStressTest.kt renamed to kotlinx-coroutines-core/jvm/test/MutexCancellationStressTest.kt

@@ -9,7 +9,7 @@ import kotlinx.coroutines.sync.*
 import org.junit.*
 import java.util.concurrent.*
 
-class MutexStressTest : TestBase() {
+class MutexCancellationStressTest : TestBase() {
     @Test
     fun testStressCancellationDoesNotBreakMutex() = runTest {
         val mutex = Mutex()
@@ -72,4 +72,4 @@ class MutexStressTest : TestBase() {
         check(counter == counterLocal.sumOf { it.value })
         dispatcher.close()
     }
-}
+}

kotlinx-coroutines-core/jvm/test/lincheck/MutexLincheckTest.kt

@@ -41,4 +41,4 @@ class MutexLincheckTest : AbstractLincheckTest() {
     override fun extractState() = (1..2).map { mutex.holdsLock(it) } + mutex.isLocked
 
     private val Int.asOwnerOrNull get() = if (this == 0) null else this
-}
+}