Current Implementation Design

[marcoferrer]

Reviewing the current implementation there are some choices that stood out that remind me of early versions of Kroto. A few of these caused bugs that were difficult to debug due to the way Kotlin resolves extension function receivers in nested scopes. Leaking resources and breaking structured concurrency.

Is there a formal process for providing feedback or should feedback be reserved until after an initial release?

[chalin]

Hi Marco, thanks for your offer to give feedback.

Is there a formal process for providing feedback or should feedback be reserved until after an initial release?

There is no formal process, and I don't see any reason to wait.

If there are separate points that you'd like to raise, feel free to open separate issues for each of them (it'll be easier to track resolution that way). Or maybe just start a discussion here.

That's my 2 cents. I'll let @lowasser @bshaffer comment further if they feel the need.

[lowasser]

I think we might even actively prefer feedback before the initial release.

[lowasser]

Bump. @marcoferrer , we'd love to hear your feedback as soon as you're willing to give it.

[marcoferrer]

I realized the abstract service base implements CoroutineScope. Usually, coroutine scope is meant to be implemented by types with a finite lifecycle. Making it a miss-match for classes whose life cycle is bound to that of the entire application. I noticed it does leverage a supervisor job which does mitigate some potential issues but there is one that's harder to avoid. Implicit receivers for extensions functions. Since a common convention of the coroutines API is using extension functions on scopes it is really easy for a service to orphan a child coroutine and break structured concurrency.

class MyService : MyServiceBaseImpl {
	
	fun someUnaryCall(req: Request): Response {
		return calculateA() + doSomething()
	}

	suspend doSomething() {
		// Implcitly gets orphan context from class instead of 
		// showing error that  coroutineScope {} builder is needed
		val asyncVal1 = async { "something1" }
		val asyncVal2 = async { "something2" }

		return asyncVal1.awat() +  asyncVal2.await()
	}

}

Early implementation of Kroto took this approach and users reported issues where they were receiving an unexpected coroutineContext in non API methods.

Another thing I wanted to mention is a little more contentious and that the interface for streaming API methods. I'll give my feedback but this has been open to debate and there is no real clear answer. When using flow as a request and return type for streaming API there are two common caveats.

The first being that on the client-side, it is difficult for a request flow to emit values based on values received from the response flow. This was a common pattern that we had a hard time trying to support with a flow based design.

The second being that once you have begun collecting your response flow the ergonomics of canceling an rpc you didn't need anymore were error-prone and could potentially cancel your whole coroutine. Flows use exception throwing to signal cancellation

[lowasser]

I am 100% persuaded to abandon implementing CoroutineScope in server stubs.

As far as client-side flow-to-flow logic, I think the sanest way to deal with that is probably to fall back to channels, and adapt from channels to flows via produceIn and consumeAsFlow. (I find myself wondering if there's a good way to factor that logic out to make a recursively defined flow, whether or not the transformation operation comes from an RPC.)

[lowasser]

We did some significant updates to the design before launch as a result of this feedback. Thanks for letting us know your thoughts!

[asarkar]

@marcoferrer wrote:

The first being that on the client-side, it is difficult for a request flow to emit values based on values received from the response flow.

This is exactly my use case, and I solved it by having a requestChannel: Channel; the requestChannel is passed to the stub asFlow, making it a hot flow, and to the response handler, that simply sends to the channel if it needs to.

ergonomics of canceling an rpc

Huh, this is my use case too, and I opened #153. I don't want to throw an exception in the flow response handler for a no-complaint cancellation, it is bad design to use exceptions as control flow.

After looking into the source code for this library, it seems currentCoroutineContext()[Job]?.cancel() will do the job. https://github.com/grpc/grpc-kotlin/blob/master/stub/src/main/java/io/grpc/kotlin/ClientCalls.kt#L321