recoverStackTrace breaks exception message

[orange-buffalo]

Consider the following test case:

class ExceptionTest {
    @Test
    fun `should preserve exception message`() {
        Assertions.assertThatThrownBy {
            runBlocking {
                withContext(newFixedThreadPoolContext(10, "my-context")) {
                    throw MyTokenException("42")
                }
            }
        }.isInstanceOf(MyTokenException::class.java).hasMessage("Token 42 is not valid")
    }
}

class MyTokenException(token: String) : RuntimeException("Token $token is not valid")

It is failing on message assertion:

Expecting message to be:
  <"Token 42 is not valid">
but was:
  <"Token Token 42 is not valid is not valid">

It looks like recoverStackTrace / tryCopyException incorrectly recovers the exception. #1040 mentions that similar problem is fixed in 1.2.0, but this test fails on 1.3.2.

[qwwdfsad]

I am not sure we can do better automatically without reflectively hacking Throwable class (that probably will produce a shameful warning on Java 9+), I'll try to figure it out.

To understand why it happens, I recommend you to get familiar with stacktrace recovery machinery:

The main idea is to reflectively create a copy of the exception, preserving as much information from the original exception as possible. Usually, it is a message and a cause, so we reflectively call a constructor with String parameter and, if possible, with Throwable (or call initCause, whatever) and we cannot potentially know whether this String parameter maps directly to Throwable.message or not.

As a workaround, you can fix this problem by extending CopyableThrowable in your MyTokenException class

[monosoul]

Having the same issue here. I can set property kotlinx.coroutines.stacktrace.recovery to false to run tests properly, but the problem here is that it's a system property. And if I run tests in parallel, setting this property in a single test would definitely affect other tests.

[qwwdfsad]

@monosoul do you mean broken exception message or stacktrace recovery in general?

I don't think we can provide something besides system property, because stacktrace recovery was designed as static property in the first place. Otherwise, everyone if (STACKTRACE_RECOVERY_ENABLED) becomes non-foldable by the compiler, the implementation should be aware that this property can change its value and Android minifier will have a hard time removing stacktrace recovery machinery

[monosoul]

@qwwdfsad yeah, I mean broken exception message.

Maybe it'd be a better idea to create a RuntimeException instance instead of trying to recreate an instance of the exception class that had been thrown in coroutine? At least it'd be less obscure behavior than the one we're getting at the moment. It took me some time to figure out why am I having this message duplication because of a formatted string. Maybe you can introduce a special exception class for that reason.

I mean, of course it's also possible to implement CopyableThrowable, but the problem is that implementation of this class wouldn't make any sense in terms of a problem domain. 🙂

[qwwdfsad]

Creating RuntimeException may look tempting, but it will break most of the code that handles specific exceptions with catch block :)

One of the possible options is to prefer constructor without a message and to hack message reflectively, but again, it will require reflective access to java.base module

[monosoul]

@qwwdfsad Oh, yeah, you're right.

I guess another solution could be to use CGLIB (or something like that) to create a proxy for the exception class. Of course it'd have a performance impact, but on the other hand this impact would only affect tests/local runs, which is fine IMO.

Anyway, in our case we're just wrapping different exceptions, so we decided to just go with a static exception message and different causes.

[qwwdfsad]

The easiest option is to introspect constructor parameter name using reflection and copy an exception only if the parameter is named "message".
Unfortunately, this functionality is blocked by #1589