Fix a race condition in code completion #899
Conversation
|
@swift-ci Please test |
| private static let completionQueue = AsyncQueue(.serial) | ||
|
|
||
| /// The code completion session for each sourcekitd instance. | ||
| /// | ||
| /// `sourcekitd` has a global code completion session, that's why we need to | ||
| /// have a global mapping from `sourcekitd` to its currently active code | ||
| /// completion session. | ||
| /// | ||
| /// Modification of code completion sessions should only happen on | ||
| /// `completionQueue`. | ||
| private static var completionSessions: [ObjectIdentifier: CodeCompletionSession] = [:] |
There was a problem hiding this comment.
Can't say I love this global state, is there any reason not to store it on SourceKitDImpl instead? FWIW the abstraction with the SourceKitD protocol doesn't seem super compelling to me since it seems like the only other conforming type is FakeSourceKitD, and that's only used to test the registry, it doesn't implement any of the other requirements. Seems like we could turn SourceKitDRegistry it into a generic DylibRegistry<T> type or something, and then delete the SourceKitD protocol.
There was a problem hiding this comment.
Removing the abstraction of SourceKitD an SourceKitDImpl is also something that’s on my mind. Expect a follow-up PR for that sometime in the future.
Can't say I love this global state, is there any reason not to store it on
SourceKitDImplinstead?
It’s a layering decision: CodeCompletionSession is tied quite closely to the needs of LSP (it takes DocumentSnapshot and returns CompletionList, both of which are defined in LanguageServerProtocol) while the SourceKitD module is a fairly low-level wrapper around the C sourcekitd API. So, I think CodeCompletionSession doesn’t belong in the SourceKitD module and thus it can’t be a member of SourceKitD (unless you are suggesting that SourceKitDImpl has something like codeCompletionSession: Any and I assume that’s not what you are suggesting).
There was a problem hiding this comment.
Hmm yeah that's fair, it's unfortunate because it seems like completion sessions are a sourcekid-specific concept. If we wanted to sink CodeCompletionSession down to a lower layer, it does seem like it doesn't have to return a CompletionList, it could return the raw response and let the language server handle transforming it. And it also seems like it doesn't need to store a full DocumentSnapshot, it only needs the DocumentURI I think (though maybe there's a use I'm missing). In particular it looks like the use in guard snapshot.version == self.snapshot.version is now redundant since that should be true by construction.
Either that or we could have a higher level wrapper that encapsulates both a SourceKitD and any session state it needs.
Doesn't have to be done in this PR, but I do think there's probably a better way to model this without the global state.
There was a problem hiding this comment.
Let’s re-consider when we remove the SourceKitD/SourceKitDImpl layering. I filed #901
ahoppen
force-pushed
the
ahoppen/completion-race-condition
branch
from
79d2363
to
b38dd7f
Compare
ahoppen
force-pushed
the
ahoppen/completion-race-condition
branch
from
b38dd7f
to
29f4b54
Compare
|
@swift-ci Please test |
|
@swift-ci Please test macOS |
`SKCompletionOptions.serverSideFiltering` is `true` by default and I know of no editor that disables it. Delete it. Fixes apple#863 rdar://116703670
…ourceKitServer` This just clean up the layering slightly.
Each `sourcekitd` uses a single, global code completion session but we were managing code completion sessions on the `SwiftLanguageServer` level. This had two issues: - Code completion is considered non-blocking on `SourceKitServer` and thus we could execute two concurrent code completion requests, which would conflict with each other. - You could have multiple `SwiftLanguageServer`s that each have a connection to `sourcekitd` but share the same `sourcekitd` state. This shouldn't happen in the real world but does happen if we create multiple `SourceKitServer` instances in tests.
ahoppen
force-pushed
the
ahoppen/completion-race-condition
branch
from
29f4b54
to
16aa082
Compare
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
Each
sourcekitduses a single, global code completion session but we were managing code completion sessions on theSwiftLanguageServerlevel. This had two issues:SourceKitServerand thus we could execute two concurrent code completion requests, which would conflict with each other.SwiftLanguageServers that each have a connection tosourcekitdbut share the samesourcekitdstate. This shouldn't happen in the real world but does happen if we create multipleSourceKitServerinstances in tests.As a preparation for this do the following:
SKCompletionOptions.serverSideFilteringistrueby default and I know of no editor that disables it. Delete it. (Fixes Remove client-side filtering for code completion #863 / rdar://116703670)CodeCompletionSessionto not have a circular reference toSourceKitServer