marcoferrer / kroto-plus

gRPC Kotlin Coroutines, Protobuf DSL, Scripting for Protoc
Apache License 2.0
493 stars 28 forks source link

Uncaught exceptions #39

Closed travikk closed 5 years ago

travikk commented 5 years ago

Hey there,

So I have a very simple service:

@GrpcService
class SimpleThrowingService() : SimpleThrowingServiceCoroutineGrpc.SimpleThrowingServiceImplBase() {

    private val exceptionHandler = CoroutineExceptionHandler { _, ex ->
        logger.error("Exception caught", ex)
    }

    override val initialContext: CoroutineContext
        get() = exceptionHandler

    override suspend fun throwingStream(request: Empty, responseChannel: SendChannel<Int>) {
        throw IllegalArgumentException(":(")
    }
}

Because I'm setting exceptionHandlerContext into the initialContext I would expect the handler catch the exception and log the message. Unfortunately, what happens is... Nothing. The exception gets swallowed and not even printed out.

When I however start a new CoroutineScope with my exceptionHandler as it's context, that seems to work fine.

Currently I'm on 22-RC3

Why does that happen?

Thanks!

marcoferrer commented 5 years ago

Thanks for opening this issue. I've been working on documenting the expected behavior for handling exceptions. Let me explain a little.

Exceptions thrown from an rpc method are caught and mapped to their appropriate StatusRuntimeException if needed using this method. So because of this they will never reach the CoroutineExceptionHandler, which is why you're not seeing it invoked.

This exception is then passed to the appropriate responseObserver.onError(). Unlike the plain java implementation, rpc methods do not receive a reference to the responseObserver so returning errors to clients is done via throw e and responseChannel.close(e).

The methods used to execute incoming rpc calls can be found in ServerCalls.kt. These methods then use handleRpc (found in CallExts.kt) to ensure errors are returned to the client and prevent leaks of unhandled requests and abandoned response channels.

Is there a particular use case you have that this behavior is preventing?

marcoferrer commented 5 years ago

Closing this out since the readme has been updated with a better explanation. If this answer wasn't sufficient, feel free to reopen this issue.

AliLozano commented 5 years ago

And there is a way to handle all exceptions with a delegated class without having to use try catch and changing the exception with StatusException.