Skip to content

Simplify internal handling of suppressed exceptions #4007

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
Jan 15, 2024
Merged
Show file tree
Hide file tree
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
Expand Up @@ -35,7 +35,7 @@ public fun handleCoroutineException(context: CoroutineContext, exception: Throwa
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)
addSuppressed(originalException)
}
}

Expand Down
1 change: 0 additions & 1 deletion kotlinx-coroutines-core/common/src/Exceptions.common.kt
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,5 @@ internal expect class JobCancellationException(

internal class CoroutinesInternalError(message: String, cause: Throwable) : Error(message, cause)

internal expect fun Throwable.addSuppressedThrowable(other: Throwable)
// For use in tests
internal expect val RECOVER_STACK_TRACES: Boolean
4 changes: 2 additions & 2 deletions kotlinx-coroutines-core/common/src/JobSupport.kt
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
val unwrapped = unwrap(exception)
if (unwrapped !== rootCause && unwrapped !== unwrappedCause &&
unwrapped !is CancellationException && seenExceptions.add(unwrapped)) {
rootCause.addSuppressedThrowable(unwrapped)
rootCause.addSuppressed(unwrapped)
}
}
}
Expand Down Expand Up @@ -369,7 +369,7 @@ public open class JobSupport constructor(active: Boolean) : Job, ChildJob, Paren
try {
node.invoke(cause)
} catch (ex: Throwable) {
exception?.apply { addSuppressedThrowable(ex) } ?: run {
exception?.apply { addSuppressed(ex) } ?: run {
exception = CompletionHandlerException("Exception in completion handler $node for $this", ex)
}
}
Expand Down
2 changes: 1 addition & 1 deletion kotlinx-coroutines-core/common/src/channels/Deprecated.kt
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ internal fun consumesAll(vararg channels: ReceiveChannel<*>): CompletionHandler
if (exception == null) {
exception = e
} else {
exception.addSuppressedThrowable(e)
exception.addSuppressed(e)
}
}
exception?.let { throw it }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ private suspend fun <T> FlowCollector<T>.invokeSafely(
try {
action(cause)
} catch (e: Throwable) {
if (cause !== null && cause !== e) e.addSuppressedThrowable(cause)
if (cause !== null && cause !== e) e.addSuppressed(cause)
throw e
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ internal abstract class DispatchedTask<in T> internal constructor(
internal fun handleFatalException(exception: Throwable?, finallyException: Throwable?) {
if (exception === null && finallyException === null) return
if (exception !== null && finallyException !== null) {
exception.addSuppressedThrowable(finallyException)
exception.addSuppressed(finallyException)
}

val cause = exception ?: finallyException
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ internal fun <E> OnUndeliveredElement<E>.callUndeliveredElementCatchingException
// undeliveredElementException.cause !== ex is an optimization in case the same exception is thrown
// over and over again by on OnUndeliveredElement
if (undeliveredElementException != null && undeliveredElementException.cause !== ex) {
undeliveredElementException.addSuppressedThrowable(ex)
undeliveredElementException.addSuppressed(ex)
} else {
return UndeliveredElementException("Exception in undelivered element handler for $element", ex)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class ConcurrentExceptionsStressTest : TestBase() {
val completionException = deferred.getCompletionExceptionOrNull()
val cause = completionException as? StressException
?: unexpectedException("completion", completionException)
val suppressed = cause.suppressed
val suppressed = cause.suppressedExceptions
val indices = listOf(cause.index) + suppressed.mapIndexed { index, e ->
(e as? StressException)?.index ?: unexpectedException("suppressed $index", e)
}
Expand All @@ -62,6 +62,6 @@ class ConcurrentExceptionsStressTest : TestBase() {
throw IllegalStateException("Unexpected $msg exception", e)
}

private class StressException(val index: Int) : SuppressSupportingThrowable()
private class StressException(val index: Int) : Throwable()
}

Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,26 @@
package kotlinx.coroutines.exceptions

import kotlinx.coroutines.*
import kotlin.concurrent.Volatile
import kotlin.random.*

internal expect open class SuppressSupportingThrowable() : Throwable
expect val Throwable.suppressed: Array<Throwable>
// Unused on purpose, used manually during debugging sessions
expect fun Throwable.printStackTrace()
fun randomWait() {
val n = Random.nextInt(1000)
if (n < 500) return // no wait 50% of time
repeat(n) {
BlackHole.sink *= 3
}
// use the BlackHole value somehow, so even if the compiler gets smarter, it won't remove the object
val sinkValue = if (BlackHole.sink > 16) 1 else 0
if (n + sinkValue > 900) yieldThread()
}

private object BlackHole {
@Volatile
var sink = 1
}

expect fun randomWait()
expect inline fun yieldThread()

expect fun currentThreadName(): String

Expand Down
3 changes: 0 additions & 3 deletions kotlinx-coroutines-core/jsAndWasmShared/src/Exceptions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,5 @@ internal actual class JobCancellationException public actual constructor(
(message!!.hashCode() * 31 + job.hashCode()) * 31 + (cause?.hashCode() ?: 0)
}

@Suppress("NOTHING_TO_INLINE")
internal actual inline fun Throwable.addSuppressedThrowable(other: Throwable) { /* empty */ }

// For use in tests
internal actual val RECOVER_STACK_TRACES: Boolean = false
4 changes: 0 additions & 4 deletions kotlinx-coroutines-core/jvm/src/Exceptions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,3 @@ internal actual class JobCancellationException public actual constructor(
override fun hashCode(): Int =
(message!!.hashCode() * 31 + job.hashCode()) * 31 + (cause?.hashCode() ?: 0)
}

@Suppress("NOTHING_TO_INLINE")
internal actual inline fun Throwable.addSuppressedThrowable(other: Throwable) =
addSuppressed(other)
22 changes: 1 addition & 21 deletions kotlinx-coroutines-core/jvm/test/ConcurrentTestUtilities.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,6 @@

package kotlinx.coroutines.exceptions

import kotlin.random.*

actual fun randomWait() {
val n = Random.nextInt(1000)
if (n < 500) return // no wait 50% of time
repeat(n) {
BlackHole.sink *= 3
}
if (n > 900) Thread.yield()
}

private object BlackHole {
@Volatile
var sink = 1
}

@Suppress("ACTUAL_WITHOUT_EXPECT")
internal actual typealias SuppressSupportingThrowable = Throwable

@Suppress("EXTENSION_SHADOWED_BY_MEMBER", "unused")
actual fun Throwable.printStackTrace() = printStackTrace()
actual inline fun yieldThread() { Thread.yield() }

actual fun currentThreadName(): String = Thread.currentThread().name
11 changes: 0 additions & 11 deletions kotlinx-coroutines-core/jvm/test/exceptions/Exceptions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,6 @@ import java.util.*
import kotlin.coroutines.*
import kotlin.test.*

/**
* Proxy for [Throwable.getSuppressed] for tests, which are compiled for both JDK 1.6 and JDK 1.8,
* but run only under JDK 1.8
*/
@Suppress("ConflictingExtensionProperty")
actual val Throwable.suppressed: Array<Throwable> get() {
val method = this::class.java.getMethod("getSuppressed") ?: error("This test can only be run using JDK 1.7")
@Suppress("UNCHECKED_CAST")
return method.invoke(this) as Array<Throwable>
}

internal inline fun <reified T : Throwable> checkException(exception: Throwable): Boolean {
assertTrue(exception is T)
assertTrue(exception.suppressed.isEmpty())
Expand Down
8 changes: 0 additions & 8 deletions kotlinx-coroutines-core/native/src/Exceptions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@

package kotlinx.coroutines

import kotlinx.coroutines.internal.*
import kotlinx.coroutines.internal.SuppressSupportingThrowableImpl

/**
* Thrown by cancellable suspending functions if the [Job] of the coroutine is cancelled while it is suspending.
* It indicates _normal_ cancellation of a coroutine.
Expand Down Expand Up @@ -38,10 +35,5 @@ internal actual class JobCancellationException public actual constructor(
(message!!.hashCode() * 31 + job.hashCode()) * 31 + (cause?.hashCode() ?: 0)
}

@Suppress("NOTHING_TO_INLINE")
internal actual inline fun Throwable.addSuppressedThrowable(other: Throwable) {
if (this is SuppressSupportingThrowableImpl) addSuppressed(other)
}

// For use in tests
internal actual val RECOVER_STACK_TRACES: Boolean = false
15 changes: 0 additions & 15 deletions kotlinx-coroutines-core/native/src/internal/Concurrent.kt
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,5 @@ internal actual inline fun <T> ReentrantLock.withLock(action: () -> T): T = this

internal actual fun <E> identitySet(expectedSize: Int): MutableSet<E> = HashSet()


// "Suppress-supporting throwable" is currently used for tests only
internal open class SuppressSupportingThrowableImpl : Throwable() {
private val _suppressed = atomic<Array<Throwable>>(emptyArray())

val suppressed: Array<Throwable>
get() = _suppressed.value

fun addSuppressed(other: Throwable) {
_suppressed.update { current ->
current + other
}
}
}

@Suppress("ACTUAL_WITHOUT_EXPECT") // This suppress can be removed in 2.0: KT-59355
internal actual typealias BenignDataRace = kotlin.concurrent.Volatile
26 changes: 1 addition & 25 deletions kotlinx-coroutines-core/native/test/ConcurrentTestUtilities.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,9 @@

package kotlinx.coroutines.exceptions

import kotlinx.coroutines.internal.*
import platform.posix.*
import kotlin.native.concurrent.*
import kotlin.random.*

actual fun randomWait() {
val n = Random.nextInt(1000)
if (n < 500) return // no wait 50% of time
repeat(n) {
BlackHole.sink *= 3
}
// use the BlackHole value somehow, so even if the compiler gets smarter, it won't remove the object
val sinkValue = if (BlackHole.sink > 16) 1 else 0
if (n + sinkValue > 900) sched_yield()
}

@ThreadLocal
private object BlackHole {
var sink = 1
}

internal actual typealias SuppressSupportingThrowable = SuppressSupportingThrowableImpl

actual val Throwable.suppressed: Array<Throwable>
get() = (this as? SuppressSupportingThrowableImpl)?.suppressed ?: emptyArray()

@Suppress("EXTENSION_SHADOWED_BY_MEMBER", "unused")
actual fun Throwable.printStackTrace() = printStackTrace()
actual inline fun yieldThread() { sched_yield() }

actual fun currentThreadName(): String = Worker.current.name