Migrate huge chunks of sourcekit-lsp to actors/async/await #850
Conversation
This file contains hidden or 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
When we switch `SourceKitServer`, `SwiftLanguageServer` etc. to be actors, we can’t rely on them to provide ordering guarantees anymore because Swift concurrency doesn’t provide any ordering guarantees. What we should thus do, is to handle all messages on a serial queue on the `Connection` level. This queue will be blocked from handling any new messages until a message has been sufficiently handled to avoid out-of-order handling of messages. For sourcekitd, this means that a request has been sent to sourcekitd and for clangd, this means that we have forwarded the request to clangd. Note that this serial queue is not the main thread, so we will continue accepting data over stdin, just the handling of those messages is blocked.
This is the prerequisite for making `SourceKitServer` an actor, which will mean that the `handle` methods will be `async`. The current paradigm of returning from `handle` once we can guarantee that there’s no out-of-order execution and then returning the actual result via the callback that’s attached to `Request` is a little weird still. I am hoping to change this paradigm to return the actual result and have a callback function that `handle` can call to indicate that it’s ready to accept another message while guaranteeing in-order execution, essentially flipping the role of the return value and the closure callback. But that’s something to be done after the entire stack has been asyncificied.
Unfortuantely, we have a few potential out-of-order exeuction possibilities while we migrate everything else to also be asyncronous. But those should be low-probability issues that we can fix in follow-up commits, so I think it’s fine for now. All of these places are marked with `FIXME: (async)`
This is a preparation step for making `SwiftLanguageServer` and `ClangLanguageServerShim` actors.
…build settings Instead of storing build settings inside the language servers based on update notifications from the build system, always call into the `BuildSystemManager` to get the build settings. Overall, I think this is a much clearer separation of concerns and will allow us to remove `SourceKitServer.documentToPendingQueue` in a follow-up commit as `SwiftLanguageServer` can always directly call into `BuildSystemManager` to get build settings and we don’t need to wait for the initial notification to receive the first build settings. This requies `BuildServerBuildSystem` to keep track of the build settings it has received from the BSP server. `ClangLanguageServer` still caches build settings locally. `ClangLanguageServer` will change to the same pull-based model in a follow-up commit.
Fairly straightforward since we have all the infrastructure now.
…get build settings The same kind of change that we did for `SwiftLanguageServer`. Instead of caching build settings inside `ClangLanguageServerShim`, always call into `BuildSystemManager` to get the build settings.
…o be connected again
…ived a reply instead of until it has been handled This was causing the sourcekit-lsp integration test to fail.
Again, fairly straightforward.
This is the prerequisite for making the build systems actors.
…ldSystemDelegate` async This concludes the migration of the build systems to async.
…`nil` Without this overload, `T` was inferred to be `Something?`, thus the first parameter was inferred to be `Something??` and the first parameter was always wrapped in an optional, effectively making `if let optional` never be hit.
We want to implement cancellation on top of Swift concurrency’s cancellation, which will most likely need a completely different paradigm.
…to SourceKitServer instead of through a LocalConnection `LocalConnection` with its dynamic registration of a message handler made the overall design unnecessarily complicated. If we just call `SourceKitServer` from `ClangLanguageServerShim` and `SwiftLanguageServer` directly, it’s a lot more obvious, what’s going on, IMO.
…mManagerTests` to get build settings
…not about new build settings This defines away an entire class of data races if delegate callbacks are delivered out-of-order. If we aren’t providing the new build settings in the delegate callback, then it doesn’t matter if two `fileBuildSettingsChanged` calls change order since they don’t carry any state.
…d `BuildSystemManager` The core idea here is that the toolchain language servers always call into `BuildSystemManager` and `BuildSystemManager` will always reply with build settings. If it hasn’t computed them yet, it will reply with fallback settings. With that assumption, we can remove the `documentToPendingQueue` from `SourceKitServer` since there are no longer any documents that are pending – everything has a build settings immediately. Similarly, `BuildSystemManager.mainFileStatuses` also isn’t needed anymore. And lastly, since we know that `BuildSystemManager.buildSettings` will always return a value `registerForChangeNotifications` is changed not call `fileBuildSettingsChanged` immediately. Instead, it will only cause `fileBuildSettingsChanged` to be called when the file’s build settings change after the `registerForChangeNotifications` call.
Keep track of build settings in `BuildSystemManager` instead of `SourceKitServer`
These are no longer needed as all callers are async 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`.
…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.
Asynchronously return the request result for folding range requests
Fix a couple `FIXME: (async)`
…-op functions from it
The motivating change for this was to deterministically pick a main file for a header file instead of picking the first element in a set, which is not deterministic. While doing this, I also changed the main file computation to not carry any state about previous main files around. If a header file is associated with a main file b.cpp and a new a.cpp gets added that imports the header as well, we should be using a.cpp for the build settings. That way we will get the same build settings if we close and re-open the project. And this was a good opportunity to refactor some of the main file handling into smaller, more dedicated functions.
…tation Refactor the computation of main files
|
@swift-ci Please test |
|
Previous test passed, running again to check if there are non-deterministic failures. @swift-ci Please test |
|
macOS and Linux passed, Windows failed for an unrelated reason. Triggering a third time and will merge afterwards. @swift-ci Please test |
|
@swift-ci Please test Windows |
hamishknight
approved these changes
Oct 4, 2023
MaxDesiatov
added a commit
to swiftlang/swift-package-manager
that referenced
this pull request
Dec 12, 2023
### Motivation: Using locks to convert asynchronous functions to synchronous is dangerous, since this can deadlock the Swift Concurrency thread pool. Unfortunately, `temp_await` (AKA `tsc_await`) function utilizes exactly this approach. As SourceKit-LSP starts adopting Swift Concurrency (swiftlang/sourcekit-lsp#850), we should avoid using `temp_await` in our codebase and make the callers of corresponding `async` or callback-taking functions themselves `async`. Otherwise there's a risk of deadlocks in SourceKit-LSP itself, since their Swift Concurrency tasks may be blocked by our uses of `temp_await` under the hood. ### Modifications: `loadRootPackage` already has an `async` overload in `Workspace`. Converted callers in SwiftPM commands to use `AsyncSwiftCommand` protocol instead of `SwiftCommand`. Also, test cases were made `async` too. ### Result: Reduced the number of `temp_await` uses in `Source`. rdar://79350642
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.
All of the changes here have been reviewed individually. The purpose of this PR is to integrate the changes into
mainand make sure that CI passes consistently.