From 25a3553ed0466ca14d5166f9386cc8607d56ac84 Mon Sep 17 00:00:00 2001 From: Vsevolod Tolstopyatov Date: Wed, 3 May 2023 10:49:26 +0200 Subject: [PATCH] =?UTF-8?q?Properly=20recover=20exceptions=20when=20they?= =?UTF-8?q?=20are=20constructed=20from=20'Throwable=E2=80=A6=20(#3731)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 undependable from source-code order Fixes #3714 --- .idea/codeStyles/Project.xml | 2 +- .../jvm/src/internal/ExceptionsConstructor.kt | 68 ++++++++++--------- .../jvm/src/internal/StackTraceRecovery.kt | 21 ++---- .../channels/testReceiveFromChannel.txt | 3 +- .../resume-mode/testUnconfined.txt | 3 +- .../StackTraceRecoveryCustomExceptionsTest.kt | 20 ++++++ 6 files changed, 66 insertions(+), 51 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..03308f6137 100644 --- a/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt +++ b/kotlinx-coroutines-core/jvm/src/internal/ExceptionsConstructor.kt @@ -32,42 +32,48 @@ 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 } - 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 + * 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 we can + * not rely on the order of iteration. Instead, we assign a unique priority to each ctor type. + */ + 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 } - 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 -> runCatching { block(e) }.getOrNull() } +private fun safeCtor(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..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,14 +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 - // 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 -} - /* * Here we partially copy original exception stackTrace to make current one much prettier. * E.g. for @@ -109,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 @@ -157,7 +149,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 { @@ -198,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/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) {