Skip to content

In the test module, collect and report the actual uncaught exceptions #3449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 24, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -27,7 +27,8 @@ class ListAllCoroutineThrowableSubclassesTest {
"kotlinx.coroutines.JobCancellationException",
"kotlinx.coroutines.internal.UndeliveredElementException",
"kotlinx.coroutines.CompletionHandlerException",
"kotlinx.coroutines.DiagnosticCoroutineContextException",
"kotlinx.coroutines.internal.DiagnosticCoroutineContextException",
"kotlinx.coroutines.internal.ExceptionSuccessfullyProcessed",
"kotlinx.coroutines.CoroutinesInternalError",
"kotlinx.coroutines.channels.ClosedSendChannelException",
"kotlinx.coroutines.channels.ClosedReceiveChannelException",
Original file line number Diff line number Diff line change
@@ -4,10 +4,9 @@

package kotlinx.coroutines

import kotlinx.coroutines.internal.*
import kotlin.coroutines.*

internal expect fun handleCoroutineExceptionImpl(context: CoroutineContext, exception: Throwable)

/**
* Helper function for coroutine builder implementations to handle uncaught and unexpected exceptions in coroutines,
* that could not be otherwise handled in a normal way through structured concurrency, saving them to a future, and
@@ -26,11 +25,11 @@ public fun handleCoroutineException(context: CoroutineContext, exception: Throwa
return
}
} catch (t: Throwable) {
handleCoroutineExceptionImpl(context, handlerException(exception, t))
handleUncaughtCoroutineException(context, handlerException(exception, t))
return
}
// If a handler is not present in the context or an exception was thrown, fallback to the global handler
handleCoroutineExceptionImpl(context, exception)
handleUncaughtCoroutineException(context, exception)
}

internal fun handlerException(originalException: Throwable, thrownException: Throwable): Throwable {
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/

package kotlinx.coroutines.internal

import kotlinx.coroutines.*
import kotlin.coroutines.*

/**
* The list of globally installed [CoroutineExceptionHandler] instances that will be notified of any exceptions that
* were not processed in any other manner.
*/
internal expect val platformExceptionHandlers: Collection<CoroutineExceptionHandler>

/**
* Ensures that the given [callback] is present in the [platformExceptionHandlers] list.
*/
internal expect fun ensurePlatformExceptionHandlerLoaded(callback: CoroutineExceptionHandler)

/**
* The platform-dependent global exception handler, used so that the exception is logged at least *somewhere*.
*/
internal expect fun propagateExceptionFinalResort(exception: Throwable)

/**
* 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))
}
}

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()
12 changes: 0 additions & 12 deletions kotlinx-coroutines-core/js/src/CoroutineExceptionHandlerImpl.kt

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/

package kotlinx.coroutines.internal

import kotlinx.coroutines.*
import kotlin.coroutines.*

private val platformExceptionHandlers_ = mutableSetOf<CoroutineExceptionHandler>()

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())

62 changes: 0 additions & 62 deletions kotlinx-coroutines-core/jvm/src/CoroutineExceptionHandlerImpl.kt

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/

package kotlinx.coroutines.internal

import java.util.*
import kotlinx.coroutines.*
import kotlin.coroutines.*

/**
* A list of globally installed [CoroutineExceptionHandler] instances.
*
* Note that Android may have dummy [Thread.contextClassLoader] which is used by one-argument [ServiceLoader.load] function,
* see (https://stackoverflow.com/questions/13407006/android-class-loader-may-fail-for-processes-that-host-multiple-applications).
* So here we explicitly use two-argument `load` with a class-loader of [CoroutineExceptionHandler] class.
*
* 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.
*/
internal actual val platformExceptionHandlers: Collection<CoroutineExceptionHandler> = ServiceLoader.load(
CoroutineExceptionHandler::class.java,
CoroutineExceptionHandler::class.java.classLoader
).iterator().asSequence().toList()

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()
}

override fun fillInStackTrace(): Throwable {
// Prevent Android <= 6.0 bug, #1866
stackTrace = emptyArray()
return this
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/

package kotlinx.coroutines.internal

import kotlinx.coroutines.*
import kotlin.coroutines.*
import kotlin.native.*

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 propagateExceptionFinalResort(exception: Throwable) {
// log exception
processUnhandledException(exception)
}

internal actual class DiagnosticCoroutineContextException actual constructor(context: CoroutineContext) :
RuntimeException(context.toString())
12 changes: 12 additions & 0 deletions kotlinx-coroutines-test/common/src/TestScope.kt
Original file line number Diff line number Diff line change
@@ -226,6 +226,14 @@ internal class TestScopeImpl(context: CoroutineContext) :
throw IllegalStateException("Only a single call to `runTest` can be performed during one test.")
entered = true
check(!finished)
/** the order is important: [reportException] is only guaranteed not to throw if [entered] is `true` but
* [finished] is `false`.
* However, we also want [uncaughtExceptions] to be queried after the callback is registered,
* 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 { ensurePlatformExceptionHandlerLoaded(ExceptionCollector) }
ExceptionCollector.addOnExceptionCallback(lock, this::reportException)
uncaughtExceptions
}
if (exceptions.isNotEmpty()) {
@@ -239,6 +247,8 @@ internal class TestScopeImpl(context: CoroutineContext) :
/** Called at the end of the test. May only be called once. Returns the list of caught unhandled exceptions. */
fun leave(): List<Throwable> = synchronized(lock) {
check(entered && !finished)
/** After [finished] becomes `true`, it is no longer valid to have [reportException] as the callback. */
ExceptionCollector.removeOnExceptionCallback(lock)
finished = true
uncaughtExceptions
}
@@ -247,6 +257,8 @@ internal class TestScopeImpl(context: CoroutineContext) :
fun legacyLeave(): List<Throwable> {
val exceptions = synchronized(lock) {
check(entered && !finished)
/** After [finished] becomes `true`, it is no longer valid to have [reportException] as the callback. */
ExceptionCollector.removeOnExceptionCallback(lock)
Copy link
Member

@qwwdfsad qwwdfsad Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like callbacks are never removed for non-legacy version, consuming the memory lineary to number of tests run, could you please address that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. You're right, of course. I simply rebased the code on develop but totally forgot that the leave function has split in two and needs changes in both places.

finished = true
uncaughtExceptions
}
98 changes: 98 additions & 0 deletions kotlinx-coroutines-test/common/src/internal/ExceptionCollector.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/

package kotlinx.coroutines.test.internal

import kotlinx.coroutines.*
import kotlinx.coroutines.internal.*
import kotlin.coroutines.*

/**
* 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.
*/
internal object ExceptionCollector : AbstractCoroutineContextElement(CoroutineExceptionHandler), CoroutineExceptionHandler {
private val lock = SynchronizedObject()
private var enabled = false
private val unprocessedExceptions = mutableListOf<Throwable>()
private val callbacks = mutableMapOf<Any, (Throwable) -> Unit>()

/**
* 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)
check(previousValue === null)
// try to process the exceptions using the newly-registered callback
unprocessedExceptions.forEach { reportException(it) }
unprocessedExceptions.clear()
}

/**
* Unregisters the callback associated with [owner].
*/
fun removeOnExceptionCallback(owner: Any) = synchronized(lock) {
val existingValue = callbacks.remove(owner)
check(existingValue !== null)
}

/**
* 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
}

/**
* 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. */
}
return executedACallback
}

@Suppress("INVISIBLE_MEMBER")
override fun handleException(context: CoroutineContext, exception: Throwable) {
if (handleException(exception)) {
throw ExceptionSuccessfullyProcessed
}
}

override fun equals(other: Any?): Boolean = other is ExceptionCollector || other is ExceptionCollectorAsService
}

/**
* A workaround for being unable to treat an object as a `ServiceLoader` service.
*/
internal class ExceptionCollectorAsService: CoroutineExceptionHandler by ExceptionCollector {
override fun equals(other: Any?): Boolean = other is ExceptionCollectorAsService || other is ExceptionCollector
override fun hashCode(): Int = ExceptionCollector.hashCode()
}
27 changes: 25 additions & 2 deletions kotlinx-coroutines-test/common/test/Helpers.kt
Original file line number Diff line number Diff line change
@@ -31,9 +31,32 @@ inline fun <T> assertRunsFast(timeout: Duration, block: () -> T): T {
inline fun <T> assertRunsFast(block: () -> T): T = assertRunsFast(2.seconds, block)

/**
* Passes [test] as an argument to [block], but as a function returning not a [TestResult] but [Unit].
* Runs [test], and then invokes [block], passing to it the lambda that functionally behaves
* the same way [test] does.
*/
expect fun testResultMap(block: (() -> Unit) -> Unit, test: () -> TestResult): TestResult
fun testResultMap(block: (() -> Unit) -> Unit, test: () -> TestResult): TestResult = testResultChain(
block = test,
after = {
block { it.getOrThrow() }
createTestResult { }
}
)

/**
* Chains together [block] and [after], passing the result of [block] to [after].
*/
expect fun testResultChain(block: () -> TestResult, after: (Result<Unit>) -> TestResult): TestResult

fun testResultChain(vararg chained: (Result<Unit>) -> TestResult): TestResult =
if (chained.isEmpty()) {
createTestResult { }
} else {
testResultChain(block = {
chained[0](Result.success(Unit))
}) {
testResultChain(*chained.drop(1).toTypedArray())
}
}

class TestException(message: String? = null): Exception(message)

56 changes: 56 additions & 0 deletions kotlinx-coroutines-test/common/test/TestScopeTest.kt
Original file line number Diff line number Diff line change
@@ -491,6 +491,62 @@ class TestScopeTest {
}
}

/*
* Tests that the [TestScope] exception reporting mechanism will report the exceptions that happen between
* different tests.
*
* This test must be ran manually, because such exceptions still go through the global exception handler
* (as there's no guarantee that another test will happen), and the global exception handler will
* log the exceptions or, on Native, crash the test suite.
*/
@Test
@Ignore
fun testReportingStrayUncaughtExceptionsBetweenTests() {
val thrown = TestException("x")
testResultChain({
// register a handler for uncaught exceptions
runTest { }
}, {
GlobalScope.launch(start = CoroutineStart.UNDISPATCHED) {
throw thrown
}
runTest {
fail("unreached")
}
}, {
// this `runTest` will not report the exception
runTest {
when (val exception = it.exceptionOrNull()) {
is UncaughtExceptionsBeforeTest -> {
assertEquals(1, exception.suppressedExceptions.size)
assertSame(exception.suppressedExceptions[0], thrown)
}
else -> fail("unexpected exception: $exception")
}
}
})
}

/**
* Tests that the uncaught exceptions that happen during the test are reported.
*/
@Test
fun testReportingStrayUncaughtExceptionsDuringTest(): TestResult {
val thrown = TestException("x")
return testResultChain({ _ ->
runTest {
val job = launch(Dispatchers.Default + NonCancellable) {
throw thrown
}
job.join()
}
}, {
runTest {
assertEquals(thrown, it.exceptionOrNull())
}
})
}

companion object {
internal val invalidContexts = listOf(
Dispatchers.Default, // not a [TestDispatcher]
13 changes: 5 additions & 8 deletions kotlinx-coroutines-test/js/test/Helpers.kt
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
/*
* Copyright 2016-2021 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
* Copyright 2016-2022 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
*/

package kotlinx.coroutines.test

import kotlin.test.*

actual fun testResultMap(block: (() -> Unit) -> Unit, test: () -> TestResult): TestResult =
test().then(
actual fun testResultChain(block: () -> TestResult, after: (Result<Unit>) -> TestResult): TestResult =
block().then(
{
block {
}
after(Result.success(Unit))
}, {
block {
throw it
}
after(Result.failure(it))
})

actual typealias NoJs = Ignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
kotlinx.coroutines.test.internal.ExceptionCollectorAsService
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should have the corresponding provides in module-info.java

Original file line number Diff line number Diff line change
@@ -59,7 +59,7 @@ public class TestCoroutineExceptionHandler :
override fun handleException(context: CoroutineContext, exception: Throwable) {
synchronized(_lock) {
if (_coroutinesCleanedUp) {
handleCoroutineExceptionImpl(context, exception)
handleUncaughtCoroutineException(context, exception)
}
_exceptions += exception
}
3 changes: 3 additions & 0 deletions kotlinx-coroutines-test/jvm/src/module-info.java
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import kotlinx.coroutines.CoroutineExceptionHandler;
import kotlinx.coroutines.internal.MainDispatcherFactory;
import kotlinx.coroutines.test.internal.ExceptionCollectorAsService;
import kotlinx.coroutines.test.internal.TestMainDispatcherFactory;

module kotlinx.coroutines.test {
@@ -8,4 +10,5 @@
exports kotlinx.coroutines.test;

provides MainDispatcherFactory with TestMainDispatcherFactory;
provides CoroutineExceptionHandler with ExceptionCollectorAsService;
}
9 changes: 6 additions & 3 deletions kotlinx-coroutines-test/jvm/test/HelpersJvm.kt
Original file line number Diff line number Diff line change
@@ -3,8 +3,11 @@
*/
package kotlinx.coroutines.test

actual fun testResultMap(block: (() -> Unit) -> Unit, test: () -> TestResult) {
block {
test()
actual fun testResultChain(block: () -> TestResult, after: (Result<Unit>) -> TestResult): TestResult {
try {
block()
after(Result.success(Unit))
} catch (e: Throwable) {
after(Result.failure(e))
}
}
9 changes: 6 additions & 3 deletions kotlinx-coroutines-test/native/test/Helpers.kt
Original file line number Diff line number Diff line change
@@ -5,9 +5,12 @@ package kotlinx.coroutines.test

import kotlin.test.*

actual fun testResultMap(block: (() -> Unit) -> Unit, test: () -> TestResult) {
block {
test()
actual fun testResultChain(block: () -> TestResult, after: (Result<Unit>) -> TestResult): TestResult {
try {
block()
after(Result.success(Unit))
} catch (e: Throwable) {
after(Result.failure(e))
}
}