rouzwawi / grpc-kotlin

gRPC with Kotlin Coroutines
https://gitter.im/gRPC-Kotlin/Lobby
Apache License 2.0
218 stars 22 forks source link

Remove exception handling from PasswordStatusServiceImplBase.connect...ToObserver functions #4

Closed wfhartford closed 5 years ago

wfhartford commented 6 years ago

Also changed connect...ToObserver functions to user runBlocking rather than launch

Client will receive the same 'io.grpc.StatusRuntimeException: UNKNOWN', however the exception gets propagated through interceptor and logged on the server.

wfhartford commented 6 years ago

Resolves #3

rouzwawi commented 5 years ago

Thanks for raising this. I'm trying to figure out what the expected behavior is when throwing an exception in the async handler code. Since there is no observer to call onError(Throwable) on, the only way to actually return an exception is to send it through the Deferred value or Channel.

override fun greet(request: GreetRequest): Deferred<GreetReply> = async(pool) {
  throw Status.NOT_FOUND.asRuntimeException()
}

In this case the kotlin code does as expected and propagates the status to the client while the java code does an internal close on the call and returns an UNKNOWN status. See ServerImpl.java.

I would like to better understand the expected behavior on what to do with exceptions thrown from within the coroutines. I understand that currently anything that can not be mapped to a Status by gRPC will just be erased to an UNKNOWN status exception without allowing for any interception. So maybe the fix should try to call onError(Throwable) when it's a StatusException or StatusRuntimeException and otherwise propagate up to the ServerStreamListener.

wfhartford commented 5 years ago

I've updated the PR with your suggestion, you're absolutely right, I found that when working on some subsequent changes I made to my fork but forgot to update the PR.

rouzwawi commented 5 years ago

@wfhartford There's still some issues with this change when I tested it. In the case when an application specific exception is thrown, the response observer is never closed and the request hangs (until timed out).

I concluded that there's an inherent conflict between how exceptions normally propagate up into gRPC internals and how exceptions propagate in coroutines. So the best way to intercept exceptions thrown from coroutines is to use the CoroutineExceptionHandler together with a CoroutineContext.

This mechanism is implemented in #7, I hope this solves your issue.

wfhartford commented 5 years ago

Closing this PR as #7 addresses it much more cleanly.