Support Kotlin-gRPC client CoroutineStub #2669

ikhoon · 2020-04-16T07:37:26Z

Motivation:

gRPC for Kotlin is actively developed and
0.1.1 has been released last week.

Modifications:

Allow GrpcClientFactory creating client from CoroutineStub
Migrate Kotlin example to gRPC-Kotlin

Result:

Partially Fixes Support CoroutineStubs from grpc-kotlin #2662
You can now run Armeria gRPC client with gproto* and gRPC-Kotlin CoroutinStub

ikhoon · 2020-04-16T07:42:26Z

/cc @gary-lo

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java

trustin · 2020-04-16T07:56:55Z

/cc @gary-lo

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/HelloServiceImpl.kt

gary-lo · 2020-04-16T08:05:40Z

Wow thanks for doing this. It's surprising and awesome that so much of our code looks the same 😅 even though we worked independently. I'll close my PR.

codecov · 2020-04-16T08:20:17Z

Codecov Report

Merging #2669 into master will decrease coverage by 0.46%.
The diff coverage is 47.02%.

@@             Coverage Diff              @@
##             master    #2669      +/-   ##
============================================
- Coverage     73.40%   72.94%   -0.47%     
- Complexity    11274    11293      +19     
============================================
  Files           993      997       +4     
  Lines         43329    44057     +728     
  Branches       5392     5463      +71     
============================================
+ Hits          31807    32136     +329     
- Misses         8760     9127     +367     
- Partials       2762     2794      +32

Impacted Files	Coverage Δ	Complexity Δ
...rmeria/internal/client/grpc/GrpcClientFactory.java	`54.54% <25.00%> (-8.15%)`	`12.00 <0.00> (ø)`
...c/main/java/example/armeria/grpc/kotlin/Hello.java	`34.72% <34.72%> (ø)`	`2.00 <2.00> (?)`
.../example/armeria/grpc/kotlin/HelloServiceGrpc.java	`48.37% <48.37%> (ø)`	`13.00 <13.00> (?)`
...linGrpc/example/armeria/grpc/kotlin/HelloGrpcKt.kt	`89.02% <89.02%> (ø)`	`0.00 <0.00> (?)`
...in/example/armeria/grpc/kotlin/HelloServiceImpl.kt	`97.14% <97.05%> (+12.52%)`	`7.00 <7.00> (ø)`
...n/java/com/linecorp/armeria/server/HttpServer.java	`40.00% <0.00%> (-20.00%)`	`2.00% <0.00%> (-1.00%)`
...om/linecorp/armeria/client/HttpSessionHandler.java	`64.13% <0.00%> (-8.28%)`	`40.00% <0.00%> (-3.00%)`
...p/armeria/common/stream/AbstractStreamMessage.java	`82.05% <0.00%> (-1.71%)`	`19.00% <0.00%> (ø%)`
...com/linecorp/armeria/server/HttpServerHandler.java	`81.00% <0.00%> (-1.34%)`	`79.00% <0.00%> (-2.00%)`
...inecorp/armeria/server/HttpResponseSubscriber.java	`77.16% <0.00%> (-1.19%)`	`53.00% <0.00%> (-2.00%)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad3ac18...c0a578c. Read the comment docs.

ikhoon · 2020-04-16T08:34:30Z

Wow thanks for doing this. It's surprising and awesome that so much of our code looks the same 😅 even though we worked independently. I'll close my PR.

To make a test code under gRPC-Kotlin coroutine stub, I refer to your PR airtasker#3

Let me add you as a co-author. 🙇‍♂️

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/HelloServiceImpl.kt

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java

This gRPC-Kotlin is really awesome. ❤️ Motivation: Armeria is going to early adopt gRPC-Kotlin by our users' requests. line/armeria#2669 Armeria gRPC-Java client access `ServiceDescriptor` via reflection. I think we can access to the orignal gRPC-Java stub via `StubFor` annotation in coroutine stub. Modifications: - Change AnnotationRetention from `BINARY` to `RUNTIME` Result: Better interop between Armeria and gRPC-Kotlin 😀

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/HelloServiceImpl.kt

anuraaga

Hmm - propagating armeria context into grpc context and kotlin context is pretty wild and it'd be nice if we can keep on playing with the Dispatcher, it should work somehow in theory. It's generally a code smell that we have to add to armeria-grpc only to support kotlin

I tried playing with this branch to look at the generated code, but probably since I'm on Windows, even if I run with JAVA_HOME set to a JDK 11, the compiler just shows a GUI prompt saying it requires a JDK 11 and doesn't generate any protos... Maybe we should wait a little more for the project to mature?

anuraaga · 2020-04-17T12:51:29Z

Found the kotlin compiler is this shell script

https://github.com/grpc/grpc-kotlin/blob/master/compiler/stub.sh

Indeed seems to have way too many implicit dependencies on the run environment 😓

ikhoon · 2020-04-17T13:42:38Z

I tried playing with this branch to look at the generated code

I updated the generated gRPC-Kotlin files to a different branch.😉
ikhoon@af3b29f

anuraaga · 2020-04-17T14:17:17Z

I came up with this very hacky approach that seems to work. It relies on the implementation detail that the grpc-kotlin stub calls context + grpccontext.

It's not great to rely on the implementation detail, and only really works for gRPC singe other usages aren't guaranteed to call plus, but seems to provide good UX (not just context mounted but event loop is used too). Can you file an issue with grpc-kotlin that instead of taking a context as a constructor parameter for the service they should instead take a context provider or an abstract method? Contexts conceptually are per-request so the API as is doesn't make sense as it requires a singleton context as the service implementation is also a singleton. I don't know if I'd proceed with this PR until that's improved upstream.

package example.armeria.grpc.kotlin

import com.linecorp.armeria.common.RequestContext
import kotlinx.coroutines.*
import kotlin.coroutines.Continuation
import kotlin.coroutines.ContinuationInterceptor
import kotlin.coroutines.CoroutineContext
import kotlin.coroutines.EmptyCoroutineContext

internal object ArmeriaContext: CoroutineContext {
    override fun <R> fold(initial: R, operation: (R, CoroutineContext.Element) -> R): R =
            EmptyCoroutineContext.fold(initial, operation)

    override fun <E : CoroutineContext.Element> get(key: CoroutineContext.Key<E>): E? =
            EmptyCoroutineContext.get(key)

    override fun minusKey(key: CoroutineContext.Key<*>): CoroutineContext =
            EmptyCoroutineContext.minusKey(key)

    override fun plus(context: CoroutineContext): CoroutineContext {
        val requestCtx: RequestContext = RequestContext.current()
        return requestCtx.contextAwareExecutor().asCoroutineDispatcher() + context
    }
}

val Dispatchers.Armeria: CoroutineContext
    get() = ArmeriaContext

ikhoon · 2020-04-17T14:54:03Z

Can you file an issue with grpc-kotlin that instead of taking a context as a constructor parameter for the service they should instead take a context provider or an abstract method?

Yes, I think we need to file two issues

1. To call RequestContext.current(), the *CoroutineStub should be called by an Armeria event loop directly. If not, we can't acquire a request context from the beginning. I think the upstream could invoke a coroutine stub in current call frame if the current thread is an instance of event loop such as io.netty.channel.EventLoop.

~~2. As you said it would be nice if the upstream offers a context provider or an abstract method for customizing context for requests.~~

anuraaga · 2020-04-17T15:13:07Z

Do we need anything special for point 1? My hack already works ok. What matters is that the context provider method of point 2) is called in the caller frame, and I guess it always would be since the context is needed to start the coroutine.

ikhoon · 2020-04-17T15:34:51Z

Do we need anything special for point 1? My hack already works ok

Ah... now I understood how your hack works exactly. Your hack looks awesome. 👍

ikhoon · 2020-04-18T04:37:32Z

Filed the issue. grpc/grpc-kotlin#66

anuraaga · 2020-04-18T05:22:29Z

Thanks! As long as the hack is kept to the example and not a published artifact, I guess it's ok. For this, can we also go ahead and check in the generated code? The proto compiler plugin is very much not ready for wide use right now and should leave it out of our build until it's more mature.

examples/grpc-kotlin/src/test/kotlin/example/armeria/grpc/kotlin/HelloServiceTest.kt

Motivation: gRPC for Kotlin is actively developed and [0.1.1](https://github.com/grpc/grpc-kotlin/releases/tag/v0.1.1) has been released last week. Modifications: - Allow GrpcClientFactory creating client from CoroutineStub - Migrate Kotlin example to gRPC-Kotlin Result: - (Partially) Fixes line#2662 - You can now run Armeria gRPC client with `gproto` protocol and gRPC-Kotline CoroutinStub Co-authored-by: Gary Lo <gary.lo@airtasker.com>

gradle/scripts/lib/java-rpc-proto.gradle

Generate kotlin stub if a project has `kotlin-grpc` flag and `io.grpc:grpc-kotlin-stub` dependency See line/armeria#2669

examples/grpc-kotlin/build.gradle.kts

trustin

Thanks a lot, @ikhoon !

trustin · 2020-04-29T10:00:38Z

@anuraaga @gary-lo @minwoox Any other comments?

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/ArmeriaContext.kt

gary-lo

LGTM thank you

ikhoon · 2020-04-29T14:02:22Z

@gary-lo We built this PR together! 😀

anuraaga · 2020-04-29T14:09:11Z

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/HelloServiceImpl.kt

    }

    /**
     * Sends 5 [HelloReply] responses when receiving a request.
     *
     * @see lazyHello(HelloRequest, StreamObserver)
     */
-    override fun lotsOfReplies(request: HelloRequest, responseObserver: StreamObserver<HelloReply>) {
+    override fun lotsOfReplies(request: HelloRequest): Flow<HelloReply> {


Does this need to be flow? I wouldn't expect a for loop in a coroutine to cause any differences compared to our normal withArmeriaContext methods

To clarify, it doesn't matter so much whether it's a flow or not but was hoping withArmeriaContext applies to all the methods without having yet another pattern of flowOn

I’ve tried but it was impossible to apply because withContext only could be applied to suspend function. This function is just return flow and not suspend :-(

Or we can wrap withArmeriaContext with the inside of flow block.

I see - does that mean we can have

flow { withArmeriaContext { for (i...)

?

Definitely not great but I guess a bit better than a new concept flowOn. And probably good evidence to add to our issue about getting better support for customizing the context globally :)

I see - does that mean we can have

Yes, flow takes suspend function.
https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/common/src/flow/Builders.kt#L49

And probably good evidence to add to our issue about getting better support for customizing the context globally :)

That sounds good. 👍

The test failed, the nested block does not work...

flow { withArmeriaContext { for (i...)

emit() should be called from dispatchers of flow block unless it throws ISE.
https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/common/src/flow/Builders.kt#L37-L45

Too bad :( Let's stick with flowOn then since best we can do

minwoox

👍

trustin · 2020-05-06T03:06:57Z

@anuraaga Shall we merge this and revisit when grpc-kotlin is updated, then?

trustin · 2020-05-06T06:52:50Z

Thanks, @ikhoon, @gary-lo and other reviewers! Added @gary-lo as a co-author.

Motivation: gRPC for Kotlin is actively developed and [0.1.1](https://github.com/grpc/grpc-kotlin/releases/tag/v0.1.1) has been released last week. Modifications: - Allow GrpcClientFactory creating client from CoroutineStub - Migrate Kotlin example to gRPC-Kotlin Result: - Partially fixes line#2662 - You can now run Armeria gRPC client with `gproto*` and gRPC-Kotlin CoroutinStub Co-authored-by: Gary Lo <gary.lo@airtasker.com>

ikhoon added the new feature label Apr 16, 2020

ikhoon added this to the 0.99.4 milestone Apr 16, 2020

ikhoon requested review from anuraaga, minwoox and trustin as code owners April 16, 2020 07:37

trustin reviewed Apr 16, 2020

View reviewed changes

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java Outdated Show resolved Hide resolved

gary-lo reviewed Apr 16, 2020

View reviewed changes

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/HelloServiceImpl.kt Outdated Show resolved Hide resolved

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/HelloServiceImpl.kt Outdated Show resolved Hide resolved

gary-lo mentioned this pull request Apr 16, 2020

gRPC Kotlin Example #2668

Closed

anuraaga reviewed Apr 16, 2020

View reviewed changes

minwoox reviewed Apr 17, 2020

View reviewed changes

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java Outdated Show resolved Hide resolved

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/GrpcClientFactory.java Outdated Show resolved Hide resolved

ikhoon mentioned this pull request Apr 17, 2020

Make StubFor access via reflection grpc/grpc-kotlin#63

Merged

anuraaga reviewed Apr 17, 2020

View reviewed changes

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/HelloServiceImpl.kt Show resolved Hide resolved

gary-lo mentioned this pull request Apr 17, 2020

Dispatchers.IO Coroutines Example grpc/grpc-kotlin#60

Closed

anuraaga reviewed Apr 17, 2020

View reviewed changes

ikhoon mentioned this pull request Apr 18, 2020

Provide a way to propagate a custom context to CoroutineContext grpc/grpc-kotlin#66

Closed

ikhoon commented Apr 18, 2020

View reviewed changes

examples/grpc-kotlin/src/test/kotlin/example/armeria/grpc/kotlin/HelloServiceTest.kt Outdated Show resolved Hide resolved

ikhoon force-pushed the grpc-kotlin branch from 9c5e797 to a704ef8 Compare April 24, 2020 05:45

ikhoon force-pushed the grpc-kotlin branch from a704ef8 to 2d0a33c Compare April 24, 2020 05:47

ikhoon added 3 commits April 24, 2020 14:50

Revert gjson to gproto

f9ad40f

Fix wrong dependencies

4c6927f

Kotlin lint

20bc5bf

trustin reviewed Apr 24, 2020

View reviewed changes

gradle/scripts/lib/java-rpc-proto.gradle Show resolved Hide resolved

ikhoon added a commit to ikhoon/gradle-scripts that referenced this pull request Apr 27, 2020

Support gRPC-Kotlin

e8383ed

Generate kotlin stub if a project has `kotlin-grpc` flag and `io.grpc:grpc-kotlin-stub` dependency See line/armeria#2669

ikhoon mentioned this pull request Apr 27, 2020

Support gRPC-Kotlin line/gradle-scripts#73

Merged

ikhoon added 2 commits April 27, 2020 13:29

Check in gRPC-Kotlin stub and exclude it from delete task

f1a8787

Remove kotlin-grpc flag

7f3dfdb

ikhoon force-pushed the grpc-kotlin branch from 9ca9fc6 to 7f3dfdb Compare April 27, 2020 04:36

Fix compile error / Add 'kotlinGrpc' path as source set

e147c3a

trustin reviewed Apr 27, 2020

View reviewed changes

examples/grpc-kotlin/build.gradle.kts Outdated Show resolved Hide resolved

examples/grpc-kotlin/build.gradle.kts Outdated Show resolved Hide resolved

Exclude generated source from ktlint

47bc5ea

trustin approved these changes Apr 29, 2020

View reviewed changes

anuraaga reviewed Apr 29, 2020

View reviewed changes

examples/grpc-kotlin/src/main/kotlin/example/armeria/grpc/kotlin/ArmeriaContext.kt Outdated Show resolved Hide resolved

ikhoon added 2 commits April 29, 2020 20:36

Address comments by @anuraaga / decorate rpc method with armeria context

2f7633a

Add missed one

c0a578c

gary-lo approved these changes Apr 29, 2020

View reviewed changes

anuraaga reviewed Apr 29, 2020

View reviewed changes

minwoox approved these changes May 4, 2020

View reviewed changes

anuraaga approved these changes May 6, 2020

View reviewed changes

trustin merged commit b1decb5 into line:master May 6, 2020

ikhoon deleted the grpc-kotlin branch May 13, 2020 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Kotlin-gRPC client CoroutineStub #2669

Support Kotlin-gRPC client CoroutineStub #2669

ikhoon commented Apr 16, 2020 •

edited

Loading

ikhoon commented Apr 16, 2020

trustin commented Apr 16, 2020

gary-lo commented Apr 16, 2020

codecov bot commented Apr 16, 2020 •

edited

Loading

ikhoon commented Apr 16, 2020

anuraaga left a comment •

edited

Loading

anuraaga commented Apr 17, 2020

ikhoon commented Apr 17, 2020

anuraaga commented Apr 17, 2020 •

edited

Loading

ikhoon commented Apr 17, 2020 •

edited

Loading

anuraaga commented Apr 17, 2020

ikhoon commented Apr 17, 2020

ikhoon commented Apr 18, 2020

anuraaga commented Apr 18, 2020

trustin left a comment

trustin commented Apr 29, 2020

gary-lo left a comment

ikhoon commented Apr 29, 2020

anuraaga Apr 29, 2020

anuraaga Apr 29, 2020

ikhoon Apr 29, 2020

ikhoon Apr 29, 2020 •

edited

Loading

anuraaga Apr 29, 2020

ikhoon Apr 29, 2020

ikhoon Apr 29, 2020 •

edited

Loading

anuraaga Apr 29, 2020

minwoox left a comment

trustin commented May 6, 2020

trustin commented May 6, 2020

Support Kotlin-gRPC client CoroutineStub #2669

Support Kotlin-gRPC client CoroutineStub #2669

Conversation

ikhoon commented Apr 16, 2020 • edited Loading

ikhoon commented Apr 16, 2020

trustin commented Apr 16, 2020

gary-lo commented Apr 16, 2020

codecov bot commented Apr 16, 2020 • edited Loading

Codecov Report

ikhoon commented Apr 16, 2020

anuraaga left a comment • edited Loading

Choose a reason for hiding this comment

anuraaga commented Apr 17, 2020

ikhoon commented Apr 17, 2020

anuraaga commented Apr 17, 2020 • edited Loading

ikhoon commented Apr 17, 2020 • edited Loading

anuraaga commented Apr 17, 2020

ikhoon commented Apr 17, 2020

ikhoon commented Apr 18, 2020

anuraaga commented Apr 18, 2020

trustin left a comment

Choose a reason for hiding this comment

trustin commented Apr 29, 2020

gary-lo left a comment

Choose a reason for hiding this comment

ikhoon commented Apr 29, 2020

anuraaga Apr 29, 2020

Choose a reason for hiding this comment

anuraaga Apr 29, 2020

Choose a reason for hiding this comment

ikhoon Apr 29, 2020

Choose a reason for hiding this comment

ikhoon Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

anuraaga Apr 29, 2020

Choose a reason for hiding this comment

ikhoon Apr 29, 2020

Choose a reason for hiding this comment

ikhoon Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

anuraaga Apr 29, 2020

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

trustin commented May 6, 2020

trustin commented May 6, 2020

ikhoon commented Apr 16, 2020 •

edited

Loading

codecov bot commented Apr 16, 2020 •

edited

Loading

anuraaga left a comment •

edited

Loading

anuraaga commented Apr 17, 2020 •

edited

Loading

ikhoon commented Apr 17, 2020 •

edited

Loading

ikhoon Apr 29, 2020 •

edited

Loading

ikhoon Apr 29, 2020 •

edited

Loading