Status cause is replaced when it shouldn't be

[andrewparmet]

After #400, we're seeing a change in behavior in our interceptors when a call is closed due to a thrown StatusException or StatusRuntimeException. In particular, the status' cause is being replaced: whereas beforehand the cause was the cause of the exception's status, it is now the exception itself.

Supposing we have:

val exception = Exception("foo")

// service definition
override suspend fun sayHello(request: HelloRequest): HelloReply {
    throw Status.fromCode(code)
        .withCause(exception)
        .asException()
}

In our intercepted call's close() method we now get:

override fun close(status: Status, trailers: Metadata) {
     // the thrown exception from sayHello; in grpc-kotlin prior to #400 and in grpc-java this is `exception` itself
    status.cause

    // this is now how we have to get `exception`
    Status.fromThrowable(status.cause).cause 
}

This is because

grpc-kotlin/stub/src/main/java/io/grpc/kotlin/ServerCalls.kt

Line 265 in c449254

else -> Status.fromThrowable(failure).withCause(failure)

replaces the cause of the status even when that status had been correctly inferred from a StatusException/StatusRuntimeException.

We noticed this in an interceptor we use for services using both grpc-kotlin and grpc-java service implementations. We can use some clever branching (Status.fromThrowable(status.cause) == status means we're in grpc-java or prior versions of grpc-kotlin) but I don't think this is really the best way to handle it; I don't think grpc-kotlin should replace a validly inferred cause from Status.fromThrowable.

I think the solution is to add two branches to the when expression in ServerCalls.kt - one each for StatusException and StatusRuntimeException that do not attach failure as the cause. In service of the desired behavior of #400, not replacing the cause for these cases should still preserve caller info in the stack trace since failure is still available in the status object. Does that sound reasonable?

[jamesward]

Thanks for the detailed report! I think this sounds good but would also like feedback from @lowasser and @zakhenry.

[zakhenry]

Yep good catch @andrewparmet and sorry for the regression. Your proposal does make sense, and in theory the test I added in #400 should still be passing with your change, which will confirm the original issue that I fixed stays fixed.

[andrewparmet]

Here's a PR with a fix: #456