Guard access to state by queue in Clang/SwiftLanguageServer

Author: ahoppen

Sources/SourceKit/clangd/ClangLanguageServer.swift

@@ -21,7 +21,7 @@ import TSCBasic
 /// A thin wrapper over a connection to a clangd server providing build setting handling.
 final class ClangLanguageServerShim: ToolchainLanguageServer {
 
-  /// The server's request queue, used to serialize requests and responses to `clangd`.
+  /// The server's request queue, used to protect shared access to mutable state and to serialize requests and responses to `clangd`.
   public let queue: DispatchQueue = DispatchQueue(label: "clangd-language-server-queue", qos: .userInitiated)
 
   /// The connection to `clangd`. `nil` before `initialize` has been called.
@@ -39,6 +39,10 @@ final class ClangLanguageServerShim: ToolchainLanguageServer {
 
   private var state: LanguageServerState {
      didSet {
+      if #available(OSX 10.12, *) {
+        // `state` must only be set from `queue`.
+        dispatchPrecondition(condition: .onQueue(queue))
+      }
        for handler in stateChangeHandlers {
          handler(oldValue, state)
        }
@@ -109,8 +113,10 @@ final class ClangLanguageServerShim: ToolchainLanguageServer {
       connection.close()
       if process.terminationStatus != 0 {
         if let self = self {
-          self.state = .connectionInterrupted
-          self.restartClangd()
+          self.queue.async {
+            self.state = .connectionInterrupted
+            self.restartClangd()
+          }
         }
       }
     }
@@ -153,15 +159,19 @@ final class ClangLanguageServerShim: ToolchainLanguageServer {
         _ = try self.initializeSync(initializeRequest)
         self.clientInitialized(InitializedNotification())
         self.reopenDocuments(self)
-        self.state = .connected
+        self.queue.async {
+          self.state = .connected
+        }
       } catch {
         log("Failed to restart clangd after a crash.", level: .error)
       }
     }
   }
 
   func addStateChangeHandler(handler: @escaping (LanguageServerState, LanguageServerState) -> Void) {
-    stateChangeHandlers.append(handler)
+    queue.async {
+      self.stateChangeHandlers.append(handler)
+    }
   }
 
 

Sources/SourceKit/sourcekitd/SwiftLanguageServer.swift

@@ -60,7 +60,7 @@ fileprivate func diagnosticsEnabled(for document: DocumentURI) -> Bool {
 
 public final class SwiftLanguageServer: ToolchainLanguageServer {
 
-  /// The server's request queue, used to serialize requests and responses to `sourcekitd`.
+  /// The server's request queue, used to protect shared access to mutable state and to serialize requests and responses to `sourcekitd`.
   public let queue: DispatchQueue = DispatchQueue(label: "swift-language-server-queue", qos: .userInitiated)
 
   let client: Connection
@@ -69,6 +69,10 @@ public final class SwiftLanguageServer: ToolchainLanguageServer {
 
   private var state: LanguageServerState {
     didSet {
+      if #available(OSX 10.12, *) {
+        // `state` must only be set from `queue`.
+        dispatchPrecondition(condition: .onQueue(queue))
+      }
       for handler in stateChangeHandlers {
         handler(oldValue, state)
       }
@@ -109,7 +113,9 @@ public final class SwiftLanguageServer: ToolchainLanguageServer {
   }
 
   public func addStateChangeHandler(handler: @escaping (_ oldState: LanguageServerState, _ newState: LanguageServerState) -> Void) {
-    stateChangeHandlers.append(handler)
+    queue.async {
+      self.stateChangeHandlers.append(handler)
+    }
   }
 
   /// Should be called on self.queue.
@@ -174,24 +180,28 @@ extension SwiftLanguageServer {
     api.set_notification_handler { [weak self] notification in
       guard let self = self else { return }
       let notification = SKResponse(notification, sourcekitd: self.sourcekitd)
-
-      if notification.value?[self.keys.notification] == self.values.notification_sema_enabled {
-        self.state = .connected
-      }
-
-      if self.state == .connectionInterrupted {
-        // If we get a notification while we are restoring the connection, it means that the server has restarted.
-        // We still need to wait for semantic functionality to come back up.
-        self.state = .semanticFunctionalityDisabled
-
-        // Ask our parent to re-open all of our documents.
-        self.reopenDocuments(self)
-      }
-
-      if let error = notification.error, error.code == .connectionInterrupted {
-        self.state = .connectionInterrupted
-
-        self.queue.async {
+      
+      // Check if we need to update our `state` based on the contents of the notification.
+      // Execute the entire code block on `queue` because we need to switch to `queue` anyway to
+      // check `state` in the second `if`. Moving `queue.async` up ensures we only need to switch
+      // queues once and makes the code inside easier to read.
+      self.queue.async {
+        if notification.value?[self.keys.notification] == self.values.notification_sema_enabled {
+          self.state = .connected
+        }
+        
+        if self.state == .connectionInterrupted {
+          // If we get a notification while we are restoring the connection, it means that the server has restarted.
+          // We still need to wait for semantic functionality to come back up.
+          self.state = .semanticFunctionalityDisabled
+          
+          // Ask our parent to re-open all of our documents.
+          self.reopenDocuments(self)
+        }
+        
+        if let error = notification.error, error.code == .connectionInterrupted {
+          self.state = .connectionInterrupted
+          
           // We don't have any open documents anymore after sourcekitd crashed.
           // Reset the document manager to reflect that.
           self.documentManager = DocumentManager()
@@ -259,7 +269,9 @@ extension SwiftLanguageServer {
 
   func exit(_ notification: Notification<ExitNotification>) {
     api.shutdown()
-    state = .shutDown
+    queue.async {
+      self.state = .shutDown
+    }
   }
 
   /// Tell sourcekitd to crash itself. For testing purposes only.