Function returning ReceiveChannel should not be marked with suspend

[jcornaz]

Hello,

May I ask why the functions returning ReceiveChannel are marked with suspend?

This is not necessary and a bit confusing.

Here is what I would personnaly expect:

// server streaming rpc
override fun greetServerStream(request: GreetRequest) = produce<GreetReply> { // <-- produce starts a coroutine and doesn't require to suspend
	send(GreetReply.newBuilder() // <-- here we suspend the coroutine started by `produce`. It doesn't require `greetServerStream` to suspend.
		.setReply("Hello ${request.greeting}!")
		.build())
	send(GreetReply.newBuilder()
		.setReply("Greetings ${request.greeting}!")
		.build())
}

// bidirectional rpc
override fun greetBidirectional(requestChannel: ReceiveChannel<GreetRequest>) = produce<GreetReply> { // <-- produce starts a coroutine and doesn't require to suspend
	var count = 0

	for (request in requestChannel) { // <-- here we suspend the coroutine started by `produce`. It doesn't require `greetBidirectional` to suspend.
	  val n = count++
	  launch {
		delay(1000)
		send(GreetReply.newBuilder()
			.setReply("Yo #$n ${request.greeting}")
			.build())
	  }
	}
}

[rouzwawi]

Hi @jcornaz, this was introduced in #9 with the reasoning that the handler could make suspending calls to other services.

Now in hindsight (and after seeing some more usage of suspend vs Channel) I'm inclined to remove the suspend modifier from server streaming calls. I think the original reason implies that other functions are mixing suspend and channels, which should be avoided.

I remember a rule of thumb from some coroutines talk

• use suspend for functions that do some work, potentially suspending and then return a result value
• use channels for functions that set up a structure for future work and return immediately

[jcornaz]

I remember a rule of thumb from some coroutines talk

• use suspend for functions that do some work, potentially suspending and then return a result value
• use channels for functions that set up a structure for future work and return immediately

Exactly.

the handler could make suspending calls to other services.

It is possible even if the function is not marked suspend. And that's the point.

Example:

fun foo(): ReceiveChannel<String> = GlobalScope.produce {
  service.doSomethingUseful() // <-- suspending call to another service. It is fine, since we are in a coroutine.

  send("Hello")
  send("world")
}

[jcornaz]

Just to come back on what has been said in #9 by @wfhartford:

Currently RPCs which return a single value are suspend functions, it probably makes sense to be consistent, making all service functions suspend

I think it does not make sense. Marking a function suspend is the equivalent of returning a CompletableFuture, Single, Maybe etc. with other technologies.

Returning a ReceiveChannel is the equivalent of returning Observable, Flow, etc. with other technologies.

Therefore marking a function suspend and returning a ReceiveChannel sounds like if we would create a function returning Single<Observable<T>>... Which doesn't really make sense, does it?

This also allows certain natural implementations other than using the produce function. In my case, my service function is calling to another suspend function which returns a ReceiveChannel, map is then used to transform the results from that channel into gRPC response objects.

This use-case is not valid IMHO for 2 reasons:

1. The "other" function which suspend and return a ReceiveChannel is badly designed for the same reason. As said above, a function should either be marked suspend and return a value, or not suspend and return a channel. But not both.
2. It is anyway possible to use it:

  suspend fun otherFunctionBadlyDesigned(): ReceiveChannel<String> =...

  fun goodFunction(): ReceiveChannel<Int> = produce {
     otherFunctionBadlyDesigned().map { it.length }.consumeEach { send(it) }
  }

[wfhartford]

Regarding my comments in #9, I must have come to the same conclusion that @jcornaz did, that something wasn't very well designed there, since that code is now significantly different. All of my current use-cases of response message streaming use the produce pattern, so do not benefit from the function being marked as suspend.

[voidzcy]

Since the GreeterImpBase class has suspend keyword for those functions, removing it in GreeterImp cause the compilation to fail. They need to be consistent.

[jcornaz]

Since the GreeterImpBase class has suspend keyword for those functions, removing it in GreeterImp cause the compilation to fail. They need to be consistent.

I think there is a mismatch in the documentation currently because the readme is documenting the new style available in the master branch, but it has not been released yet.

@voidzcy, here is the version of the readme you want to read: https://github.com/rouzwawi/grpc-kotlin/blob/v0.1.0/README.md

[voidzcy]

@jcornaz Cool. Thanks.