Replacing isCancelledWithoutCause with proper contract on exception, disabling stacktraces of JobCancellationException in production mode

Author: qwwdfsad

common/kotlinx-coroutines-core-common/src/main/kotlin/kotlinx/coroutines/experimental/AbstractCoroutine.kt

@@ -76,8 +76,8 @@ public abstract class AbstractCoroutine<in T>(
      * This function is invoked once when this coroutine is cancelled or is completed,
      * similarly to [invokeOnCompletion] with `onCancelling` set to `true`.
      *
-     * @param cause the cause that was passed to [Job.cancel] function or `null` if coroutine was cancelled
-     *              without cause or is completing normally.
+     * @param cause the cause that was passed to [Job.cancel] function, [JobCancellationException] if it was completed without cause
+     *        or `null` if coroutine is completing normally.
      */
     protected open fun onCancellation(cause: Throwable?) {}
 
@@ -98,7 +98,7 @@ public abstract class AbstractCoroutine<in T>(
     @Suppress("UNCHECKED_CAST")
     internal override fun onCompletionInternal(state: Any?, mode: Int) {
         if (state is CompletedExceptionally)
-            onCompletedExceptionally(state.exception)
+            onCompletedExceptionally(state.cause)
         else
             onCompleted(state as T)
     }

common/kotlinx-coroutines-core-common/src/main/kotlin/kotlinx/coroutines/experimental/CompletedExceptionally.kt

@@ -20,13 +20,13 @@ package kotlinx.coroutines.experimental
  * Class for an internal state of a job that had completed exceptionally, including cancellation.
  *
  * **Note: This class cannot be used outside of internal coroutines framework**.
+ * **Note: cannot be internal until we get rid of MutableDelegateContinuation in IO**
  *
  * @param cause the exceptional completion cause. It's either original exceptional cause
  *        or artificial JobCancellationException if no cause was provided
  * @suppress **This is unstable API and it is subject to change.**
  */
-// todo: make it internal
-public open class CompletedExceptionally(
+open class CompletedExceptionally(
     public val cause: Throwable
 ) {
     /**
@@ -36,13 +36,7 @@ public open class CompletedExceptionally(
     // todo: Remove exception usages
     public val exception: Throwable get() = cause // alias for backward compatibility
 
-    /**
-     * Returns true if the current exceptional reason is cancellation without cause
-     */
-    // todo: Remove it. Instead, classify based on `cause is CancellationException`
-    public open fun isCancelledWithoutCause() = false
-
-    override fun toString(): String = "$classSimpleName[$exception]"
+    override fun toString(): String = "$classSimpleName[$cause]"
 }
 
 /**
@@ -54,12 +48,7 @@ public open class CompletedExceptionally(
  * @param cause the exceptional completion cause. If `cause` is null, then a [JobCancellationException] is created.
  * @suppress **This is unstable API and it is subject to change.**
  */
-// todo: make it internal
-public class Cancelled(
+internal class Cancelled(
     private val job: Job,
     cause: Throwable?
-) : CompletedExceptionally(cause ?: JobCancellationException("Job was cancelled normally", null, job)) {
-
-    override fun isCancelledWithoutCause(): Boolean = cause is JobCancellationException && cause.job == job
-}
-
+) : CompletedExceptionally(cause ?: JobCancellationException("Job was cancelled normally", null, job))

common/kotlinx-coroutines-core-common/src/main/kotlin/kotlinx/coroutines/experimental/Job.kt

@@ -641,15 +641,16 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
     private fun isCorrespondinglyCancelled(cancelled: Cancelled, proposedUpdate: Any?): Boolean {
         if (proposedUpdate !is Cancelled) return false
         // NOTE: equality comparison of causes is performed here by design, see equals of JobCancellationException
-        return proposedUpdate.cause == cancelled.cause ||
-            proposedUpdate.cause is JobCancellationException && cancelled.isCancelledWithoutCause()
+        return proposedUpdate.cause == cancelled.cause || proposedUpdate.cause is JobCancellationException
     }
 
     private fun createCancelled(cancelled: Cancelled, proposedUpdate: Any?): Cancelled {
         if (proposedUpdate !is CompletedExceptionally) return cancelled // not exception -- just use original cancelled
-        val exception = proposedUpdate.exception
-        if (cancelled.exception == exception) return cancelled // that is the cancelled we need already!
-        if (!cancelled.isCancelledWithoutCause()) {
+        val exception = proposedUpdate.cause
+        if (cancelled.cause == exception) return cancelled // that is the cancelled we need already!
+
+        // Do not spam with JCE in suppressed exceptions
+        if (cancelled.cause !is JobCancellationException) {
             exception.addSuppressedThrowable(cancelled.cause)
         }
         return Cancelled(this, exception)
@@ -752,11 +753,11 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
         val state = this.state
         return when {
             state is Finishing && state.cancelled != null ->
-                state.cancelled.exception.toCancellationException("Job is being cancelled")
+                state.cancelled.cause.toCancellationException("Job is being cancelled")
             state is Incomplete ->
                 error("Job was not completed or cancelled yet: $this")
             state is CompletedExceptionally ->
-                state.exception.toCancellationException("Job has failed")
+                state.cause.toCancellationException("Job has failed")
             else -> JobCancellationException("Job has completed normally", null, this)
         }
     }
@@ -766,21 +767,14 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
 
     /**
      * Returns the cause that signals the completion of this job -- it returns the original
-     * [cancel] cause or **`null` if this job had completed
-     * normally or was cancelled without a cause**. This function throws
-     * [IllegalStateException] when invoked for an job that has not [completed][isCompleted] nor
+     * [cancel] cause, [JobCancellationException] or **`null` if this job had completed normally**.
+     * This function throws [IllegalStateException] when invoked for an job that has not [completed][isCompleted] nor
      * [isCancelled] yet.
      */
     protected fun getCompletionCause(): Throwable? {
         val state = this.state
         return when {
-            state is Finishing && state.cancelled != null -> {
-                if (state.cancelled.isCancelledWithoutCause()) {
-                    null
-                } else {
-                    state.cancelled.cause
-                }
-            }
+            state is Finishing && state.cancelled != null -> state.cancelled.cause
             state is Incomplete -> error("Job was not completed or cancelled yet")
             state is CompletedExceptionally -> state.cause
             else -> null
@@ -1060,7 +1054,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
             }
             // cancel all children in list on exceptional completion
             if (proposedUpdate is CompletedExceptionally)
-                child?.cancelChildrenInternal(proposedUpdate.exception)
+                child?.cancelChildrenInternal(proposedUpdate.cause)
             // switch to completing state
             val cancelled = (state as? Finishing)?.cancelled ?: (proposedUpdate as? Cancelled)
             val completing = Finishing(list, cancelled, true)
@@ -1080,7 +1074,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
     }
 
     private val Any?.exceptionOrNull: Throwable?
-        get() = (this as? CompletedExceptionally)?.exception
+        get() = (this as? CompletedExceptionally)?.cause
 
     private fun firstChild(state: Incomplete) =
         state as? Child ?: state.list?.nextChild()
@@ -1232,7 +1226,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
     internal fun getCompletedInternal(): Any? {
         val state = this.state
         check(state !is Incomplete) { "This job has not completed yet" }
-        if (state is CompletedExceptionally) throw state.exception
+        if (state is CompletedExceptionally) throw state.cause
         return state
     }
 
@@ -1245,7 +1239,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
             val state = this.state
             if (state !is Incomplete) {
                 // already complete -- just return result
-                if (state is CompletedExceptionally) throw state.exception
+                if (state is CompletedExceptionally) throw state.cause
                 return state
 
             }
@@ -1259,7 +1253,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
             val state = this.state
             check(state !is Incomplete)
             if (state is CompletedExceptionally)
-                cont.resumeWithException(state.exception)
+                cont.resumeWithException(state.cause)
             else
                 cont.resume(state)
         })
@@ -1278,7 +1272,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
                 // already complete -- select result
                 if (select.trySelect(null)) {
                     if (state is CompletedExceptionally)
-                        select.resumeSelectCancellableWithException(state.exception)
+                        select.resumeSelectCancellableWithException(state.cause)
                     else
                         block.startCoroutineUndispatched(state as T, select.completion)
                 }
@@ -1300,7 +1294,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
         val state = this.state
         // Note: await is non-atomic (can be cancelled while dispatched)
         if (state is CompletedExceptionally)
-            select.resumeSelectCancellableWithException(state.exception)
+            select.resumeSelectCancellableWithException(state.cause)
         else
             block.startCoroutineCancellable(state as T, select.completion)
     }

core/kotlinx-coroutines-core/src/main/kotlin/kotlinx/coroutines/experimental/CoroutineContext.kt

@@ -41,7 +41,7 @@ public const val DEBUG_PROPERTY_VALUE_ON = "on"
  */
 public const val DEBUG_PROPERTY_VALUE_OFF = "off"
 
-private val DEBUG = run {
+internal val DEBUG = run {
     val value = try { System.getProperty(DEBUG_PROPERTY_NAME) }
         catch (e: SecurityException) { null }
     when (value) {

core/kotlinx-coroutines-core/src/main/kotlin/kotlinx/coroutines/experimental/Exceptions.kt

@@ -16,8 +16,6 @@
 
 package kotlinx.coroutines.experimental
 
-import java.util.concurrent.*
-
 /**
  * This exception gets thrown if an exception is caught while processing [CompletionHandler] invocation for [Job].
  */
@@ -47,7 +45,24 @@ public actual class JobCancellationException public actual constructor(
      */
     public actual val job: Job
 ) : CancellationException(message) {
-    init { if (cause != null) initCause(cause) }
+
+    init {
+        if (cause != null) initCause(cause)
+    }
+
+    override fun fillInStackTrace(): Throwable {
+        if (DEBUG) {
+            return super.fillInStackTrace()
+        }
+
+        /*
+         * In non-debug mode we don't want to have a stacktrace on every cancellation/close,
+         * parent job reference is enough. Stacktrace of JCE is not needed most of the time (e.g., it is not logged)
+         * and hurts performance.
+         */
+        return this
+    }
+
     override fun toString(): String = "${super.toString()}; job=$job"
     override fun equals(other: Any?): Boolean =
         other === this ||

core/kotlinx-coroutines-core/src/main/kotlin/kotlinx/coroutines/experimental/channels/Produce.kt

@@ -124,7 +124,7 @@ private class ProducerCoroutine<E>(
         val cause = exceptionally?.cause
         val processed = when (exceptionally) {
             // producer coroutine was cancelled -- cancel channel, but without cause if it was closed without cause
-            is Cancelled -> _channel.cancel(if (exceptionally.isCancelledWithoutCause()) null else cause)
+            is Cancelled -> _channel.cancel(if (cause is JobCancellationException) null else cause)
             else -> _channel.close(cause) // producer coroutine has completed -- close channel
         }
         if (!processed && cause != null)

core/kotlinx-coroutines-core/src/test/kotlin/kotlinx/coroutines/experimental/channels/ActorTest.kt

@@ -20,9 +20,9 @@ import kotlinx.coroutines.experimental.*
 import org.hamcrest.core.*
 import org.junit.*
 import org.junit.Assert.*
-import org.junit.runner.RunWith
-import org.junit.runners.Parameterized
-import java.io.IOException
+import org.junit.runner.*
+import org.junit.runners.*
+import java.io.*
 import kotlin.coroutines.experimental.*
 
 @RunWith(Parameterized::class)
@@ -80,9 +80,8 @@ class ActorTest(private val capacity: Int) : TestBase() {
         finish(7)
     }
 
-
     @Test
-    fun testCancelWithoutCause() = runTest {
+    fun testCloseWithoutCause() = runTest {
         val actor = actor<Int>(coroutineContext, capacity) {
             val element = channel.receiveOrNull()
             expect(2)
@@ -101,7 +100,7 @@ class ActorTest(private val capacity: Int) : TestBase() {
     }
 
     @Test
-    fun testCancelWithCause() = runTest {
+    fun testCloseWithCause() = runTest {
         val actor = actor<Int>(coroutineContext, capacity) {
             val element = channel.receiveOrNull()
             expect(2)
@@ -120,4 +119,31 @@ class ActorTest(private val capacity: Int) : TestBase() {
         yield()
         finish(4)
     }
+
+    @Test
+    fun testCancelEnclosingJob() = runTest {
+        val job = async(coroutineContext) {
+            actor<Int>(coroutineContext, capacity) {
+                expect(1)
+                channel.receiveOrNull()
+                expectUnreached()
+            }
+        }
+
+        yield()
+        yield()
+
+        expect(2)
+        yield()
+        job.cancel()
+
+        try {
+            job.await()
+            expectUnreached()
+        } catch (e: JobCancellationException) {
+            assertTrue(e.message?.contains("Job was cancelled normally") ?: false)
+        }
+
+        finish(3)
+    }
 }

core/kotlinx-coroutines-core/src/test/kotlin/kotlinx/coroutines/experimental/channels/ArrayChannelTest.kt

@@ -19,7 +19,7 @@ package kotlinx.coroutines.experimental.channels
 import kotlinx.coroutines.experimental.*
 import org.junit.*
 import org.junit.Assert.*
-import java.io.IOException
+import java.io.*
 import kotlin.coroutines.experimental.*
 
 class ArrayChannelTest : TestBase() {

reactive/kotlinx-coroutines-reactive/src/main/kotlin/kotlinx/coroutines/experimental/reactive/Publish.kt

@@ -167,7 +167,7 @@ private class PublisherCoroutine<in T>(
                 _nRequested.value = SIGNALLED // we'll signal onError/onCompleted (that the final state -- no CAS needed)
                 val cause = getCompletionCause()
                 try {
-                    if (cause != null)
+                    if (cause != null && cause !is JobCancellationException)
                         subscriber.onError(cause)
                     else
                         subscriber.onComplete()

reactive/kotlinx-coroutines-rx1/src/main/kotlin/kotlinx/coroutines/experimental/rx1/RxObservable.kt

@@ -168,7 +168,7 @@ private class RxObservableCoroutine<in T>(
                 _nRequested.value = SIGNALLED // we'll signal onError/onCompleted (that the final state -- no CAS needed)
                 val cause = getCompletionCause()
                 try {
-                    if (cause != null)
+                    if (cause != null && cause !is JobCancellationException)
                         subscriber.onError(cause)
                     else
                         subscriber.onCompleted()

reactive/kotlinx-coroutines-rx2/src/main/kotlin/kotlinx/coroutines/experimental/rx2/RxObservable.kt

@@ -158,7 +158,7 @@ private class RxObservableCoroutine<T>(
                 _signal.value = SIGNALLED // we'll signal onError/onCompleted (that the final state -- no CAS needed)
                 val cause = getCompletionCause()
                 try {
-                    if (cause != null)
+                    if (cause != null && cause !is JobCancellationException)
                         subscriber.onError(cause)
                     else
                         subscriber.onComplete()