Skip to content

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

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Oct 3, 2023

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 an AsyncQueue. 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.

Since `ClangLanguageServerShim` calls directly into `SourceKitServer` we also no longer need the logic to forward a message from clangd to the editor in `handle`.
Comment on lines 87 to 89
// We treat a serial queue just like a concurrent queue in which every task
// is a barrier.
let isBarrier = isBarrier || kind == .serial
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member Author

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.

Comment on lines 50 to 51
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: duplication

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍🏽

Comment on lines 578 to 576
if self.capabilities?.foldingRangeProvider?.isSupported == true {
forwardRequestToClangd(req)
return try await forwardRequestToClangd(req)
} else {
req.reply(.success(nil))
return nil
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it.

@ahoppen ahoppen force-pushed the ahoppen/async-request-result-return branch from 2b0a8fb to f74b6b5 Compare October 3, 2023 14:54
…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.
@ahoppen ahoppen force-pushed the ahoppen/async-request-result-return branch from f74b6b5 to 1b6015f Compare October 3, 2023 14:57
@ahoppen ahoppen merged commit 6206585 into swiftlang:ahoppen/asyncification Oct 3, 2023
@ahoppen ahoppen deleted the ahoppen/async-request-result-return branch October 3, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants