Introduce a BarrierRequest that ensures all messages are handled before it replies

We could get into a race condition in `testAddFile` where the `DidChangeWatchedFilesNotification` would get handled after we try getting completion results that rely on it.

In a real-world use case, this is OK. Completion might still be incorrect until `DidChangeWatchedFilesNotification` gets handled but it will catch up eventually  - usually earlier than later because in real-world scenarios the `DidChangeWatchedFilesNotification` and completion request are more than a few milliseconds apart.

In test, however, we need to guarantee deterministic ordering. Introduce a `BarrierRequest` that has `TaskMetadata.globalConfigurationChange` and thus ensures that all notifications and requests before it have finished before returning. We can run this fake request after sending the `DidChangeWatchedFilesNotification` to make sure that it is handled.

An alternative would be to mark `DidChangeWatchedFilesNotification` as `TaskMetadata.globalConfigurationChange`. But I would really like to avoid introducing a global ordering barrier between requests for a notification that is, for example, sent whenever a `.swift` file in the `.build` directory changes (e.g. on every package update).

Author: ahoppen

Sources/LanguageServerProtocol/CMakeLists.txt

@@ -27,6 +27,7 @@ add_library(LanguageServerProtocol STATIC
   Notifications/WorkDoneProgress.swift
 
   Requests/ApplyEditRequest.swift
+  Requests/BarrierRequest.swift
   Requests/CallHierarchyIncomingCallsRequest.swift
   Requests/CallHierarchyOutgoingCallsRequest.swift
   Requests/CallHierarchyPrepareRequest.swift

Sources/LanguageServerProtocol/Messages.swift

@@ -17,6 +17,7 @@
 /// `MessageRegistry._register()` which allows you to avoid bloating the real server implementation.
 public let builtinRequests: [_RequestType.Type] = [
   ApplyEditRequest.self,
+  BarrierRequest.self,
   CallHierarchyIncomingCallsRequest.self,
   CallHierarchyOutgoingCallsRequest.self,
   CallHierarchyPrepareRequest.self,

Sources/LanguageServerProtocol/Requests/BarrierRequest.swift

@@ -0,0 +1,20 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+/// A no-op request that ensures all previous notifications and requests have been handled before any message
+/// after the barrier request is handled.
+public struct BarrierRequest: RequestType {
+  public static var method: String = "workspace/_barrier"
+  public typealias Response = VoidResponse
+
+  public init() {}
+}

Sources/SourceKitLSP/SourceKitServer.swift

@@ -215,6 +215,8 @@ fileprivate enum TaskMetadata: DependencyTracker {
       self = .globalConfigurationChange
     case is WorkspaceSymbolsRequest:
       self = .freestanding
+    case is BarrierRequest:
+      self = .globalConfigurationChange
     case is PollIndexRequest:
       self = .globalConfigurationChange
     case let request as ExecuteCommandRequest:
@@ -734,6 +736,8 @@ extension SourceKitServer: MessageHandler {
       await self.handleRequest(request, handler: self.workspaceSymbols)
     case let request as Request<PollIndexRequest>:
       await self.handleRequest(request, handler: self.pollIndex)
+    case let request as Request<BarrierRequest>:
+      await self.handleRequest(request, handler: { _ in VoidResponse() })
     case let request as Request<ExecuteCommandRequest>:
       await self.handleRequest(request, handler: self.executeCommand)
     case let request as Request<CallHierarchyIncomingCallsRequest>:

Tests/SourceKitLSPTests/CompilationDatabaseTests.swift

@@ -64,6 +64,9 @@ final class CompilationDatabaseTests: XCTestCase {
       ])
     )
 
+    // Ensure that the DidChangeWatchedFilesNotification is handled before we continue.
+    _ = try await ws.testClient.send(BarrierRequest())
+
     // DocumentHighlight should now point to the definition in the `#else` block.
 
     let expectedPostEditHighlight = [

Tests/SourceKitLSPTests/SwiftPMIntegration.swift

@@ -134,6 +134,9 @@ final class SwiftPMIntegrationTests: XCTestCase {
       ])
     )
 
+    // Ensure that the DidChangeWatchedFilesNotification is handled before we continue.
+    _ = try await ws.testClient.send(BarrierRequest())
+
     let completions = try await ws.testClient.send(
       CompletionRequest(textDocument: TextDocumentIdentifier(newFileUri), position: newFilePositions["2️⃣"])
     )

Tests/SourceKitLSPTests/WorkspaceTests.swift

@@ -272,6 +272,9 @@ final class WorkspaceTests: XCTestCase {
       ])
     )
 
+    // Ensure that the DidChangeWatchedFilesNotification is handled before we continue.
+    _ = try await ws.testClient.send(BarrierRequest())
+
     // After updating Package.swift in PackageB, PackageB can provide proper build settings for MyExec/main.swift and
     // thus workspace membership should switch to PackageB.