Skip to content
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

recoverStackTrace causes nested exceptions #3714

Closed
iwismer opened this issue Apr 13, 2023 · 3 comments
Closed

recoverStackTrace causes nested exceptions #3714

iwismer opened this issue Apr 13, 2023 · 3 comments
Assignees

Comments

@iwismer
Copy link

iwismer commented Apr 13, 2023

Describe the bug

When throwing an exception in a coroutine scope it sometimes becomes nested one level too deep in the enclosing exception.

For example, if I throw a: TestException(IllegalStateException("test")) (a TestException with a cause of IllegalStateException with a message), then the coroutine scope will throw an exception that is: TestException(TestException(IllegalStateException("test"))).

Provide a Reproducer

The following test will fail as a result of this bug:

@Test
fun testNestedException() = runTest {
    val result = runCatching {
        coroutineScope<Unit> {
            throw NestedException(IllegalStateException("ERROR"))
        }
    }
    val ex = result.exceptionOrNull() ?: error("Expected to fail")
    assertTrue(ex is NestedException)
    assertTrue(ex.cause is IllegalStateException)
    assertEquals("ERROR", (ex.cause as IllegalStateException).message)
}

class NestedException : RuntimeException {
    constructor(message: String) : super(message)
    constructor(cause: Throwable) : super(cause)
    constructor() : super()
}

I can get this test to not fail by changing the order of the constructors in the class to:

class NestedException : RuntimeException {
    constructor(cause: Throwable) : super(cause)
    constructor(message: String) : super(message)
    constructor() : super()
}

But I don't think this is actually a good solution as it shouldn't matter the order of the constructors in my exception, it should work the same way regardless.

I think the issue is at this line where the wrong constructor is being selected (the string constructor instead of the cause constructor).

@iwismer iwismer added the bug label Apr 13, 2023
@dkhalanskyjb
Copy link
Collaborator

Thank you for the report! The actual problem is in this line:

Instead of ordering the constructors from best to worst, we exit when we find the first one that matches any viable signature.

@qwwdfsad qwwdfsad self-assigned this May 1, 2023
@qwwdfsad
Copy link
Contributor

qwwdfsad commented May 1, 2023

These two constructors should be identical, something is going completely wrong here

@qwwdfsad qwwdfsad added the debug label May 1, 2023
qwwdfsad added a commit that referenced this issue May 1, 2023
…(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
@iwismer
Copy link
Author

iwismer commented May 15, 2023

The test that was implemented in #3731 seems to have some incorrect asserts that are actually testing that the error is still happening.

The asserts in the test are:

val ex = result.exceptionOrNull() ?: error("Expected to fail")
assertIs<NestedException>(ex)
assertIs<NestedException>(ex.cause)
val originalCause = ex.cause?.cause
assertIs<IllegalStateException>(originalCause)
assertEquals("ERROR", originalCause.message)

which asserts that the error is: NestedException(NestedException(IllegalStateException()))

But what the thrown error should actually be is: NestedException(IllegalStateException())
So the asserts should be changed to:

val ex = result.exceptionOrNull() ?: error("Expected to fail")
assertIs<NestedException>(ex)
val originalCause = ex.cause
assertIs<IllegalStateException>(originalCause)
assertEquals("ERROR", originalCause.message)

So it seems like this issue is not yet solved, and I'm still running into this issue in my dependant code after updating to 1.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants