Fix broken client request routing and loosen trait bounds #184
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It seems that the previous implementation of server-to-client request routing was broken because of `service.call(request).await` in `Server` blocking forever, preventing the response from being routed back to the awaiting RPC handler. This commit fixes this by rewriting `Server::serve() to use `buffer_unordered()` to drive at least 4 concurrent futures at the same time, thereby avoiding stalling the executor and enabling proper multiplexing of client-to-server and server-to-client communication. As a bonus, we also eliminate one use of `tokio::spawn()` by using `futures::join!()` to drive the printer and reader futures concurrently without depending on a threaded executor. This is a nice step towards eventually decoupling from `tokio`.
This has been a long-standing issue resulting from the strict requirements imposed by the `tokio::spawn()` call in `Server::serve()`. But now that we rely on `join!()` to drive both futures to completion without having to deal with the complexities of a multi-threaded executor, we no longer require that the interleaved stream and `AsyncRead`/`AsyncWrite` handles be `Send + 'static`. This finally allows us to update the `stdio.rs` tests to take mock `stdin` and `stdout` handles as `&mut`, allowing us to inspect the final state once the test is done and the `Server` has shut down.
ebkalderon
changed the title
This was referenced Apr 30, 2020
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changed
tokio::spawn()in favor offutures::join!()(see Avoid calling tokio::spawn internally #180).!Sendand!'staticstreams and IO handles to be used withServer.stdio.rstests to take&mut stdinand&mut stdoutand inspect the two buffers afterwards for correctness. This one was a long time coming!Fixed
buffer_unordered().It seems that server-to-client request routing (introduced in #134) was broken because of
service.call(request).awaitinServerblocking forever, preventing the response from being routed back to the awaiting RPC handler. This commit fixes this issue, while also removing some strict trait bounds on thestdinandstdouthandles, as well as theinterleavestream, and decoupling us a tiny bit further fromtokio.Fixes #183.