From c3a46d63bf0678cf45ada151b9e1af7f32cb9aba Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Mon, 1 May 2023 17:43:29 +0200 Subject: [PATCH 1/4] Properly recover exceptions when they are constructed from 'Throwable(cause)' constructor. And restore the original behaviour. After #1631 this constructor's recovery mechanism was broken because 'Throwable(cause)' changes the message to be equal to 'cause.toString()' which isn't equal to the original message. Also, make reflective constructor choice independable from source-code order Fixes #3714 --- .idea/codeStyles/Project.xml | 2 +- .../jvm/src/internal/ExceptionsConstructor.kt | 39 +++++++++++++++---- .../jvm/src/internal/StackTraceRecovery.kt | 4 -- .../StackTraceRecoveryCustomExceptionsTest.kt | 20 ++++++++++ 4 files changed, 53 insertions(+), 12 deletions(-) diff --git a/.idea/codeStyles/Project.xml b/.idea/codeStyles/Project.xml index 62fd5c7dfd..461a31ed76 100644 --- a/.idea/codeStyles/Project.xml +++ b/.idea/codeStyles/Project.xml @@ -12,4 +12,4 @@ - \ No newline at end of file + diff --git a/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt b/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt index de15225266..8f7a05eecf 100644 --- a/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt +++ b/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt @@ -32,13 +32,29 @@ internal fun tryCopyException(exception: E): E? { private fun createConstructor(clz: Class): Ctor { val nullResult: Ctor = { null } // Pre-cache class - // Skip reflective copy if an exception has additional fields (that are usually populated in user-defined constructors) + // Skip reflective copy if an exception has additional fields (that are typically populated in user-defined constructors) if (throwableFields != clz.fieldsCountOrDefault(0)) return nullResult /* - * Try to reflectively find constructor(), constructor(message, cause), constructor(cause) or constructor(message). - * Exceptions are shared among coroutines, so we should copy exception before recovering current stacktrace. - */ - val constructors = clz.constructors.sortedByDescending { it.parameterTypes.size } + * Try to reflectively find constructor(message, cause), constructor(message), constructor(cause), or constructor() + * Exceptions are shared among coroutines, so we should copy exception before recovering current stacktrace. + * + * By default, Java's reflection iterates over ctors in the source-code order and the sorting is stable, + * so in comparator we are picking the message ctor over the cause one to break such implcit dependency on the source code. + */ + val constructors = clz.constructors.sortedByDescending { + // (m, c) -> 3 + // (m) -> 2 + // (c) -> 1 + // () -> 0 + val params = it.parameterTypes // cloned array + when (params.size) { + 2 -> 3 + 1 -> if (params[0] == String::class.java) 2 else 1 + 0 -> 0 + else -> -1 + } + + } for (constructor in constructors) { val result = createSafeConstructor(constructor) if (result != null) return result @@ -66,8 +82,17 @@ private fun createSafeConstructor(constructor: Constructor<*>): Ctor? { } } -private inline fun safeCtor(crossinline block: (Throwable) -> Throwable): Ctor = - { e -> runCatching { block(e) }.getOrNull() } +private inline fun safeCtor(crossinline block: (Throwable) -> Throwable): Ctor = { e -> + runCatching { + val result = block(e) + /* + * Verify that the new exception has the same message as the original one (bail out if not, see #1631) + * or if the new message complies the contract from `Throwable(cause).message` contract. + */ + if (e.message != result.message && result.message != e.toString()) null + else result + }.getOrNull() +} private fun Class<*>.fieldsCountOrDefault(defaultValue: Int) = kotlin.runCatching { fieldsCount() }.getOrDefault(defaultValue) diff --git a/kotlinx-coroutines-core/jvm/src/internal/StackTraceRecovery.kt b/kotlinx-coroutines-core/jvm/src/internal/StackTraceRecovery.kt index 5193b0dc26..8c3dfb1bb3 100644 --- a/kotlinx-coroutines-core/jvm/src/internal/StackTraceRecovery.kt +++ b/kotlinx-coroutines-core/jvm/src/internal/StackTraceRecovery.kt @@ -84,9 +84,6 @@ private fun recoverFromStackFrame(exception: E, continuation: Co private fun tryCopyAndVerify(exception: E): E? { val newException = tryCopyException(exception) ?: return null - // Verify that the new exception has the same message as the original one (bail out if not, see #1631) - // CopyableThrowable has control over its message and thus can modify it the way it wants - if (exception !is CopyableThrowable<*> && newException.message != exception.message) return null return newException } @@ -157,7 +154,6 @@ private fun mergeRecoveredTraces(recoveredStacktrace: Array, } } -@Suppress("NOTHING_TO_INLINE") internal actual suspend inline fun recoverAndThrow(exception: Throwable): Nothing { if (!RECOVER_STACK_TRACES) throw exception suspendCoroutineUninterceptedOrReturn { diff --git a/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryCustomExceptionsTest.kt b/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryCustomExceptionsTest.kt index d4e19040a5..0f987e56e0 100644 --- a/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryCustomExceptionsTest.kt +++ b/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryCustomExceptionsTest.kt @@ -87,6 +87,26 @@ class StackTraceRecoveryCustomExceptionsTest : TestBase() { assertEquals("Token OK", ex.message) } + @Test + fun testNestedExceptionWithCause() = runTest { + val result = runCatching { + coroutineScope { + throw NestedException(IllegalStateException("ERROR")) + } + } + val ex = result.exceptionOrNull() ?: error("Expected to fail") + assertIs(ex) + assertIs(ex.cause) + val originalCause = ex.cause?.cause + assertIs(originalCause) + assertEquals("ERROR", originalCause.message) + } + + class NestedException : RuntimeException { + constructor(cause: Throwable) : super(cause) + constructor() : super() + } + @Test fun testWrongMessageExceptionInChannel() = runTest { val result = produce(SupervisorJob() + Dispatchers.Unconfined) { From 7584345b6769c5907fb32542e1ca1c784a92b98e Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Tue, 2 May 2023 16:29:31 +0200 Subject: [PATCH 2/4] ~do some inlining and fix STR bug when STR stackframe was accidentally part of recovered exception --- .../jvm/src/internal/StackTraceRecovery.kt | 17 ++++++----------- .../channels/testReceiveFromChannel.txt | 3 +-- .../stacktraces/resume-mode/testUnconfined.txt | 3 +-- .../StackTraceRecoveryChannelsTest.kt | 1 + 4 files changed, 9 insertions(+), 15 deletions(-) diff --git a/kotlinx-coroutines-core/jvm/src/internal/StackTraceRecovery.kt b/kotlinx-coroutines-core/jvm/src/internal/StackTraceRecovery.kt index 8c3dfb1bb3..afc9989646 100644 --- a/kotlinx-coroutines-core/jvm/src/internal/StackTraceRecovery.kt +++ b/kotlinx-coroutines-core/jvm/src/internal/StackTraceRecovery.kt @@ -33,16 +33,16 @@ private val stackTraceRecoveryClassName = runCatching { internal actual fun recoverStackTrace(exception: E): E { if (!RECOVER_STACK_TRACES) return exception // No unwrapping on continuation-less path: exception is not reported multiple times via slow paths - val copy = tryCopyAndVerify(exception) ?: return exception + val copy = tryCopyException(exception) ?: return exception return copy.sanitizeStackTrace() } private fun E.sanitizeStackTrace(): E { val stackTrace = stackTrace val size = stackTrace.size - val lastIntrinsic = stackTrace.frameIndex(stackTraceRecoveryClassName) + val lastIntrinsic = stackTrace.indexOfLast { stackTraceRecoveryClassName == it.className } val startIndex = lastIntrinsic + 1 - val endIndex = stackTrace.frameIndex(baseContinuationImplClassName) + val endIndex = stackTrace.firstFrameIndex(baseContinuationImplClassName) val adjustment = if (endIndex == -1) 0 else size - endIndex val trace = Array(size - lastIntrinsic - adjustment) { if (it == 0) { @@ -70,7 +70,7 @@ private fun recoverFromStackFrame(exception: E, continuation: Co val (cause, recoveredStacktrace) = exception.causeAndStacktrace() // Try to create an exception of the same type and get stacktrace from continuation - val newException = tryCopyAndVerify(cause) ?: return exception + val newException = tryCopyException(cause) ?: return exception // Update stacktrace val stacktrace = createStackTrace(continuation) if (stacktrace.isEmpty()) return exception @@ -82,11 +82,6 @@ private fun recoverFromStackFrame(exception: E, continuation: Co return createFinalException(cause, newException, stacktrace) } -private fun tryCopyAndVerify(exception: E): E? { - val newException = tryCopyException(exception) ?: return null - return newException -} - /* * Here we partially copy original exception stackTrace to make current one much prettier. * E.g. for @@ -106,7 +101,7 @@ private fun tryCopyAndVerify(exception: E): E? { private fun createFinalException(cause: E, result: E, resultStackTrace: ArrayDeque): E { resultStackTrace.addFirst(ARTIFICIAL_FRAME) val causeTrace = cause.stackTrace - val size = causeTrace.frameIndex(baseContinuationImplClassName) + val size = causeTrace.firstFrameIndex(baseContinuationImplClassName) if (size == -1) { result.stackTrace = resultStackTrace.toTypedArray() return result @@ -194,7 +189,7 @@ private fun createStackTrace(continuation: CoroutineStackFrame): ArrayDeque.frameIndex(methodName: String) = indexOfFirst { methodName == it.className } +private fun Array.firstFrameIndex(methodName: String) = indexOfFirst { methodName == it.className } private fun StackTraceElement.elementWiseEquals(e: StackTraceElement): Boolean { /* diff --git a/kotlinx-coroutines-core/jvm/test-resources/stacktraces/channels/testReceiveFromChannel.txt b/kotlinx-coroutines-core/jvm/test-resources/stacktraces/channels/testReceiveFromChannel.txt index 64085ad329..da3558ba30 100644 --- a/kotlinx-coroutines-core/jvm/test-resources/stacktraces/channels/testReceiveFromChannel.txt +++ b/kotlinx-coroutines-core/jvm/test-resources/stacktraces/channels/testReceiveFromChannel.txt @@ -1,5 +1,4 @@ kotlinx.coroutines.RecoverableTestException - at kotlinx.coroutines.internal.StackTraceRecoveryKt.recoverStackTrace(StackTraceRecovery.kt) at kotlinx.coroutines.channels.BufferedChannel.receive$suspendImpl(BufferedChannel.kt) at kotlinx.coroutines.channels.BufferedChannel.receive(BufferedChannel.kt) at kotlinx.coroutines.exceptions.StackTraceRecoveryChannelsTest.channelReceive(StackTraceRecoveryChannelsTest.kt) @@ -7,4 +6,4 @@ kotlinx.coroutines.RecoverableTestException at kotlinx.coroutines.exceptions.StackTraceRecoveryChannelsTest$channelReceive$1.invokeSuspend(StackTraceRecoveryChannelsTest.kt) Caused by: kotlinx.coroutines.RecoverableTestException at kotlinx.coroutines.exceptions.StackTraceRecoveryChannelsTest$testReceiveFromChannel$1$job$1.invokeSuspend(StackTraceRecoveryChannelsTest.kt) - at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt) \ No newline at end of file + at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt) diff --git a/kotlinx-coroutines-core/jvm/test-resources/stacktraces/resume-mode/testUnconfined.txt b/kotlinx-coroutines-core/jvm/test-resources/stacktraces/resume-mode/testUnconfined.txt index a8461556d1..9fc7167833 100644 --- a/kotlinx-coroutines-core/jvm/test-resources/stacktraces/resume-mode/testUnconfined.txt +++ b/kotlinx-coroutines-core/jvm/test-resources/stacktraces/resume-mode/testUnconfined.txt @@ -1,5 +1,4 @@ kotlinx.coroutines.RecoverableTestException - at kotlinx.coroutines.internal.StackTraceRecoveryKt.recoverStackTrace(StackTraceRecovery.kt) at kotlinx.coroutines.channels.BufferedChannel.receive$suspendImpl(BufferedChannel.kt) at kotlinx.coroutines.channels.BufferedChannel.receive(BufferedChannel.kt) at kotlinx.coroutines.exceptions.StackTraceRecoveryResumeModeTest$withContext$2.invokeSuspend(StackTraceRecoveryResumeModeTest.kt) @@ -7,4 +6,4 @@ Caused by: kotlinx.coroutines.RecoverableTestException at kotlinx.coroutines.exceptions.StackTraceRecoveryResumeModeTest.testResumeModeFastPath(StackTraceRecoveryResumeModeTest.kt) at kotlinx.coroutines.exceptions.StackTraceRecoveryResumeModeTest.access$testResumeModeFastPath(StackTraceRecoveryResumeModeTest.kt) at kotlinx.coroutines.exceptions.StackTraceRecoveryResumeModeTest$testUnconfined$1.invokeSuspend(StackTraceRecoveryResumeModeTest.kt) - at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt) \ No newline at end of file + at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt) diff --git a/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt b/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt index 2d8c0ebc75..49f9503371 100644 --- a/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt +++ b/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt @@ -67,6 +67,7 @@ class StackTraceRecoveryChannelsTest : TestBase() { block() expectUnreached() } catch (e: RecoverableTestException) { + e.printStackTrace() verifyStackTrace("channels/${name.methodName}", e) } } From 7ec43340080b13ea74b82153a1f276c9af281ab9 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Tue, 2 May 2023 17:05:55 +0200 Subject: [PATCH 3/4] [PATCH] Simplify the code --- .../jvm/src/internal/ExceptionsConstructor.kt | 65 +++++++------------ 1 file changed, 23 insertions(+), 42 deletions(-) diff --git a/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt b/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt index 8f7a05eecf..03308f6137 100644 --- a/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt +++ b/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt @@ -35,54 +35,35 @@ private fun createConstructor(clz: Class): Ctor { // Skip reflective copy if an exception has additional fields (that are typically populated in user-defined constructors) if (throwableFields != clz.fieldsCountOrDefault(0)) return nullResult /* - * Try to reflectively find constructor(message, cause), constructor(message), constructor(cause), or constructor() + * Try to reflectively find constructor(message, cause), constructor(message), constructor(cause), or constructor(), + * in that order of priority. * Exceptions are shared among coroutines, so we should copy exception before recovering current stacktrace. * - * By default, Java's reflection iterates over ctors in the source-code order and the sorting is stable, - * so in comparator we are picking the message ctor over the cause one to break such implcit dependency on the source code. + * By default, Java's reflection iterates over ctors in the source-code order and the sorting is stable, so we can + * not rely on the order of iteration. Instead, we assign a unique priority to each ctor type. */ - val constructors = clz.constructors.sortedByDescending { - // (m, c) -> 3 - // (m) -> 2 - // (c) -> 1 - // () -> 0 - val params = it.parameterTypes // cloned array - when (params.size) { - 2 -> 3 - 1 -> if (params[0] == String::class.java) 2 else 1 - 0 -> 0 - else -> -1 + return clz.constructors.map { constructor -> + val p = constructor.parameterTypes + when (p.size) { + 2 -> when { + p[0] == String::class.java && p[1] == Throwable::class.java -> + safeCtor { e -> constructor.newInstance(e.message, e) as Throwable } to 3 + else -> null to -1 + } + 1 -> when (p[0]) { + String::class.java -> + safeCtor { e -> (constructor.newInstance(e.message) as Throwable).also { it.initCause(e) } } to 2 + Throwable::class.java -> + safeCtor { e -> constructor.newInstance(e) as Throwable } to 1 + else -> null to -1 + } + 0 -> safeCtor { e -> (constructor.newInstance() as Throwable).also { it.initCause(e) } } to 0 + else -> null to -1 } - - } - for (constructor in constructors) { - val result = createSafeConstructor(constructor) - if (result != null) return result - } - return nullResult -} - -private fun createSafeConstructor(constructor: Constructor<*>): Ctor? { - val p = constructor.parameterTypes - return when (p.size) { - 2 -> when { - p[0] == String::class.java && p[1] == Throwable::class.java -> - safeCtor { e -> constructor.newInstance(e.message, e) as Throwable } - else -> null - } - 1 -> when (p[0]) { - Throwable::class.java -> - safeCtor { e -> constructor.newInstance(e) as Throwable } - String::class.java -> - safeCtor { e -> (constructor.newInstance(e.message) as Throwable).also { it.initCause(e) } } - else -> null - } - 0 -> safeCtor { e -> (constructor.newInstance() as Throwable).also { it.initCause(e) } } - else -> null - } + }.maxByOrNull(Pair<*, Int>::second)?.first ?: nullResult } -private inline fun safeCtor(crossinline block: (Throwable) -> Throwable): Ctor = { e -> +private fun safeCtor(block: (Throwable) -> Throwable): Ctor = { e -> runCatching { val result = block(e) /* From 3c53fb5d4d2f87c61dfaae8ca0914d82f141edb5 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Tue, 2 May 2023 17:11:56 +0200 Subject: [PATCH 4/4] ~remove debug output from test --- .../jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt b/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt index 49f9503371..2d8c0ebc75 100644 --- a/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt +++ b/kotlinx-coroutines-core/jvm/test/exceptions/StackTraceRecoveryChannelsTest.kt @@ -67,7 +67,6 @@ class StackTraceRecoveryChannelsTest : TestBase() { block() expectUnreached() } catch (e: RecoverableTestException) { - e.printStackTrace() verifyStackTrace("channels/${name.methodName}", e) } }