Move ExceptionCollector to the test module

Author: dkhalanskyjb

integration-testing/src/jvmCoreTest/kotlin/ListAllCoroutineThrowableSubclassesTest.kt

@@ -28,6 +28,7 @@ class ListAllCoroutineThrowableSubclassesTest {
         "kotlinx.coroutines.internal.UndeliveredElementException",
         "kotlinx.coroutines.CompletionHandlerException",
         "kotlinx.coroutines.internal.DiagnosticCoroutineContextException",
+        "kotlinx.coroutines.internal.ExceptionSuccessfullyProcessed",
         "kotlinx.coroutines.CoroutinesInternalError",
         "kotlinx.coroutines.channels.ClosedSendChannelException",
         "kotlinx.coroutines.channels.ClosedReceiveChannelException",

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

@@ -7,89 +7,66 @@ package kotlinx.coroutines.internal
 import kotlinx.coroutines.*
 import kotlin.coroutines.*
 
-internal expect val ANDROID_DETECTED: Boolean
-
-internal expect fun propagateExceptionToPlatform(context: CoroutineContext, exception: Throwable)
-
 /**
- * If [addOnExceptionCallback] is called, the provided callback will be evaluated each time
- * [handleCoroutineException] is executed and can't find a [CoroutineExceptionHandler] to
- * process the exception.
- *
- * When a callback is registered once, even if it's later removed, the system starts to assume that
- * other callbacks will eventually be registered, and so collects the exceptions.
- * Once a new callback is registered, the collected exceptions are used with it.
- *
- * The callbacks in this object are the last resort before relying on platform-dependent
- * ways to report uncaught exceptions from coroutines.
+ * The list of globally installed [CoroutineExceptionHandler] instances that will be notified of any exceptions that
+ * were not processed in any other manner.
  */
-@PublishedApi
-internal object ExceptionCollector {
-    private val lock = SynchronizedObject()
-    private var enabled = false
-    private var unprocessedExceptions = mutableListOf<Throwable>()
-    private val callbacks = mutableMapOf<Any, (Throwable) -> Unit>()
+internal expect val platformExceptionHandlers: Collection<CoroutineExceptionHandler>
 
-    /**
-     * Registers [callback] to be executed when an uncaught exception happens.
-     * [owner] is a key by which to distinguish different callbacks.
-     */
-    fun addOnExceptionCallback(owner: Any, callback: (Throwable) -> Unit) = synchronized(lock) {
-        enabled = true // never becomes `false` again
-        val previousValue = callbacks.put(owner, callback)
-        assert { previousValue === null }
-        // try to process the exceptions using the newly-registered callback
-        unprocessedExceptions.forEach { reportException(it) }
-        unprocessedExceptions = mutableListOf()
-    }
-
-    /**
-     * Unregisters the callback associated with [owner].
-     */
-    fun removeOnExceptionCallback(owner: Any) = synchronized(lock) {
-        val existingValue = callbacks.remove(owner)
-        assert { existingValue !== null }
-    }
+/**
+ * Ensures that the given [callback] is present in the [platformExceptionHandlers] list.
+ */
+internal expect fun ensurePlatformExceptionHandlerLoaded(callback: CoroutineExceptionHandler)
 
-    /**
-     * Tries to handle the exception by propagating it to an interested consumer.
-     * Returns `true` if the exception does not need further processing.
-     *
-     * Doesn't throw.
-     */
-    fun handleException(exception: Throwable): Boolean = synchronized(lock) {
-        if (!enabled) return false
-        if (reportException(exception)) return true
-        /** we don't return the result of the `add` function because we don't have a guarantee
-         * that a callback will eventually appear and collect the unprocessed exceptions, so
-         * we can't consider [exception] to be properly handled. */
-        unprocessedExceptions.add(exception)
-        return false
-    }
+/**
+ * The platform-dependent global exception handler, used so that the exception is logged at least *somewhere*.
+ */
+internal expect fun propagateExceptionFinalResort(exception: Throwable)
 
-    /**
-     * Try to report [exception] to the existing callbacks.
-     */
-    private fun reportException(exception: Throwable): Boolean {
-        var executedACallback = false
-        for (callback in callbacks.values) {
-            callback(exception)
-            executedACallback = true
-            /** We don't leave the function here because we want to fan-out the exceptions to every interested consumer,
-             * it's not enough to have the exception processed by one of them.
-             * The reason is, it's less big of a deal to observe multiple concurrent reports of bad behavior than not
-             * to observe the report in the exact callback that is connected to that bad behavior. */
+/**
+ * Deal with exceptions that happened in coroutines and weren't programmatically dealt with.
+ *
+ * First, it notifies every [CoroutineExceptionHandler] in the [platformExceptionHandlers] list.
+ * If one of them throws [ExceptionSuccessfullyProcessed], it means that that handler believes that the exception was
+ * dealt with sufficiently well and doesn't need any further processing.
+ * Otherwise, the platform-dependent global exception handler is also invoked.
+ */
+internal fun handleUncaughtCoroutineException(context: CoroutineContext, exception: Throwable) {
+    // use additional extension handlers
+    for (handler in platformExceptionHandlers) {
+        try {
+            handler.handleException(context, exception)
+        } catch (_: ExceptionSuccessfullyProcessed) {
+            return
+        } catch (t: Throwable) {
+            propagateExceptionFinalResort(handlerException(exception, t))
         }
-        return executedACallback
     }
-}
 
-internal fun handleUncaughtCoroutineException(context: CoroutineContext, exception: Throwable) {
-    /** this check is purely for the whole [ExceptionCollector] to be eliminated when an Android app is minified. */
-    if (ANDROID_DETECTED) {
-        propagateExceptionToPlatform(context, exception)
-    } else {
-        if (!ExceptionCollector.handleException(exception))
-            propagateExceptionToPlatform(context, exception)
+    try {
+        exception.addSuppressed(DiagnosticCoroutineContextException(context))
+    } catch (e: Throwable) {
+        // addSuppressed is never user-defined and cannot normally throw with the only exception being OOM
+        // we do ignore that just in case to definitely deliver the exception
     }
+    propagateExceptionFinalResort(exception)
 }
+
+/**
+ * Private exception that is added to suppressed exceptions of the original exception
+ * when it is reported to the last-ditch current thread 'uncaughtExceptionHandler'.
+ *
+ * The purpose of this exception is to add an otherwise inaccessible diagnostic information and to
+ * be able to poke the context of the failing coroutine in the debugger.
+ */
+internal expect class DiagnosticCoroutineContextException(context: CoroutineContext) : RuntimeException
+
+/**
+ * A dummy exception that signifies that the exception was successfully processed by the handler and no further
+ * action is required.
+ *
+ * Would be nicer if [CoroutineExceptionHandler] could return a boolean, but that would be a breaking change.
+ * For now, we will take solace in knowledge that such exceptions are exceedingly rare, even rarer than globally
+ * uncaught exceptions in general.
+ */
+internal object ExceptionSuccessfullyProcessed: Exception()

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

@@ -4,11 +4,23 @@
 
 package kotlinx.coroutines.internal
 
+import kotlinx.coroutines.*
 import kotlin.coroutines.*
 
-internal actual val ANDROID_DETECTED = false
+private val platformExceptionHandlers_ = mutableSetOf<CoroutineExceptionHandler>()
 
-internal actual fun propagateExceptionToPlatform(context: CoroutineContext, exception: Throwable) {
+internal actual val platformExceptionHandlers: Collection<CoroutineExceptionHandler>
+    get() = platformExceptionHandlers_
+
+internal actual fun ensurePlatformExceptionHandlerLoaded(callback: CoroutineExceptionHandler) {
+    platformExceptionHandlers_ += callback
+}
+
+internal actual fun propagateExceptionFinalResort(exception: Throwable) {
     // log exception
     console.error(exception)
 }
+
+internal actual class DiagnosticCoroutineContextException actual constructor(context: CoroutineContext) :
+    RuntimeException(context.toString())
+

kotlinx-coroutines-core/jvm/src/internal/CoroutineExceptionHandlerImplJvm.kt

@@ -5,8 +5,8 @@
 package kotlinx.coroutines.internal
 
 import java.util.*
-import kotlin.coroutines.*
 import kotlinx.coroutines.*
+import kotlin.coroutines.*
 
 /**
  * A list of globally installed [CoroutineExceptionHandler] instances.
@@ -18,19 +18,25 @@ import kotlinx.coroutines.*
  * We are explicitly using the `ServiceLoader.load(MyClass::class.java, MyClass::class.java.classLoader).iterator()`
  * form of the ServiceLoader call to enable R8 optimization when compiled on Android.
  */
-private val handlers: List<CoroutineExceptionHandler> = ServiceLoader.load(
-        CoroutineExceptionHandler::class.java,
-        CoroutineExceptionHandler::class.java.classLoader
+internal actual val platformExceptionHandlers: Collection<CoroutineExceptionHandler> = ServiceLoader.load(
+    CoroutineExceptionHandler::class.java,
+    CoroutineExceptionHandler::class.java.classLoader
 ).iterator().asSequence().toList()
 
-/**
- * Private exception without stacktrace that is added to suppressed exceptions of the original exception
- * when it is reported to the last-ditch current thread 'uncaughtExceptionHandler'.
- *
- * The purpose of this exception is to add an otherwise inaccessible diagnostic information and to
- * be able to poke the failing coroutine context in the debugger.
- */
-private class DiagnosticCoroutineContextException(@Transient private val context: CoroutineContext) : RuntimeException() {
+internal actual fun ensurePlatformExceptionHandlerLoaded(callback: CoroutineExceptionHandler) {
+    // we use JVM's mechanism of ServiceLoader, so this should be a no-op on JVM.
+    // The only thing we do is make sure that the ServiceLoader did work correctly.
+    check(callback in platformExceptionHandlers) { "Exception handler was not found via a ServiceLoader" }
+}
+
+internal actual fun propagateExceptionFinalResort(exception: Throwable) {
+    // use the thread's handler
+    val currentThread = Thread.currentThread()
+    currentThread.uncaughtExceptionHandler.uncaughtException(currentThread, exception)
+}
+
+// This implementation doesn't store a stacktrace, which is good because a stacktrace doesn't make sense for this.
+internal actual class DiagnosticCoroutineContextException actual constructor(@Transient private val context: CoroutineContext) : RuntimeException() {
     override fun getLocalizedMessage(): String {
         return context.toString()
     }
@@ -41,23 +47,3 @@ private class DiagnosticCoroutineContextException(@Transient private val context
         return this
     }
 }
-
-internal actual fun propagateExceptionToPlatform(context: CoroutineContext, exception: Throwable) {
-    // use additional extension handlers
-    for (handler in handlers) {
-        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()
-    // addSuppressed is never user-defined and cannot normally throw with the only exception being OOM
-    // we do ignore that just in case to definitely deliver the exception
-    runCatching { exception.addSuppressed(DiagnosticCoroutineContextException(context)) }
-    currentThread.uncaughtExceptionHandler.uncaughtException(currentThread, exception)
-}

kotlinx-coroutines-core/jvm/src/internal/FastServiceLoader.kt

@@ -14,7 +14,7 @@ import kotlin.collections.ArrayList
 /**
  * Don't use JvmField here to enable R8 optimizations via "assumenosideeffects"
  */
-internal actual val ANDROID_DETECTED = runCatching { Class.forName("android.os.Build") }.isSuccess
+internal val ANDROID_DETECTED = runCatching { Class.forName("android.os.Build") }.isSuccess
 
 /**
  * A simplified version of [ServiceLoader].

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

@@ -4,13 +4,28 @@
 
 package kotlinx.coroutines.internal
 
+import kotlinx.coroutines.*
 import kotlin.coroutines.*
 import kotlin.native.*
 
-internal actual val ANDROID_DETECTED = false
+private val lock = SynchronizedObject()
+
+internal actual val platformExceptionHandlers: Collection<CoroutineExceptionHandler>
+    get() = synchronized(lock) { platformExceptionHandlers_ }
+
+private val platformExceptionHandlers_ = mutableSetOf<CoroutineExceptionHandler>()
+
+internal actual fun ensurePlatformExceptionHandlerLoaded(callback: CoroutineExceptionHandler) {
+    synchronized(lock) {
+        platformExceptionHandlers_ += callback
+    }
+}
 
 @OptIn(ExperimentalStdlibApi::class)
-internal actual fun propagateExceptionToPlatform(context: CoroutineContext, exception: Throwable) {
+internal actual fun propagateExceptionFinalResort(exception: Throwable) {
     // log exception
     processUnhandledException(exception)
 }
+
+internal actual class DiagnosticCoroutineContextException actual constructor(context: CoroutineContext) :
+    RuntimeException(context.toString())

kotlinx-coroutines-test/common/src/TestScope.kt

@@ -226,9 +226,8 @@ internal class TestScopeImpl(context: CoroutineContext) :
              * because the exception collector will be able to report the exceptions that arrived before this test but
              * after the previous one, and learning about such exceptions as soon is possible is nice. */
             @Suppress("INVISIBLE_REFERENCE", "INVISIBLE_MEMBER")
-            run {
-                kotlinx.coroutines.internal.ExceptionCollector.addOnExceptionCallback(lock, this::reportException)
-            }
+            run { ensurePlatformExceptionHandlerLoaded(ExceptionCollector) }
+            ExceptionCollector.addOnExceptionCallback(lock, this::reportException)
             uncaughtExceptions
         }
         if (exceptions.isNotEmpty()) {
@@ -244,10 +243,7 @@ internal class TestScopeImpl(context: CoroutineContext) :
         val exceptions = synchronized(lock) {
             check(entered && !finished)
             /** After [finished] becomes `true`, it is no longer valid to have [reportException] as the callback. */
-            @Suppress("INVISIBLE_REFERENCE", "INVISIBLE_MEMBER")
-            run {
-                kotlinx.coroutines.internal.ExceptionCollector.removeOnExceptionCallback(lock)
-            }
+            ExceptionCollector.removeOnExceptionCallback(lock)
             finished = true
             uncaughtExceptions
         }