Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify that the current protocol requires that requests and notification are processed in order #12

Closed
dbaeumer opened this issue May 25, 2016 · 3 comments
Closed

Clarify that the current protocol requires that requests and notification are processed in order #12

dbaeumer opened this issue May 25, 2016 · 3 comments
Labels
clarification
Milestone
November 2016

Comments

@dbaeumer
Copy link
Member

The current protocol requires that request and notifications send from the client to the server are processed in the same order as they are sent.
@dbaeumer dbaeumer mentioned this issue May 25, 2016
Closed
@svenefftinge svenefftinge mentioned this issue May 26, 2016
Closed
@svenefftinge svenefftinge mentioned this issue Jun 3, 2016
Closed
@dbaeumer dbaeumer added this to the July 2016 milestone Jun 23, 2016
@bruno-medeiros bruno-medeiros mentioned this issue Sep 30, 2016
Closed
@akosyakov
Copy link
Contributor

@dbaeumer Does it mean that requests and notifications should no be processed concurrently?

In regards to #73. Is it expected that didChange should be processed only after completion is finished successfully or by cancelation from the client?

@bruno-medeiros
Copy link

I would suggest that requests should be processed sequentially as far as the client can tell, but internally they can be processed concurrently if it is guaranteed the result is the same as if they were processed sequentially. For example mutiple read-only/query operations can be processed concurrently, obviously.

As for mixing read-only and write operations (like didChange), for example in the scenario @akosyakov mentioned above: if a didChange request cames after a completion request, the server can just automatically cancel the previous completion (and all other pending queries), thus ensuring sequentiality.

If we wanted to get smart, the server could try to detect if the didChange actually affects the previous completion request, but I imagine for 99.9% of user IDE cases, it does invalidate the previous request, so there is really little value in the server trying to be this smart. (plus it might complicate LSP protocol implementation)

@dbaeumer dbaeumer modified the milestones: July 2016, November 2016 Oct 14, 2016
bruno-medeiros added a commit to bruno-medeiros/rls that referenced this issue Nov 11, 2016
@bruno-medeiros
Use RustLSP library.
9a61226 
A more extensive and principled way to handle the LSP protocol. Adds
*interface* support for all LSP requests.

Fixes:
* 'completionItem/resolve' should return CompletionItem not a vector
* Concurrent handling of requests. Requests must be handled in order,
otherwise, for example, text document changes could be applied out of
order. Only read-only requests can conceptually be executed in parallel
(since that would have the same result as if they were processed in
order). See here for more info:
microsoft/language-server-protocol#12

TODO for RustLSP:
 * Cancel support
 * Support sending requests to client from server 
(only notifications can be sent currently). This is required for one LSP
request: 'window/showMessageRequest'. 
 * Perhaps integrate with https://github.com/ethcore/jsonrpc-core and
share part of the JSON-RPC implementation?
@bruno-medeiros bruno-medeiros mentioned this issue Nov 11, 2016
Closed
bruno-medeiros added a commit to bruno-medeiros/rls that referenced this issue Nov 24, 2016
@bruno-medeiros
Use RustLSP library.
a6c54d7 
A more extensive and principled way to handle the LSP protocol. Adds
*interface* support for all LSP requests.

Fixes:
* 'completionItem/resolve' should return CompletionItem not a vector
* Concurrent handling of requests. Requests must be handled in order,
otherwise, for example, text document changes could be applied out of
order. Only read-only requests can conceptually be executed in parallel
(since that would have the same result as if they were processed in
order). See here for more info:
microsoft/language-server-protocol#12

TODO for RustLSP:
 * Cancel support
@dbaeumer
Copy link
Member Author

I added the following to the spec to clarify this:

Request, Notification and response ordering

Responses for requests should be sent in the same order as the requests appear on the server or client side. So for example if a server receives a textDocument/completion request and then a textDocument/signatureHelp request it should first return the response for the textDocument/completion and then the reponse for textDocument/signatureHelp.

How the server internally processes the requests is up to the server implementation. If the server decides to execute them in parallel and this produces correct result the server is free to do so. The server is also allowed to reorder requests and notification if the reordering doesn't affect correctness.

dbaeumer added a commit that referenced this issue Dec 29, 2016
@dbaeumer
Fixes #12: Clarify that the current protocol requires that requests a…
801a88a 
…nd notification are processed in order
bruno-medeiros added a commit to bruno-medeiros/rls that referenced this issue Dec 30, 2016
@bruno-medeiros
Use RustLSP library.
145f5b2 
A more extensive and principled way to handle the LSP protocol. Adds
*interface* support for all LSP requests.

Fixes:
* 'completionItem/resolve' should return CompletionItem not a vector
* Concurrent handling of requests. Requests must be handled in order,
otherwise, for example, text document changes could be applied out of
order. Only read-only requests can conceptually be executed in parallel
(since that would have the same result as if they were processed in
order). See here for more info:
microsoft/language-server-protocol#12

TODO for RustLSP:
 * Cancel support
@jonasbb jonasbb mentioned this issue Jun 19, 2017
Closed
@vladdu vladdu mentioned this issue Jul 31, 2017
Closed
@tomv564 tomv564 mentioned this issue Oct 3, 2017
Closed
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
clarification
Projects
None yet
Milestone
November 2016
Development

No branches or pull requests
3 participants
@dbaeumer @akosyakov @bruno-medeiros