Handle exception thrown by user-defined exception handler via global exception handler, so such exception can't break coroutines machinery

Fixes #562

Author: qwwdfsad

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

@@ -18,9 +18,8 @@ internal expect fun handleCoroutineExceptionImpl(context: CoroutineContext, exce
  *
  * If there is a [Job] in the context and it's not a [caller], then [Job.cancel] is invoked.
  * If invocation returned `true`, method terminates: now [Job] is responsible for handling an exception.
- * Otherwise, If there is [CoroutineExceptionHandler] in the context, it is used.
- * Otherwise all instances of [CoroutineExceptionHandler] found via [ServiceLoader] and [Thread.uncaughtExceptionHandler] are invoked
- *
+ * Otherwise, If there is [CoroutineExceptionHandler] in the context, it is used. If it throws an exception during handling
+ * or is absent, all instances of [CoroutineExceptionHandler] found via [ServiceLoader] and [Thread.uncaughtExceptionHandler] are invoked
  * todo: Deprecate/hide this function.
  */
 @JvmOverloads // binary compatibility
@@ -39,21 +38,25 @@ private fun handleExceptionViaJob(context: CoroutineContext, exception: Throwabl
 }
 
 internal fun handleExceptionViaHandler(context: CoroutineContext, exception: Throwable) {
+    // Invoke exception handler from the context if present
     try {
-        // Invoke exception handler from the context if present
         context[CoroutineExceptionHandler]?.let {
             it.handleException(context, exception)
             return
         }
-        // If handler is not present in the context, fallback to the global handler
-        handleCoroutineExceptionImpl(context, exception)
-    } catch (handlerException: Throwable) {
-        // simply rethrow if handler threw the original exception
-        if (handlerException === exception) throw exception
-        // handler itself crashed for some other reason -- that is bad -- keep both
-        throw RuntimeException("Exception while trying to handle coroutine exception", exception).apply {
-            addSuppressedThrowable(handlerException)
-        }
+    } catch (t: Throwable) {
+        handleCoroutineExceptionImpl(context, handlerException(exception, t))
+        return
+    }
+
+    // If handler is not present in the context or exception was thrown, fallback to the global handler
+    handleCoroutineExceptionImpl(context, exception)
+}
+
+internal fun handlerException(originalException: Throwable, thrownException: Throwable): Throwable {
+    if (originalException === thrownException) return originalException
+    return RuntimeException("Exception while trying to handle coroutine exception", thrownException).apply {
+        addSuppressedThrowable(originalException)
     }
 }
 

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

@@ -877,7 +877,10 @@ internal open class JobSupport constructor(active: Boolean) : Job, SelectClause0
          * 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.
+         * 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,
+         * 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
     }

core/kotlinx-coroutines-core/src/CoroutineExceptionHandlerImpl.kt

@@ -21,8 +21,15 @@ private val handlers: List<CoroutineExceptionHandler> = CoroutineExceptionHandle
 internal actual fun handleCoroutineExceptionImpl(context: CoroutineContext, exception: Throwable) {
     // use additional extension handlers
     for (handler in handlers) {
-        handler.handleException(context, exception)
+        try {
+            handler.handleException(context, exception)
+        } catch (t: Throwable) {
+            // Use thread's handler if custom handler failed to handle exception
+            val currentThread = Thread.currentThread()
+            currentThread.uncaughtExceptionHandler.uncaughtException(currentThread, handlerException(exception, t))
+        }
     }
+
     // use thread's handler
     val currentThread = Thread.currentThread()
     currentThread.uncaughtExceptionHandler.uncaughtException(currentThread, exception)

core/kotlinx-coroutines-core/test/exceptions/CoroutineExceptionHandlerJvmTest.kt

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2016-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
+package kotlinx.coroutines.experimental.exceptions
+
+import kotlinx.coroutines.experimental.*
+import org.junit.*
+import org.junit.Test
+import kotlin.test.*
+
+class CoroutineExceptionHandlerJvmTest : TestBase() {
+
+    private val exceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
+    private lateinit var caughtException: Throwable
+
+    @Before
+    fun setUp() {
+        Thread.setDefaultUncaughtExceptionHandler({ _, e -> caughtException = e })
+    }
+
+    @After
+    fun tearDown() {
+        Thread.setDefaultUncaughtExceptionHandler(exceptionHandler)
+    }
+
+    @Test
+    fun testFailingHandler() = runBlocking {
+        expect(1)
+        val job = GlobalScope.launch(CoroutineExceptionHandler { _, _ -> throw AssertionError() }) {
+            expect(2)
+            throw TestException()
+        }
+
+        job.join()
+        assertTrue(caughtException is RuntimeException)
+        assertTrue(caughtException.cause is AssertionError)
+        assertTrue(caughtException.suppressed()[0] is TestException)
+
+        finish(3)
+    }
+
+    private class TestException : Throwable()
+}