Concurrent LSP with cancelation #869
Conversation
andrewbranch
requested review from
Copilot,
jakebailey,
ahejlsberg and
gabritto
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR introduces asynchronous handling and cancellation support to the LSP server by refactoring service methods to be context-aware and by using a CheckerPool for concurrent type checking. Key changes include:
- Adding a context argument to most LanguageService, API, and compiler methods.
- Refactoring checker handling to use a CheckerPool for concurrent requests.
- Updating several conversion and completion functions to work with the new asynchronous model.
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/lsp/lsproto/jsonrpc.go | Added new message types and helper functions for JSONRPC messages. |
| internal/ls/utilities.go | Updated LSP range conversion to remove error handling. |
| internal/ls/languageservice.go | Refactored service creation and file lookup to accept context. |
| internal/ls/hover.go | Updated hover functionality to use context and improved code fencing. |
| internal/ls/host.go | Removed outdated host methods and adjusted interface to use GetLineMap. |
| internal/ls/diagnostics.go | Switched to context-based diagnostics and updated report types. |
| internal/ls/definition.go | Updated definition lookup to be context-aware. |
| internal/ls/converters.go | Refactored converters to use a simplified Script interface without error returns. |
| internal/ls/completions*.go | Modified completion functions to support context and concurrent checkers. |
| internal/api/* | All API methods now accept context; updated server request handling accordingly. |
| internal/compiler/program.go | Replaced direct checker use with CheckerPool calls for concurrency control. |
| internal/compiler/emitHost.go | Adjusted to use context when retrieving type checkers. |
| internal/compiler/checkerpool.go | Added CheckerPool implementation for managing checkers concurrently. |
| internal/checker/* | Minor updates to support new cancellation and context handling. |
|
I approved this but I think you said that there's some bug with duplicate program creation you want to address (so I'll happy look again after too) |
|
|
||
| wg.RunAndWait() | ||
|
|
||
| p.fileAssociations = make(map[*ast.SourceFile]*checker.Checker, len(p.program.files)) |
There was a problem hiding this comment.
this is good for now but in theory the questions are asked only on open files so assign only for those ?
There was a problem hiding this comment.
I meant in editor scenario not cli
There was a problem hiding this comment.
This implementation is only used in the CLI. In the editor this map is created with no capacity hint.
|
|
||
| textChanges := make([]ls.TextChange, len(changes)) | ||
| for i, change := range changes { | ||
| if partialChange := change.TextDocumentContentChangePartial; partialChange != nil { |
There was a problem hiding this comment.
There is delayUpdateProjectGraph in this file- in strada we use to schedule the update (unless new change came in - then we would cancel and restart the timer) - with this change does this mean - send the refresh diagnostics notification - instead of updating as we want to update on next request instead ?
There was a problem hiding this comment.
We don't send the diagnostic refresh notification from here. The client will ask for new diagnostics after an update, so I don't think there's any need for us to schedule anything.
This PR asyncifies the LSP serve and enables parallel execution of requests.
At the top level, one goroutine reads and parses messages from STDIN and puts them onto a queue to be handled. Another pulls from that queue and dispatches them to the project service, and puts the response objects onto another queue. A third goroutine pulls the response messages and writes them to STDOUT.
The dispatcher categorizes requests into ones that modify state and ones that only query the existing state. The former are handled synchronously within the dispatcher goroutine, while the latter are spun off into their own goroutines so they can execute in parallel.
Executing multiple requests in parallel agains the same program introduces two challenges:
CheckerPoolinterface whose methods for getting a checker take acontext.Contextand return an available checker, along with a function to return it to the pool when done. The implementation incompilerused fortscis just the logic that was already built into Program, while the implementation inprojectcreates checkers dynamically and tracks which ones are in use by which requests.lsprototypes (which are exclusively line/character) so all the conversions have to happen in that layer. Then, I removed LanguageService’s ability to access any mutable state (basically). When it asks for the line map of a file, its host attempts to pull the cached one from the text storage layer only if the versions match. Otherwise, the text storage has been updated by another request, and the line map for the old version is computed on demand from the SourceFile text, which is immutable.