Return responses in corresponding request order #212
Merged
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
According to these three discussions on GitHub: microsoft/language-server-protocol#12 microsoft/language-server-protocol#306 microsoft/language-server-protocol#584 While the server implementation is free to process incoming requests concurrently in whatever order it wants, it is generally preferred for servers to send responses back to the client in the same order the requests were sent, for the sake of convenience on the client side. To achieve this, we switch the message stream from `buffer_unordered(4)` to `buffered(4)` instead. As before, at most 4 futures race to completion concurrently with one another, but the responses now have to wait for the others started before them to complete in order to preserve correct message transmission order. However, this introduces a potential for deadlocks when considering RPC method handlers which need to send a request back to the client and await for a response back as part of computing the final result. Imagine a scenario where a flood of client-to-server requests come in all at once over `stdin`, where each of the method handlers happen to issue one or more server-to-client requests as part of their work. The handlers for these incoming requests exhaust all 4 available execution slots in the buffer, issue server-to-client requests, and await the responses. At this point, the `Buffered<St>` is blocked and the `mspc::channel(1)` buffer has filled up as well, temporarily blocking `framed_stdin` from processing new messages as a form of backpressure. Unfortunately, this means that the pending RPC handlers will likely never receive the responses they require from the client, and the 4 pending futures will block forever. While this is generally less likely to occur with `buffer_unordered(4)` (the lack of an order requirement means that quickly-resolving futures can exit the buffer quickly, compensating for any blocked futures and making room for more incoming requests to be processed), it's still possible to stall the pipeline given enough volume from the client. To fix this issue while keeping the request/response ordering requirement, we can either increase the buffer size of `mpsc::channel()` or switch to an `mpsc::unbounded()` to allow the `framed_stdin` stream to keep moving forward even when all 4 slots in `Buffered<St>` are full. Given that having no backpressure mechanism is probably inadvisable, we opt for increasing the `mspc::channel()` buffer size from 1 to 16. At this point, even if all 4 available buffer slots are full, we can skip forward at least 16 JSON-RPC messages ahead in `framed_stdin` and process the incoming responses from the client, if any. Typing very quickly in my editor with a simple language server in place, this seems like a reasonable value to me.
ebkalderon
force-pushed
the
enforce-response-ordering
branch
from
2d818d9
to
030d3c0
Compare
|
After waiting a while and pondering the implications of this change, I think this should be okay to merge. We can always easily revert to the old behavior should issues arise, and it should not constitute a semver breaking change (request/response ordering isn't strictly defined in either JSON-RPC nor LSP; the change this PR lands is purely advisory). |
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
While the server implementation is free to process incoming requests concurrently in whatever order it wants, it is generally preferred for servers to send responses back to the client in the same order the requests were sent, for the sake of convenience on the client side.
To ensure we don't stall the executor in cases of exceptionally high bidirectional traffic, we also increase the minimum buffer size for processing
stdinmessages from 1 to 16. In practice, typing extremely quickly in my own editor, this seemed to be a pretty reasonable threshold.