gRPC context should be propagated to service code #22
Conversation
gRPC uses a io.grpc.Context class to make metadata about a call available to implementation code. The context of a call is stored in a ThreadLocal allowing it to be easily accessed within the service method call. Kotlin coroutines do not run on a single, predictable thread, making this storage insufficient. Fortunately, Kotlin provides an easy way to adapt this thread local pattern to a CoroutineContext, which is implemented in this commit.
|
One aspect I'm not completely sure about on this PR is weather or not the launch (coroutineContext + ContextCoroutineContextElement()) {
} |
| @@ -105,7 +105,7 @@ object {{className}} { | |||
| request: {{inputType}}, | |||
| responseObserver: StreamObserver<{{outputType}}> | |||
| ) { | |||
| launch { | |||
| launch (ContextCoroutineContextElement()) { | |||
There was a problem hiding this comment.
launch is an extension function on CoroutineScope which this class implements. You could add the context element to the user supplied CoroutineContext and have it be used by all lunched coroutines within the class.
There was a problem hiding this comment.
Yeah, I though about pursuing that option, but figured that propagating the context should be the default behaviour. Values attached to the context are quite useful and users will expect them to be available. Maybe a better approach would be to include this new context element in the default coroutineContext specified on the constructor.
My preference is to have the context propagated all the time, without the user having to think about it (even if the user specifies their own coroutineContext).
There was a problem hiding this comment.
Yes I'd prefer that too. What I meant was that it should be added to whatever context the user passes in to the constructor so it's always present throughout the class, instead of only being added to each launch block.
abstract class GreeterImplBase(
val _coroutineContext: CoroutineContext = Dispatchers.Default,
val sendChannelCapacity: Int = KtChannel.UNLIMITED
) : BindableService, CoroutineScope {
override val coroutineContext: CoroutineContext
get() = _coroutineContext + ContextCoroutineContextElement()
// ...There was a problem hiding this comment.
Ok strike that. I realized this won't work because the Context.current() call would happen when the service instance is created and not on each call. So either we have to get the current context differently (can it be done in updateThreadContext?), or we have to add the context element in each launch block as you've done.
There was a problem hiding this comment.
Strike that too, this is full of surprises :)
So in my example, the ContextCoroutineContextElement() call is done in the property getter, which in turn is called by coroutine builders like launch. This means that the Context.current() will be the one from the call scope. So that works!
I would write some tests to make sure we get the expected behavior.
There was a problem hiding this comment.
Yeah, I think I like that. I'll update the PR with that change, then add a some tests.
Addition of tests cases will follow
| @@ -87,10 +87,13 @@ object {{className}} { | |||
|
|
|||
| {{#javaDoc}}{{{javaDoc}}}{{/javaDoc}} | |||
| abstract class {{serviceName}}ImplBase( | |||
| override val coroutineContext: CoroutineContext = Dispatchers.Default, | |||
| private val _coroutineContext: CoroutineContext = Dispatchers.Default, | |||
There was a problem hiding this comment.
If you drop the val on these fields, you'll avoid shadowing the field inherited from CoroutineScope.
There was a problem hiding this comment.
I'm sorry but I don't think I follow your suggestion. If _coroutineContext is not a val, then it is not available in the getter for coroutineContext.
There was a problem hiding this comment.
I had something locally that worked, maybe I used a private internal field called something else. I'll do a change if I can find it.
|
LGTM! |
gRPC uses a io.grpc.Context class to make metadata about a call available to implementation code. The context of a call is stored in a ThreadLocal allowing it to be easily accessed within the service method call. Kotlin coroutines do not run on a single, predictable thread, making this storage insufficient. Fortunately, Kotlin provides an easy way to adapt this thread local pattern to a CoroutineContext, which is implemented in this commit.