Merge branch 'no-unwrap' into version-0.30.1

qwwdfsad · qwwdfsad · commit a3826a9d87c0 · 2018-10-02T14:15:54.000+03:00
diff --git a/common/kotlinx-coroutines-core-common/src/AbstractContinuation.kt b/common/kotlinx-coroutines-core-common/src/AbstractContinuation.kt
@@ -105,7 +105,7 @@ internal abstract class AbstractContinuation<in T>(
     /**
      * It is used when parent is cancelled to get the cancellation cause for this continuation.
      */
-    open fun getParentCancellationCause(parent: Job): Throwable =
+    open fun getContinuationCancellationCause(parent: Job): Throwable =
         parent.getCancellationException()
 
     private fun trySuspend(): Boolean {
diff --git a/common/kotlinx-coroutines-core-common/src/Job.kt b/common/kotlinx-coroutines-core-common/src/Job.kt
@@ -425,13 +425,30 @@ public inline fun DisposableHandle(crossinline block: () -> Unit) =
 internal interface ChildJob : Job {
     /**
      * Parent is cancelling its child by invoking this method.
-     * Child finds the cancellation cause using [getCancellationException] of the [parentJob].
+     * Child finds the cancellation cause using [ParentJob.getChildJobCancellationCause].
      * This method does nothing is the child is already being cancelled.
      *
      * @suppress **This is unstable API and it is subject to change.**
      */
     @InternalCoroutinesApi
-    public fun parentCancelled(parentJob: Job)
+    public fun parentCancelled(parentJob: ParentJob)
+}
+
+/**
+ * A reference that child receives from its parent when it is being cancelled by the parent.
+ *
+ * @suppress **This is unstable API and it is subject to change.**
+ */
+@InternalCoroutinesApi
+internal interface ParentJob : Job {
+    /**
+     * Child job is using this method to learn its cancellation cause when the parent cancels it with [ChildJob.parentCancelled].
+     * This method is invoked only if the child was not already being cancelled.
+     *
+     * @suppress **This is unstable API and it is subject to change.**
+     */
+    @InternalCoroutinesApi
+    public fun getChildJobCancellationCause(): Throwable
 }
 
 /**
diff --git a/common/kotlinx-coroutines-core-common/src/JobSupport.kt b/common/kotlinx-coroutines-core-common/src/JobSupport.kt
@@ -20,7 +20,7 @@ import kotlin.coroutines.experimental.intrinsics.*
  * @param active when `true` the job is created in _active_ state, when `false` in _new_ state. See [Job] for details.
  * @suppress **This is unstable API and it is subject to change.**
  */
-internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, SelectClause0 {
+internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, ParentJob, SelectClause0 {
     final override val key: CoroutineContext.Key<*> get() = Job
 
     /*
@@ -230,63 +230,23 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel
             if (state.isCancelling) return createJobCancellationException()
             return null
         }
-        /*
-         * This is a place where we step on our API limitation:
-         * We can't distinguish internal JobCancellationException from our parent
-         * from external cancellation, thus we ought to collect all exceptions.
-         * If parent is cancelling, it cancels its children with JCE(rootCause).
-         * When child is building final exception, it can skip JCE(anything) if it knows
-         * that parent handles exceptions, because parent should already have this exception.
-         * If parent does not, then we should unwrap exception, otherwise in the following code
-         * ```
-         * val parent = Job()
-         * launch(parent) {
-         *   try { delay() } finally { throw E2() }
-         * }
-         * parent.cancel(E1)
-         * ```
-         * E1 will be lost.
-         */
-        var rootCause = exceptions[0]
-        if (rootCause is CancellationException) {
-            val cause = unwrap(rootCause)
-            rootCause = if (cause !== null) {
-                cause
-            } else {
-                exceptions.firstOrNull { unwrap(it) != null } ?: return rootCause
-            }
-        }
-        return rootCause
+        // Take either the first real exception (not a cancellation) or just the first exception
+        return exceptions.firstOrNull { it !is CancellationException } ?: exceptions[0]
     }
 
     private fun suppressExceptions(rootCause: Throwable, exceptions: List<Throwable>): Boolean {
         if (exceptions.size <= 1) return false // nothing more to do here
         val seenExceptions = identitySet<Throwable>(exceptions.size)
         var suppressed = false
-        for (i in 1 until exceptions.size) {
-            val unwrapped = unwrap(exceptions[i])
-            if (unwrapped !== null && unwrapped !== rootCause) {
-                if (seenExceptions.add(unwrapped)) {
-                    rootCause.addSuppressedThrowable(unwrapped)
-                    suppressed = true
-                }
+        for (exception in exceptions) {
+            if (exception !== rootCause && exception !is CancellationException && seenExceptions.add(exception)) {
+                rootCause.addSuppressedThrowable(exception)
+                suppressed = true
             }
         }
         return suppressed
     }
 
-    private tailrec fun unwrap(exception: Throwable): Throwable? {
-        if (exception is CancellationException && parentHandlesExceptions) {
-            return null
-        }
-        return if (exception is CancellationException) {
-            val cause = exception.cause
-            if (cause !== null) unwrap(cause) else null
-        } else {
-            exception
-        }
-    }
-
     // fast-path method to finalize normally completed coroutines without children
     private fun tryFinalizeSimpleState(state: Incomplete, update: Any?, mode: Int): Boolean {
         check(state is Empty || state is JobNode<*>) // only simple state without lists where children can concurrently add
@@ -616,15 +576,15 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel
         cancelImpl(cause) && handlesException
 
     // Parent is cancelling child
-    public final override fun parentCancelled(parentJob: Job) {
+    public final override fun parentCancelled(parentJob: ParentJob) {
         cancelImpl(parentJob)
     }
 
     // Child was cancelled with cause
     public open fun childCancelled(cause: Throwable): Boolean =
         cancelImpl(cause) && handlesException
 
-    // cause is Throwable or Job when cancelChild was invoked
+    // cause is Throwable or ParentJob when cancelChild was invoked
     // returns true is exception was handled, false otherwise
     private fun cancelImpl(cause: Any?): Boolean {
         if (onCancelComplete) {
@@ -636,6 +596,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel
         return makeCancelling(cause)
     }
 
+    // cause is Throwable or ParentJob when cancelChild was invoked
     private fun cancelMakeCompleting(cause: Any?): Boolean {
         loopOnState { state ->
             if (state !is Incomplete || state is Finishing && state.isCompleting) {
@@ -654,14 +615,35 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel
     private fun createJobCancellationException() =
         JobCancellationException("Job was cancelled", null, this)
 
-    // cause is Throwable or Job when cancelChild was invoked, cause can be null only on cancel
+    override fun getChildJobCancellationCause(): Throwable {
+        // determine root cancellation cause of this job (why is it cancelling its children?)
+        val state = this.state
+        val rootCause = when (state) {
+            is Finishing -> state.rootCause
+            is Incomplete -> error("Cannot be cancelling child in this state: $state")
+            is CompletedExceptionally -> state.cause
+            else -> null // create exception with the below code on normal completion
+        }
+        /*
+         * If this parent job handles exceptions, then wrap cause into JobCancellationException, because we
+         * don't want the child to handle this exception on more time. Otherwise, pass our original rootCause
+         * to the child for cancellation.
+         */
+        return if (rootCause == null || handlesException && rootCause !is CancellationException) {
+            JobCancellationException("Parent job is ${stateString(state)}", rootCause, this)
+        } else {
+            rootCause
+        }
+    }
+
+    // cause is Throwable or ParentJob when cancelChild was invoked
     private fun createCauseException(cause: Any?): Throwable = when(cause) {
         is Throwable? -> cause ?: createJobCancellationException()
-        else -> (cause as Job).getCancellationException()
+        else -> (cause as ParentJob).getChildJobCancellationCause()
     }
 
     // transitions to Cancelling state
-    // cause is Throwable or Job when cancelChild was invoked, cause can be null only on cancel
+    // cause is Throwable or ParentJob when cancelChild was invoked
     private fun makeCancelling(cause: Any?): Boolean {
         var causeExceptionCache: Throwable? = null // lazily init result of createCauseException(cause)
         loopOnState { state ->
@@ -927,10 +909,6 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel
      */
     protected open val handlesException: Boolean get() = true
 
-    // returns true when we know that parent handles exceptions
-    private val parentHandlesExceptions: Boolean get() =
-        (parentHandle as? ChildHandleNode)?.job?.handlesException ?: false
-
     /**
      * This method is invoked **exactly once** when the final exception of the job is determined
      * and before it becomes complete. At the moment of invocation the job and all its children are complete.
@@ -959,27 +937,22 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel
 
     // for nicer debugging
     public override fun toString(): String =
-        "${nameString()}{${stateString()}}@$hexAddress"
+        "${nameString()}{${stateString(state)}}@$hexAddress"
 
     /**
      * @suppress **This is unstable API and it is subject to change.**
      */
     internal open fun nameString(): String = classSimpleName
 
-    private fun stateString(): String {
-        val state = this.state
-        return when (state) {
-            is Finishing -> buildString {
-                when {
-                    state.isCancelling -> append("Cancelling")
-                    else -> append("Active")
-                }
-                if (state.isCompleting) append("Completing")
-            }
-            is Incomplete -> if (state.isActive) "Active" else "New"
-            is CompletedExceptionally -> "Cancelled"
-            else -> "Completed"
+    private fun stateString(state: Any?): String = when (state) {
+        is Finishing -> when {
+            state.isCancelling -> "Cancelling"
+            state.isCompleting -> "Completing"
+            else -> "Active"
         }
+        is Incomplete -> if (state.isActive) "Active" else "New"
+        is CompletedExceptionally -> "Cancelled"
+        else -> "Completed"
     }
 
     // Completing & Cancelling states,
@@ -1068,7 +1041,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, Sel
         delegate: Continuation<T>,
         private val job: JobSupport
     ) : CancellableContinuationImpl<T>(delegate, MODE_CANCELLABLE) {
-        override fun getParentCancellationCause(parent: Job): Throwable {
+        override fun getContinuationCancellationCause(parent: Job): Throwable {
             val state = job.state
             /*
              * When the job we are waiting for had already completely completed exceptionally or
@@ -1351,7 +1324,7 @@ internal class ChildContinuation(
     @JvmField val child: AbstractContinuation<*>
 ) : JobCancellingNode<Job>(parent) {
     override fun invoke(cause: Throwable?) {
-        child.cancelImpl(child.getParentCancellationCause(job))
+        child.cancelImpl(child.getContinuationCancellationCause(job))
     }
     override fun toString(): String =
         "ChildContinuation[$child]"
diff --git a/core/kotlinx-coroutines-core/test/exceptions/WithContextExceptionHandlingTest.kt b/core/kotlinx-coroutines-core/test/exceptions/WithContextExceptionHandlingTest.kt
@@ -13,11 +13,13 @@ import kotlin.coroutines.experimental.*
 import kotlin.test.*
 
 @RunWith(Parameterized::class)
-class WithContextExceptionHandlingTest(private val withContext: Boolean) : TestBase() {
+class WithContextExceptionHandlingTest(private val mode: Mode) : TestBase() {
+    enum class Mode { WITH_CONTEXT, ASYNC_AWAIT }
+
     companion object {
-        @Parameterized.Parameters(name = "withContext={0}")
+        @Parameterized.Parameters(name = "mode={0}")
         @JvmStatic
-        fun params(): Collection<Array<Any>> = listOf<Array<Any>>(arrayOf(true), arrayOf(false))
+        fun params(): Collection<Array<Any>> = Mode.values().map { arrayOf<Any>(it) }
     }
 
     @Test
@@ -106,16 +108,14 @@ class WithContextExceptionHandlingTest(private val withContext: Boolean) : TestB
         /*
          * context cancelled with ISE
          * block itself throws CE(IOE)
-         * Result: ISE suppressed IOE
+         * Result: ISE (because cancellation exception is always ignored and not handled)
          */
         val cancellationCause = IllegalStateException()
         val thrown = CancellationException()
         thrown.initCause(IOException())
         runCancellation(cancellationCause, thrown) { e ->
             assertSame(cancellationCause, e)
-            val suppressed = e.suppressed
-            assertEquals(1, suppressed.size)
-            assertTrue(suppressed[0] is IOException)
+            assertTrue(e.suppressed.isEmpty())
         }
     }
 
@@ -161,10 +161,11 @@ class WithContextExceptionHandlingTest(private val withContext: Boolean) : TestB
 
     @Test
     fun testThrowingCancellationWithCause() = runTest {
+        // Exception are never unwrapped, so if CE(IOE) is thrown then it is the cancellation cause
         val thrown = CancellationException()
         thrown.initCause(IOException())
         runThrowing(thrown) { e ->
-           checkException<IOException>(e)
+           assertSame(thrown, e)
         }
     }
 
@@ -247,12 +248,11 @@ class WithContextExceptionHandlingTest(private val withContext: Boolean) : TestB
     }
 
     private suspend fun withCtx(context: CoroutineContext, job: Job = Job(), block: suspend CoroutineScope.(Job) -> Nothing) {
-        if (withContext) {
-            withContext(context + job) {
+        when (mode) {
+            Mode.WITH_CONTEXT -> withContext(context + job) {
                 block(job)
             }
-        } else {
-            CoroutineScope(coroutineContext).async(context + job) {
+            Mode.ASYNC_AWAIT -> CoroutineScope(coroutineContext).async(context + job) {
                 block(job)
             }.await()
         }
