Handle exceptions with coroutines.

[AliLozano]

Now, in development there is not elegant form to show errors in console, we have to put in every coroutine try {} catch and print the error, and in production for example if I have EntityNotFoundException, I have to put a try catch and raise StatusException.

Is there an elegant way to handle exceptions with coroutins? If we use the classic grpc form, we have the interceptors, but in the case of the coroutins, the exception is encapsulated before it can be handled by the server interceptors. I think the errors should be able to be handled like coroutin errors in the context, and if they are not handled there, the server must handle it with interceptors, and if the server don't handle it, the application should be closed(By default should exist a interceptor to catch every exception and handle it, for avoid application closing).

At the moment I have solved it by modifying the library to delegate errors to a Handler from Throw.toRpcException but it is not the best.

[marcoferrer]

Thanks for reporting this.

I think you have a valid point. The original reasoning behind mapping service exceptions was to ensure proper mapping of coroutine cancellations to Status.CANCELLED. See Throwable.toRpcException().

This seemed like a safe change at the time since the grpcs onError handler does this internally. I didnt consider interceptors being used to map custom exceptions.

I found the following example of this particular use case. It shows the interceptor mapping exceptions by checking the return status cause. Is this similar to what you need to achieve?

I think this issue can be resolved in kroto with one of two options.
1) Update the rpc exception mapper to persist the original exception into the status cause
2) Remove the else clause from the exception mapper and only check for instances of CancellationException

Edit: Turns out neither of these options are needed. The cause was already being persisted. The use case outlined above specifically needed the exceptions to be thrown during onHalfClose. This isnt possible in the current implementation due to method execution being delegated to a launch block and the service method invocation immediately completing

[AliLozano]

Yes, here are other examples for authentication.

But I think is not so simple, we could raise the exception from coroutine after it was completeSafely, or completeSafely could handle the exception that it can, and throw other exceptions. but..

launch(start = CoroutineStart.ATOMIC) {
            try{
                responseObserver.onNext(block())
                responseObserver.onCompleted()
            }catch (e: Throwable){
                responseObserver.completeSafely(e)
                if(e !is CancellationException && e !is StatusRuntimeException && e !is StatusException) throw e
            }
        }

But you commented this:

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.

And I don't know what do you refer with that.

[AliLozano]

Another thing is that the connection is closed when an exception occurs, but when exception is raised, the connection should not be closed.

[marcoferrer]

That state refers to two separate points. First being that exceptions are mapped and returned to the client. Which is the default behavior for vanilla grpc java. The second part is referring to the implementation of that method. It ensures that once the body of the rpc method is completed the client is notified. If the method body terminated early from an unexpected exception or child coroutine cancellation, it is possible that the stream observer could have been abandoned.

As for throwing exceptions during onHalfClose, I dont think this is the intended usage of the grpc api. Grpc docs recommend returning errors via the provided stream observer. Its also the reason why the generated server stubs in vanilla grpc aren't marked with throws.

The stream observer supplied to server method implementation is meant to serve as a callback. This is what allows implementations to choose whether to block and perform logic or fire off an async task with a reference to the observer for completion.

In the kroto server implementions the server method is immediately invoked. Rethrowing the exception in the launch block doesnt mean it will be thrown during onHalfClose or that anything outside of the coroutine scope would be able to catch it.

[marcoferrer]

Another option for your particular case, although a bit more intrusive than an interceptor, is using a method to wrap your service methods.

inline fun <T> handleMethod(block: () -> T): T = 
    try{
        block()
    }catch (e: Throwable){
        println("log error $e")
        throw e
    }

suspend fun sayHello(request: HelloRequest): HelloReply = handleMethod {
    ...
}

[AliLozano]

Oh, I understand, I have downloaded the sources of kroto to do something about this, but I think it will not be possible, because as you say, there is no way for the coroutine to notify the main thread that there was an error without observing , unless you block it and throw a throw, but it seems that causes the entire main thread to be blocked. At first I thought that grpc handled the requests asynchronously, but I see no, if I put a sleep in a java grpc implementation, I block everything.

The solution that I think will best fit will be to handle exceptions through initialContext, as in the other issue or something like that.

[AliLozano]

Another option for your particular case, although a bit more intrusive than an interceptor, is using a method to wrap your service methods.

inline fun <T> handleMethod(block: () -> T): T = 
    try{
        block()
    }catch (e: Throwable){
        println("log error $e")
        throw e
    }

suspend fun sayHello(request: HelloRequest): HelloReply = handleMethod {
    ...
}

It can be, but I don't like 😢

[AliLozano]

I have three solutions on the table, help me choose which fits better to what kroto is looking for to make a pull request.

Basically I need have the exception and the observableResponse opened, to be able to send messages to the client, informing him what happened.

Option 1

1. Throw the exceptions to be handled by the context using CoroutineExceptionHandler

Advantage:

• It is the standard form of the coroutines

Disadvantages:*

• There is no access to the observable Response. It is shared between several requests so I cannot save the observableresponse in context. But can be solved by creating an exception that can contain the observableResponse and upon receiving it in the CoroutineExceptionHandler to cast the exception, to extract the observableResponse.

Option 2

2. Create a method in the CoroutinesServiceGrpc that is called processRequest which you receive: observableRequest, observableResponse, next () and serve as an interceptor and in turn to capture exceptions.

Advantages

• You have static typed for request and response, like classic way.
• You can add other interceptors like validations

Disadvantages

• Maybe could be confused to handle differents request types in only one.
• For use response in a interceptor, have to make a many changes, because currently, response is not passed to serverCalls.

Option 3

3. Mark the serviceBinder method as final(I will open other issue for this) and look for an alternative to do so with reflections, spring and proxies

Advantages

• We don't have to do anything here(well its recommended to implement the first alternative without the custom exception part)

Option 4

4. Implement everything ✌️ raise exceptions to context without encapsulate exception and the observableResponse closed, add a method processRequest, and allow proxies with reflections to use annotations.

[AliLozano]

Well, I've chosen the option number 3, I've created a proxy to handle exceptions. Because I was not sure if is correct if the exception have to raised to context because it is shared between requests, and the option 2 is too intrusive to this library.

If someone need it:

@Configuration
class GrpcErrorHandlerConfiguration {

    @Autowired
    lateinit var handlers: List<GrpcExceptionHandler>

    @Bean
    fun defaultGrpcExceptionHandler() =  CommonGrpcExceptionHandler()

    @Bean
    fun autoProxyCreator(errorHandlerInterceptor: MethodInterceptor): BeanNameAutoProxyCreator {
        return object: BeanNameAutoProxyCreator() {
            override fun getAdvicesAndAdvisorsForBean(beanClass: Class<*>, beanName: String, targetSource: TargetSource?): Array<Any>? {
                if(BindableService::class.java.isAssignableFrom(beanClass)) {
                    return arrayOf(errorHandlerInterceptor)
                }
                return AbstractAutoProxyCreator.DO_NOT_PROXY
            }
        }.also {
            it.isProxyTargetClass = true
        }
    }

    @Bean
    fun errorHandlerInterceptor() = MethodInterceptor { invocation -> processInvocation(invocation) }

    fun processInvocation(invocation: MethodInvocation): Any {
        try {
            return invocation.proceed()
        } catch (ex: Throwable) {
            Log.debug("{}[{}]: {}", invocation.method.declaringClass.simpleName, invocation.method.name, ex.message)
            handlers.forEach { it.process(ex) } // convert the exception to StatusException and throw it.

            Log.error("{}[{}]: {}", invocation.method.declaringClass.simpleName, invocation.method.name, ex.message, ex)
            throw ex
        }
    }

}

In this case, I've used many GrpcExceptionHandler to process different type of exceptions.

[marcoferrer]

Sorry I couldnt provide a more concrete solution. There might be a less painful way of achieving this but I think it might require a refactor of the server handler internals. Theres some prototype work I've been doing to bring the server method invocation closer to the call level. This would allow kroto to leverage configurations provided via call attributes attached wrapped server call implementations provided via interceptors

[AliLozano]

I am just reviewing coroutines and grpc but, I have some suggestion, maybe if the coroutines are handled by the first interceptors instead of the service implementation, you can have better control of the errors.

**Now: ** > Listeners > Call Service> Coroutine() > End Call Service > End Listener.

There is the problem that if there is an error in the context, we cannot use ContextHandleError in the implementation because that context is for all requests and if we throw an error towards that context it would affect all the coroutines that are inside, so The solution to this problem is to encapsulate all errors with a StatusError.

Proposal > Couroutine Listener > Call Listeners > Call Service > End Listener> End Coroutine >

The advantage of this is that since the listeners are in the same context as the service, they could capture the errors that exist in the service. On the other hand as the context used within the service implementation is a different one for each request, we can use the context handle error right like this that is the correct way.

[rocketraman]

I have this issue too -- when I updated to the latest kroto-plus, I lost all logging of exceptions on the server-side, because we removed our custom exception handling code in favor of what Kroto-Plus provides. The client now just reports useless errors like CANCELLED: Received http2 header with status: 504.

I'd be happy with just being able to inject a logger into kroto-plus to log unhandled exceptions from implementations.

[marcoferrer]

@rocketraman Was your previous exception logging done through interceptors? Could you provide a sample? Im interested in trying to adapt kroto to support async interceptors. Netflix has a nice system for handling something slightly similar in their concurrency limits library.

[rocketraman]

@marcoferrer My previous exception logging was done via wrapping all my implementations in a function, similar to as shown above (except it passes through StatusExceptions unlogged, as these are "expected" vs other exceptions which are "unexpected"). For some reason I assumed that with the improved error handling in the updated kroto-plus, I didn't need this any more, but it seems that assumption was incorrect. I'll add this back in for now.

[Globegitter]

I am also just running into this and now making use of the handleMethod example posted above. Would be great if there was a less intrusive, documented way of logging and possibly doing something else with all uncaught exceptions.

[marcoferrer]

I wanted to update this with the current state of this request.

At the moment the internal implementation for server calls is being refactored and enhanced. The upcoming changes will greatly simplify the server internals and is going to allow much more flexibility with the exposed API. With this new flexibility we are going to be introducing a mechanism for handling server method exceptions. The design at the moment is based on the existing CoroutineExceptionHandler impl but will allow access to the underlying call.

val grpcExceptionHandler = GrpcExceptionHandler { 
    call: ServerCall<*, *>, context: CoroutineContext, exception: Throwable ->

   call.close(Status....) 
}

The goal is to be able to pass an instance of this coroutine context element from either a server call interceptor or through the initialContext of the service instance