Child fails on parent failure (not cancelled)

* This ensures transparency with respect to the fact that a job is
  a child in a hierarchy. When it fails, there is no side-effect of
  its being cancelled by its parent anymore.
* ChildJob and ChildHandle are made internal to further deter 3rd-party
  implementation and usage of parent-child machinery.

Author: elizarov

binary-compatibility-validator/reference-public-api/kotlinx-coroutines-core.txt

@@ -91,10 +91,6 @@ public final class kotlinx/coroutines/experimental/CancelledContinuation : kotli
 	public fun <init> (Lkotlin/coroutines/experimental/Continuation;Ljava/lang/Throwable;)V
 }
 
-public abstract interface class kotlinx/coroutines/experimental/ChildHandle : kotlinx/coroutines/experimental/DisposableHandle {
-	public abstract fun childFailed (Ljava/lang/Throwable;)Z
-}
-
 public abstract class kotlinx/coroutines/experimental/CloseableCoroutineDispatcher : kotlinx/coroutines/experimental/CoroutineDispatcher, java/io/Closeable {
 	public fun <init> ()V
 }
@@ -373,9 +369,8 @@ public final class kotlinx/coroutines/experimental/GlobalScope : kotlinx/corouti
 
 public abstract interface class kotlinx/coroutines/experimental/Job : kotlin/coroutines/experimental/CoroutineContext$Element {
 	public static final field Key Lkotlinx/coroutines/experimental/Job$Key;
-	public abstract fun attachChild (Lkotlinx/coroutines/experimental/Job;)Lkotlinx/coroutines/experimental/ChildHandle;
+	public abstract fun attachChild (Lkotlinx/coroutines/experimental/ChildJob;)Lkotlinx/coroutines/experimental/ChildHandle;
 	public abstract fun cancel (Ljava/lang/Throwable;)Z
-	public abstract fun cancelChild (Lkotlinx/coroutines/experimental/Job;)V
 	public abstract synthetic fun cancelChildren (Ljava/lang/Throwable;)V
 	public abstract fun getCancellationException ()Ljava/util/concurrent/CancellationException;
 	public abstract fun getChildren ()Lkotlin/sequences/Sequence;
@@ -446,9 +441,8 @@ public final class kotlinx/coroutines/experimental/LazyDeferredKt {
 
 public final class kotlinx/coroutines/experimental/NonCancellable : kotlin/coroutines/experimental/AbstractCoroutineContextElement, kotlinx/coroutines/experimental/Job {
 	public static final field INSTANCE Lkotlinx/coroutines/experimental/NonCancellable;
-	public fun attachChild (Lkotlinx/coroutines/experimental/Job;)Lkotlinx/coroutines/experimental/ChildHandle;
+	public fun attachChild (Lkotlinx/coroutines/experimental/ChildJob;)Lkotlinx/coroutines/experimental/ChildHandle;
 	public fun cancel (Ljava/lang/Throwable;)Z
-	public fun cancelChild (Lkotlinx/coroutines/experimental/Job;)V
 	public synthetic fun cancelChildren (Ljava/lang/Throwable;)V
 	public fun getCancellationException ()Ljava/util/concurrent/CancellationException;
 	public fun getChildren ()Lkotlin/sequences/Sequence;

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

@@ -16,7 +16,7 @@ import kotlin.coroutines.experimental.*
 /**
  * A background job. Conceptually, a job is a cancellable thing with a life-cycle that
  * culminates in its completion. Jobs can be arranged into parent-child hierarchies where failure or cancellation
- * of parent immediately cancels all its [children].
+ * of parent lead to an immediate failure of all its [children].
  *
  * The most basic instances of [Job] are created with [launch][CoroutineScope.launch] coroutine builder or with a
  * `Job()` factory function.
@@ -69,7 +69,7 @@ import kotlin.coroutines.experimental.*
  * [coroutineContext](https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.coroutines.experimental/coroutine-context.html)
  * represents the coroutine itself.
  *
- * A job can have a _parent_ job. A job with a parent is cancelled when its parent is cancelled or is failing.
+ * A job can have a _parent_ job. A job with a parent is failing when its parent is cancelled or is failing.
  * Parent job waits in _completing_, _failing_, or _cancelling_ state for all its children to complete before finishing.
  * Note, that _completing_ state is purely internal to the job. For an outside observer a _completing_ job is still
  * active, while internally it is waiting for its children.
@@ -181,15 +181,6 @@ public interface Job : CoroutineContext.Element {
 
     // ------------ parent-child ------------
 
-    /**
-     * Cancels child job. This method is invoked by [parentJob] to cancel this child job.
-     * Child finds the cancellation cause using [getCancellationException] of the [parentJob].
-     * This method does nothing is the child is already being cancelled.
-     *
-     * @suppress **This is unstable API and it is subject to change.**
-     */
-    public fun cancelChild(parentJob: Job)
-
     /**
      * Returns a sequence of this job's children.
      *
@@ -216,21 +207,23 @@ public interface Job : CoroutineContext.Element {
      *
      * A parent-child relation has the following effect:
      * * Cancellation of parent with [cancel] or its exceptional completion (failure)
-     *   immediately cancels all its children.
+     *   immediately fails all its children.
      * * Parent cannot complete until all its children are complete. Parent waits for all its children to
-     *   complete in _completing_ or _cancelling_ state.
+     *   complete in _completing_, _failing_, or _cancelling_ states.
      *
-     * **A child must store the resulting [DisposableHandle] and [dispose][DisposableHandle.dispose] the attachment
+     * **A child must store the resulting [ChildHandle] and [dispose][DisposableHandle.dispose] the attachment
      * to its parent on its own completion.**
      *
      * Coroutine builders and job factory functions that accept `parent` [CoroutineContext] parameter
      * lookup a [Job] instance in the parent context and use this function to attach themselves as a child.
-     * They also store a reference to the resulting [DisposableHandle] and dispose a handle when they complete.
+     * They also store a reference to the resulting [ChildHandle] and dispose a handle when they complete.
      *
      * @suppress **This is unstable API and it is subject to change.**
      *           This is an internal API. This method is too error prone for public API.
      */
-    public fun attachChild(child: Job): ChildHandle
+    // ChildJob and ChildHandle are made internal on purpose to further deter 3rd-party impl of Job
+    @Suppress("EXPOSED_FUNCTION_RETURN_TYPE", "EXPOSED_PARAMETER_TYPE")
+    public fun attachChild(child: ChildJob): ChildHandle
 
     /**
      * Cancels all children jobs of this coroutine with the given [cause]. Unlike [cancel],
@@ -387,9 +380,27 @@ public interface DisposableHandle {
 }
 
 /**
+ * A reference that parent receives from its child so that it can report its failure.
+ *
+ * @suppress **This is unstable API and it is subject to change.**
+ */
+internal interface ChildJob : Job {
+    /**
+     * Parent is reporting failure to the child by invoking this method.
+     * Child finds the failure cause using [getCancellationException] of the [parentJob].
+     * This method does nothing is the child is already failing.
+     *
+     * @suppress **This is unstable API and it is subject to change.**
+     */
+    public fun parentFailed(parentJob: Job)
+}
+
+/**
+ * A handle that child keep onto its parent so that it is able to report its failure.
+ * 
  * @suppress **This is unstable API and it is subject to change.**
  */
-public interface ChildHandle : DisposableHandle {
+internal interface ChildHandle : DisposableHandle {
     /**
      * Child is reporting failure to the parent by invoking this method.
      * This method is invoked by the child twice. The first time child report its root cause as soon as possible,

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

@@ -20,7 +20,7 @@ import kotlin.coroutines.experimental.*
  * @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, SelectClause0 {
+internal open class JobSupport constructor(active: Boolean) : Job, ChildJob, SelectClause0 {
     final override val key: CoroutineContext.Key<*> get() = Job
 
     /*
@@ -476,7 +476,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
                                 rootCause = state.rootCause // != null if we are failing
                                 // We add node to the list in two cases --- either the job is not failing
                                 // or we are adding a child to a coroutine that is not completing yet
-                                if (rootCause == null || handler.isHandlerOf<ChildJob>() && !state.isCompleting) {
+                                if (rootCause == null || handler.isHandlerOf<ChildHandleImpl>() && !state.isCompleting) {
                                     // Note: add node the list while holding lock on state (make sure it cannot change)
                                     val node = nodeCache ?: makeNode(handler, onFailing).also { nodeCache = it }
                                     if (!addLastAtomic(state, list, node)) return@loopOnState // retry
@@ -613,15 +613,15 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
     public override fun cancel(cause: Throwable?): Boolean =
         fail(cause, cancel = true) && handlesException
 
+    // parent is reporting failure to a child child
+    public final override fun parentFailed(parentJob: Job) {
+        fail(parentJob, cancel = false)
+    }
+
     // child is reporting failure to the parent
-    internal fun childFailed(cause: Throwable) =
+    public fun childFailed(cause: Throwable) =
         fail(cause, cancel = false) && handlesException
 
-    // parent is cancelling child
-    public override fun cancelChild(parentJob: Job) {
-        fail(parentJob, cancel = true)
-    }
-
     // cause is Throwable or Job when cancelChild was invoked, cause can be null only on cancel
     // returns true is exception was handled, false otherwise
     private fun fail(cause: Any?, cancel: Boolean): Boolean {
@@ -783,7 +783,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
          * Otherwise, there can be a race between (completed state -> handled exception and newly attached child/join)
          * which may miss unhandled exception.
          */
-        if ((state is Empty || state is JobNode<*>) && state !is ChildJob && proposedUpdate !is CompletedExceptionally) {
+        if ((state is Empty || state is JobNode<*>) && state !is ChildHandleImpl && proposedUpdate !is CompletedExceptionally) {
             if (!tryFinalizeSimpleState(state, proposedUpdate, mode)) return COMPLETING_RETRY
             return COMPLETING_COMPLETED
         }
@@ -833,11 +833,11 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
         get() = (this as? CompletedExceptionally)?.cause
 
     private fun firstChild(state: Incomplete) =
-        state as? ChildJob ?: state.list?.nextChild()
+        state as? ChildHandleImpl ?: state.list?.nextChild()
 
     // return false when there is no more incomplete children to wait
     // ## IMPORTANT INVARIANT: Only one thread can be concurrently invoking this method.
-    private tailrec fun tryWaitForChild(state: Finishing, child: ChildJob, proposedUpdate: Any?): Boolean {
+    private tailrec fun tryWaitForChild(state: Finishing, child: ChildHandleImpl, proposedUpdate: Any?): Boolean {
         val handle = child.childJob.invokeOnCompletion(
             invokeImmediately = false,
             handler = ChildCompletion(this, state, child, proposedUpdate).asHandler
@@ -848,7 +848,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
     }
 
     // ## IMPORTANT INVARIANT: Only one thread can be concurrently invoking this method.
-    private fun continueCompleting(state: Finishing, lastChild: ChildJob, proposedUpdate: Any?) {
+    private fun continueCompleting(state: Finishing, lastChild: ChildHandleImpl, proposedUpdate: Any?) {
         require(this.state === state) // consistency check -- it cannot change while we are waiting for children
         // figure out if we need to wait for next child
         val waitChild = lastChild.nextChild()
@@ -858,39 +858,39 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
         if (tryFinalizeFinishingState(state, proposedUpdate, MODE_ATOMIC_DEFAULT)) return
     }
 
-    private fun LockFreeLinkedListNode.nextChild(): ChildJob? {
+    private fun LockFreeLinkedListNode.nextChild(): ChildHandleImpl? {
         var cur = this
         while (cur.isRemoved) cur = cur.prevNode // rollback to prev non-removed (or list head)
         while (true) {
             cur = cur.nextNode
             if (cur.isRemoved) continue
-            if (cur is ChildJob) return cur
+            if (cur is ChildHandleImpl) return cur
             if (cur is NodeList) return null // checked all -- no more children
         }
     }
 
     public final override val children: Sequence<Job> get() = buildSequence {
         val state = this@JobSupport.state
         when (state) {
-            is ChildJob -> yield(state.childJob)
+            is ChildHandleImpl -> yield(state.childJob)
             is Incomplete -> state.list?.let { list ->
-                list.forEach<ChildJob> { yield(it.childJob) }
+                list.forEach<ChildHandleImpl> { yield(it.childJob) }
             }
         }
     }
 
     @Suppress("OverridingDeprecatedMember")
-    public final override fun attachChild(child: Job): ChildHandle {
+    public final override fun attachChild(child: ChildJob): ChildHandle {
         /*
          * Note: This function attaches a special ChildNode object. This node object
          * is handled in a special way on completion on the coroutine (we wait for all of them) and
          * is handled specially by invokeOnCompletion itself -- it adds this node to the list even
          * if the job is already failing.  For "failing" state child is attached under state lock.
          * It's required to properly wait all children before completion and provide linearizable hierarchy view:
-         * If child is attached when job is failing, such child will receive immediate cancellation exception,
+         * If child is attached when job is failing, such child will receive immediate notification on failure,
          * but parent *will* wait for that child before completion and will handle its exception.
          */
-        return invokeOnCompletion(onFailing = true, handler = ChildJob(this, child).asHandler) as ChildHandle
+        return invokeOnCompletion(onFailing = true, handler = ChildHandleImpl(this, child).asHandler) as ChildHandle
     }
 
     @Suppress("OverridingDeprecatedMember")
@@ -1058,7 +1058,7 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
     private class ChildCompletion(
         private val parent: JobSupport,
         private val state: Finishing,
-        private val child: ChildJob,
+        private val child: ChildHandleImpl,
         private val proposedUpdate: Any?
     ) : JobNode<Job>(child.childJob) {
         override fun invoke(cause: Throwable?) {
@@ -1301,16 +1301,16 @@ private class InvokeOnFailing(
     override fun toString() = "InvokeOnFailing[$classSimpleName@$hexAddress]"
 }
 
-internal class ChildJob(
+internal class ChildHandleImpl(
     parent: JobSupport,
-    @JvmField val childJob: Job
+    @JvmField val childJob: ChildJob
 ) : JobFailingNode<JobSupport>(parent), ChildHandle {
-    override fun invoke(cause: Throwable?) = childJob.cancelChild(job)
+    override fun invoke(cause: Throwable?) = childJob.parentFailed(job)
     override fun childFailed(cause: Throwable): Boolean = job.childFailed(cause)
-    override fun toString(): String = "ChildJob[$childJob]"
+    override fun toString(): String = "ChildHandle[$childJob]"
 }
 
-// Same as ChildJob, but for cancellable continuation
+// Same as ChildHandleImpl, but for cancellable continuation
 internal class ChildContinuation(
     parent: Job,
     @JvmField val child: AbstractContinuation<*>

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

@@ -68,16 +68,17 @@ public object NonCancellable : AbstractCoroutineContextElement(Job), Job {
     /** Always returns `false`. */
     override fun cancel(cause: Throwable?): Boolean = false // never handles exceptions
 
-    /** @suppress */
-    override fun cancelChild(parentJob: Job): Unit = error("Cannot be invoked, does not have a parent")
-
     /** Always returns [emptySequence]. */
     override val children: Sequence<Job>
         get() = emptySequence()
 
-    /** Always returns [NonDisposableHandle] and does not do anything. */
-    @Suppress("OverridingDeprecatedMember")
-    override fun attachChild(child: Job): ChildHandle = NonDisposableHandle
+    /**
+     * Always returns [NonDisposableHandle] and does not do anything.
+     * @suppress **This is unstable API and it is subject to change.**
+     *           This is an internal API. This method is too error prone for public API.
+     */
+    @Suppress("EXPOSED_FUNCTION_RETURN_TYPE", "EXPOSED_PARAMETER_TYPE")
+    override fun attachChild(child: ChildJob): ChildHandle = NonDisposableHandle
 
     /** Does not do anything. */
     @Suppress("OverridingDeprecatedMember")

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

@@ -101,14 +101,19 @@ class CompletableDeferredTest : TestBase() {
     }
 
     @Test
-    fun testParentCancelsChild() {
+    fun testParentFailsChild() {
         val parent = Job()
         val c = CompletableDeferred<String>(parent)
         checkFresh(c)
         parent.cancel()
         assertEquals(false, parent.isActive)
         assertEquals(true, parent.isCancelled)
-        checkCancel(c)
+        assertEquals(false, c.isActive)
+        assertEquals(false, c.isCancelled)
+        assertEquals(true, c.isCompleted)
+        assertEquals(true, c.isCompletedExceptionally)
+        assertThrows<CancellationException> { c.getCompleted() }
+        assertTrue(c.getCompletionExceptionOrNull() is CancellationException)
     }
 
     @Test

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

@@ -286,15 +286,15 @@ class CoroutinesTest : TestBase() {
     }
 
     @Test
-    fun testParentCrashCancelsChildren() = runTest(
+    fun testParentCrashFailsChildren() = runTest(
         unhandled = listOf({ it -> it is TestException })
     ) {
         expect(1)
         val parent = launch(Job()) {
             expect(4)
             throw TestException("Crashed")
         }
-        launch(parent, CoroutineStart.UNDISPATCHED) {
+        val child = launch(parent, CoroutineStart.UNDISPATCHED) {
             expect(2)
             try {
                 yield() // to test
@@ -310,6 +310,9 @@ class CoroutinesTest : TestBase() {
         expect(6)
         parent.join() // make sure crashed parent still waits for its child
         finish(8)
+        // make sure child fails but it is not cancelled
+        assertTrue(child.isFailed)
+        assertFalse(child.isCancelled)
     }
 
     @Test