Auto-dispatch to I/O context #218
Conversation
jhump
force-pushed
the
jh/auto-coroutine
branch
from
0dc0249 to
93ab537
Compare
| @@ -46,92 +45,3 @@ interface HTTPClientInterface { | |||
| */ | |||
| fun stream(request: HTTPRequest, duplex: Boolean, onResult: suspend (StreamResult<Buffer>) -> Unit): Stream | |||
| } | |||
|
|
|||
| interface Stream { | |||
There was a problem hiding this comment.
Minor cleanup in here: I hoisted the Stream stuff out to a separate Stream.kt file.
| headers: Headers, | ||
| ): BidirectionalStream<Input, Output> = suspendCancellableCoroutine { continuation -> |
There was a problem hiding this comment.
More cleanup: the suspendCancellableCoroutine usage here didn't really do anything.
It tried to associate cancellation of the stream with the coroutine being cancelled. But this only does anything for the duration of this method body. Since the method always immediately resumes the continuation, this didn't really do anything in practice. So I've removed it for clarity (and replaced it with a try/catch).
Now, it's no longer a suspendable function (so can't be cancelled while executing anyway). Instead, the previous PR (#213, that adds auto-closing block-based RPC invocations to the conformance client) demonstrates the better (IMO) way forward for making sure that the RPCs are appropriately cancelled when a coroutine gets cancelled.
jhump
force-pushed
the
jh/auto-coroutine
branch
from
93ab537 to
5e55820
Compare
jhump
force-pushed
the
jh/auto-coroutine
branch
from
5e55820 to
7e6334e
Compare
| lifecycleScope.launch(Dispatchers.IO) { | ||
| stream.send(ConverseRequest.newBuilder().setSentence(sentence).build()) | ||
| } | ||
| buttonView.setOnClickListener { |
There was a problem hiding this comment.
Should code that deals with the UI still be dispatched to Dispatchers.Main? I'm not familiar with android development but we might want to keep this here.
There was a problem hiding this comment.
The documentation for LifecycleCoroutineScope (the type returned by lifecycleScope) states that it is already tied to Dispatchers.Main. So the above call to lifecycleScope.launch should already dispatch in the right context.
I can set it explicitly above if you'd prefer. I wasn't really sure what was the most idiomatic for Android apps -- to omit when using the default or to always specify explicitly. I guess, even if it is idiomatic to omit, we might want to state it explicitly, just for maximum clarity.
There was a problem hiding this comment.
Ok - I was looking for that documentation and didn't find that information. I think this is ok as-is.
This adds a new property to
ProtocolClientConfigcalledioCoroutineContext. It defaults to null, which means there is no change in behavior from the current defaults -- operations happen in the calling coroutine context, so callers of RPC methods must handle the dispatch.But when it is configured as non-null (most likely using a value like
Dispatchers.IO), then I/O and blocking operations for an RPC are automatically dispatched to the given coroutine context. This allows for RPC client code to be used from any coroutine context without having to worry about explicitly dispatching.