-
Notifications
You must be signed in to change notification settings - Fork 314
Asynchronously return the request result for folding range requests #849
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.
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
Asynchronously return the request result for folding range requests #849
Conversation
Since `ClangLanguageServerShim` calls directly into `SourceKitServer` we also no longer need the logic to forward a message from clangd to the editor in `handle`.
// We treat a serial queue just like a concurrent queue in which every task | ||
// is a barrier. | ||
let isBarrier = isBarrier || kind == .serial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a holdover from a previous design? Best I can tell, when the kind is .serial
, we don't care about isBarrier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is essential and is where the serial part of the queue comes in. The queue itself doesn’t really have an inherrent notion of being serial or concurrent (we don’t use dispatch queues or anything like that). It’s just that if the queue is serial, every task gets enqueued as a barrier, which means that they will be executed one after the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wait. Actually that’s not true and you are right, we don’t need this. Forget my comment above.
/// This includes requests and notifications sent *from* clangd and does not | ||
/// include replies from clangd. | ||
/// | ||
/// These are requests and notifications sent *from* clangd, not replies from | ||
/// clangd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍🏽
if self.capabilities?.foldingRangeProvider?.isSupported == true { | ||
forwardRequestToClangd(req) | ||
return try await forwardRequestToClangd(req) | ||
} else { | ||
req.reply(.success(nil)) | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the case before, but IMO this would be suitable for a guard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it.
2b0a8fb
to
f74b6b5
Compare
…to `SourceKitServer` This generally seems like the cleaner design because `SourceKitServer` is actually able to semantically inspect the message and decide whether it can be handled concurrently with other requests.
f74b6b5
to
1b6015f
Compare
This is the last infrastructural change to asyncify sourcekit-lsp, everything afterwards should be fairly localized fixes for
FIXME: (async)
comments and the translation of all the other requests to return results asynchronously as a return value.The basic idea is that we change
Connection
to be a class with synchronous functions again. This ensures that all messages within the connection are handles sequentially.SourceKitServer
is responsible for transitioning into the Swift concurrency world by having anAsyncQueue
. That queue is concurrent, which means that requests can be served out-of-order. To ensure that we don’t get out-of-order opens/edits/closes (or any other kind of message that modifies global state), we make these notifications dispatch barriers, which means that all messages before the edit need to be handled before the edit can be processed and all messages received after the edit are handled after the edit has been sent to sourcekitd/clangd.This kind of concurrent execution matches what we are doing in
sourcekitd
where open/edit/close are dispatch barriers.Technically, we could optimize this further by having an
AsyncQueue
for each file, because edits on one file should not block requests on another file from executing but, at least in Swift, this would get us any real benefits at the moment because sourcekitd only has a single, global queue, instead of a queue per file. Additionally, usually you are editing one file in a source editor, which means that concurrent requests to multiple files tend to be rare. Thus, I will not pursue this optimization for now.