Merge pull request #2277 from ahoppen/bsp-timeouts

Add a timeout for `workspace/buildTargets` and `buildTarget/sources` requests

Author: ahoppen

Documentation/Configuration File.md

@@ -59,3 +59,4 @@ The structure of the file is currently not guaranteed to be stable. Options may
 - `workDoneProgressDebounceDuration: number`: When a task is started that should be displayed to the client as a work done progress, how many milliseconds to wait before actually starting the work done progress. This prevents flickering of the work done progress in the client for short-lived index tasks which end within this duration.
 - `sourcekitdRequestTimeout: number`: The maximum duration that a sourcekitd request should be allowed to execute before being declared as timed out. In general, editors should cancel requests that they are no longer interested in, but in case editors don't cancel requests, this ensures that a long-running non-cancelled request is not blocking sourcekitd and thus most semantic functionality. In particular, VS Code does not cancel the semantic tokens request, which can cause a long-running AST build that blocks sourcekitd.
 - `semanticServiceRestartTimeout: number`: If a request to sourcekitd or clangd exceeds this timeout, we assume that the semantic service provider is hanging for some reason and won't recover. To restore semantic functionality, we terminate and restart it.
+- `buildServerWorkspaceRequestsTimeout: number`: Duration how long to wait for responses to `workspace/buildTargets` or `buildTarget/sources` request by the build server before defaulting to an empty response.

Sources/BuildServerIntegration/BuildServerManager.swift

@@ -482,8 +482,7 @@ package actor BuildServerManager: QueueBasedMessageHandler {
     self.filesDependenciesUpdatedDebouncer = Debouncer(
       debounceDuration: .milliseconds(500),
       combineResults: { $0.union($1) },
-      makeCall: {
-        [weak self] (filesWithUpdatedDependencies) in
+      makeCall: { [weak self] (filesWithUpdatedDependencies) in
         guard let self, let delegate = await self.delegate else {
           logger.fault("Not calling filesDependenciesUpdated because no delegate exists in SwiftPMBuildServer")
           return
@@ -502,8 +501,7 @@ package actor BuildServerManager: QueueBasedMessageHandler {
     self.filesBuildSettingsChangedDebouncer = Debouncer(
       debounceDuration: .milliseconds(20),
       combineResults: { $0.union($1) },
-      makeCall: {
-        [weak self] (filesWithChangedBuildSettings) in
+      makeCall: { [weak self] (filesWithChangedBuildSettings) in
         guard let self, let delegate = await self.delegate else {
           logger.fault("Not calling fileBuildSettingsChanged because no delegate exists in SwiftPMBuildServer")
           return
@@ -670,31 +668,76 @@ package actor BuildServerManager: QueueBasedMessageHandler {
   }
 
   private func didChangeBuildTarget(notification: OnBuildTargetDidChangeNotification) async {
-    let updatedTargets: Set<BuildTargetIdentifier>? =
+    let changedTargets: Set<BuildTargetIdentifier>? =
       if let changes = notification.changes {
         Set(changes.map(\.target))
       } else {
         nil
       }
-    self.cachedAdjustedSourceKitOptions.clear(isolation: self) { cacheKey in
-      guard let updatedTargets else {
-        // All targets might have changed
-        return true
+    await self.buildTargetsDidChange(.didChangeBuildTargets(changedTargets: changedTargets))
+  }
+
+  private enum BuildTargetsChange {
+    case didChangeBuildTargets(changedTargets: Set<BuildTargetIdentifier>?)
+    case buildTargetsReceivedResultAfterTimeout(
+      request: WorkspaceBuildTargetsRequest,
+      newResult: [BuildTargetIdentifier: BuildTargetInfo]
+    )
+    case sourceFilesReceivedResultAfterTimeout(
+      request: BuildTargetSourcesRequest,
+      newResult: BuildTargetSourcesResponse
+    )
+  }
+
+  /// Update the cached state in `BuildServerManager` because new data was received from the BSP server.
+  ///
+  /// This handles a few seemingly unrelated reasons to ensure that we think about which caches to invalidate in the
+  /// other scenarios as well, when making changes in here.
+  private func buildTargetsDidChange(_ stateChange: BuildTargetsChange) async {
+    let changedTargets: Set<BuildTargetIdentifier>?
+
+    switch stateChange {
+    case .didChangeBuildTargets(let changedTargetsValue):
+      changedTargets = changedTargetsValue
+      self.cachedAdjustedSourceKitOptions.clear(isolation: self) { cacheKey in
+        guard let changedTargets else {
+          // All targets might have changed
+          return true
+        }
+        return changedTargets.contains(cacheKey.target)
       }
-      return updatedTargets.contains(cacheKey.target)
-    }
-    self.cachedBuildTargets.clearAll(isolation: self)
-    self.cachedTargetSources.clear(isolation: self) { cacheKey in
-      guard let updatedTargets else {
-        // All targets might have changed
-        return true
+      self.cachedBuildTargets.clearAll(isolation: self)
+      self.cachedTargetSources.clear(isolation: self) { cacheKey in
+        guard let changedTargets else {
+          // All targets might have changed
+          return true
+        }
+        return !changedTargets.intersection(cacheKey.targets).isEmpty
       }
-      return !updatedTargets.intersection(cacheKey.targets).isEmpty
-    }
+    case .buildTargetsReceivedResultAfterTimeout(let request, let newResult):
+      changedTargets = nil
+
+      // Caches not invalidated:
+      // - cachedAdjustedSourceKitOptions: We would not have requested SourceKit options for targets that we didn't
+      //   know about. Even if we did, the build server now telling us about the target should not change the options of
+      //   the file within the target
+      // - cachedTargetSources: Similar to cachedAdjustedSourceKitOptions, we would not have requested sources for
+      //   targets that we didn't know about and if we did, they wouldn't be affected
+      self.cachedBuildTargets.set(request, to: newResult)
+    case .sourceFilesReceivedResultAfterTimeout(let request, let newResult):
+      changedTargets = Set(request.targets)
+
+      // Caches not invalidated:
+      // - cachedAdjustedSourceKitOptions: Same as for buildTargetsReceivedResultAfterTimeout.
+      // - cachedBuildTargets: Getting a result for the source files in a target doesn't change anything about the
+      //   target's existence.
+      self.cachedTargetSources.set(request, to: newResult)
+    }
+    // Clear caches that capture global state and are affected by all changes
     self.cachedSourceFilesAndDirectories.clearAll(isolation: self)
     self.scheduleRecomputeCopyFileMap()
 
-    await delegate?.buildTargetsChanged(notification.changes)
+    await delegate?.buildTargetsChanged(changedTargets)
     await filesBuildSettingsChangedDebouncer.scheduleCall(Set(watchedFiles.keys))
   }
 
@@ -898,6 +941,7 @@ package actor BuildServerManager: QueueBasedMessageHandler {
       previousUpdateTask?.cancel()
       await orLog("Re-computing copy file map") {
         let sourceFilesAndDirectories = try await self.sourceFilesAndDirectories()
+        try Task.checkCancellation()
         var copiedFileMap: [DocumentURI: DocumentURI] = [:]
         for (file, fileInfo) in sourceFilesAndDirectories.files {
           for copyDestination in fileInfo.copyDestinations {
@@ -1024,7 +1068,7 @@ package actor BuildServerManager: QueueBasedMessageHandler {
         if fallbackAfterTimeout {
           try await withTimeout(options.buildSettingsTimeoutOrDefault) {
             return try await self.buildSettingsFromBuildServer(for: document, in: target, language: language)
-          } resultReceivedAfterTimeout: {
+          } resultReceivedAfterTimeout: { _ in
             await self.filesBuildSettingsChangedDebouncer.scheduleCall([document])
           }
         } else {
@@ -1082,7 +1126,7 @@ package actor BuildServerManager: QueueBasedMessageHandler {
           } else {
             try await withTimeout(options.buildSettingsTimeoutOrDefault) {
               await self.canonicalTarget(for: mainFile)
-            } resultReceivedAfterTimeout: {
+            } resultReceivedAfterTimeout: { _ in
               await self.filesBuildSettingsChangedDebouncer.scheduleCall([document])
             }
           }
@@ -1236,28 +1280,39 @@ package actor BuildServerManager: QueueBasedMessageHandler {
 
     let request = WorkspaceBuildTargetsRequest()
     let result = try await cachedBuildTargets.get(request, isolation: self) { request in
-      let buildTargets = try await buildServerAdapter.send(request).targets
-      let (depths, dependents) = await self.targetDepthsAndDependents(for: buildTargets)
-      var result: [BuildTargetIdentifier: BuildTargetInfo] = [:]
-      result.reserveCapacity(buildTargets.count)
-      for buildTarget in buildTargets {
-        guard result[buildTarget.id] == nil else {
-          logger.error("Found two targets with the same ID \(buildTarget.id)")
-          continue
-        }
-        let depth: Int
-        if let d = depths[buildTarget.id] {
-          depth = d
-        } else {
-          logger.fault("Did not compute depth for target \(buildTarget.id)")
-          depth = 0
+      let result = try await withTimeout(self.options.buildServerWorkspaceRequestsTimeoutOrDefault) {
+        let buildTargets = try await buildServerAdapter.send(request).targets
+        let (depths, dependents) = await self.targetDepthsAndDependents(for: buildTargets)
+        var result: [BuildTargetIdentifier: BuildTargetInfo] = [:]
+        result.reserveCapacity(buildTargets.count)
+        for buildTarget in buildTargets {
+          guard result[buildTarget.id] == nil else {
+            logger.error("Found two targets with the same ID \(buildTarget.id)")
+            continue
+          }
+          let depth: Int
+          if let d = depths[buildTarget.id] {
+            depth = d
+          } else {
+            logger.fault("Did not compute depth for target \(buildTarget.id)")
+            depth = 0
+          }
+          result[buildTarget.id] = BuildTargetInfo(
+            target: buildTarget,
+            depth: depth,
+            dependents: dependents[buildTarget.id] ?? []
+          )
         }
-        result[buildTarget.id] = BuildTargetInfo(
-          target: buildTarget,
-          depth: depth,
-          dependents: dependents[buildTarget.id] ?? []
+        return result
+      } resultReceivedAfterTimeout: { newResult in
+        await self.buildTargetsDidChange(
+          .buildTargetsReceivedResultAfterTimeout(request: request, newResult: newResult)
         )
       }
+      guard let result else {
+        logger.error("Failed to get targets of workspace within timeout")
+        return [:]
+      }
       return result
     }
     return result
@@ -1290,7 +1345,11 @@ package actor BuildServerManager: QueueBasedMessageHandler {
     }
 
     let response = try await cachedTargetSources.get(request, isolation: self) { request in
-      try await buildServerAdapter.send(request)
+      try await withTimeout(self.options.buildServerWorkspaceRequestsTimeoutOrDefault) {
+        return try await buildServerAdapter.send(request)
+      } resultReceivedAfterTimeout: { newResult in
+        await self.buildTargetsDidChange(.sourceFilesReceivedResultAfterTimeout(request: request, newResult: newResult))
+      } ?? BuildTargetSourcesResponse(items: [])
     }
     return response.items
   }

Sources/BuildServerIntegration/BuildServerManagerDelegate.swift

@@ -29,7 +29,7 @@ package protocol BuildServerManagerDelegate: AnyObject, Sendable {
 
   /// Notify the delegate that some information about the given build targets has changed and that it should recompute
   /// any information based on top of it.
-  func buildTargetsChanged(_ changes: [BuildTargetEvent]?) async
+  func buildTargetsChanged(_ changedTargets: Set<BuildTargetIdentifier>?) async
 }
 
 /// Methods with which the `BuildServerManager` can send messages to the client (aka. editor).

Sources/SKOptions/SourceKitLSPOptions.swift

@@ -433,6 +433,22 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
     return .seconds(300)
   }
 
+  /// Duration how long to wait for responses to `workspace/buildTargets` or `buildTarget/sources` request by the build
+  /// server before defaulting to an empty response.
+  public var buildServerWorkspaceRequestsTimeout: Double? = nil
+
+  public var buildServerWorkspaceRequestsTimeoutOrDefault: Duration {
+    if let buildServerWorkspaceRequestsTimeout {
+      return .seconds(buildServerWorkspaceRequestsTimeout)
+    }
+    // The default value needs to strike a balance: If the build server is slow to respond, we don't want to constantly
+    // run into this timeout, which causes somewhat expensive computations because we trigger the `buildTargetsChanged`
+    // chain.
+    // At the same time, we do want to provide functionality based on fallback settings after some time.
+    // 15s seems like it should strike a balance here but there is no data backing this value up.
+    return .seconds(15)
+  }
+
   public init(
     swiftPM: SwiftPMOptions? = .init(),
     fallbackBuildSystem: FallbackBuildSystemOptions? = .init(),
@@ -451,7 +467,8 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
     swiftPublishDiagnosticsDebounceDuration: Double? = nil,
     workDoneProgressDebounceDuration: Double? = nil,
     sourcekitdRequestTimeout: Double? = nil,
-    semanticServiceRestartTimeout: Double? = nil
+    semanticServiceRestartTimeout: Double? = nil,
+    buildServerWorkspaceRequestsTimeout: Double? = nil
   ) {
     self.swiftPM = swiftPM
     self.fallbackBuildSystem = fallbackBuildSystem
@@ -471,6 +488,7 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
     self.workDoneProgressDebounceDuration = workDoneProgressDebounceDuration
     self.sourcekitdRequestTimeout = sourcekitdRequestTimeout
     self.semanticServiceRestartTimeout = semanticServiceRestartTimeout
+    self.buildServerWorkspaceRequestsTimeout = buildServerWorkspaceRequestsTimeout
   }
 
   public init?(fromLSPAny lspAny: LSPAny?) throws {
@@ -535,7 +553,9 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
       workDoneProgressDebounceDuration: override?.workDoneProgressDebounceDuration
         ?? base.workDoneProgressDebounceDuration,
       sourcekitdRequestTimeout: override?.sourcekitdRequestTimeout ?? base.sourcekitdRequestTimeout,
-      semanticServiceRestartTimeout: override?.semanticServiceRestartTimeout ?? base.semanticServiceRestartTimeout
+      semanticServiceRestartTimeout: override?.semanticServiceRestartTimeout ?? base.semanticServiceRestartTimeout,
+      buildServerWorkspaceRequestsTimeout: override?.buildServerWorkspaceRequestsTimeout
+        ?? base.buildServerWorkspaceRequestsTimeout
     )
   }
 

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -426,14 +426,7 @@ package final actor SemanticIndexManager {
     }
   }
 
-  package func buildTargetsChanged(_ changes: [BuildTargetEvent]?) async {
-    let targets: Set<BuildTargetIdentifier>? =
-      if let changes = changes?.map(\.target) {
-        Set(changes)
-      } else {
-        nil
-      }
-
+  package func buildTargetsChanged(_ targets: Set<BuildTargetIdentifier>?) async {
     if let targets {
       var targetsAndDependencies = targets
       targetsAndDependencies.formUnion(await buildServerManager.targets(dependingOn: targets))

Sources/SourceKitLSP/Workspace.swift

@@ -452,9 +452,9 @@ package final class Workspace: Sendable, BuildServerManagerDelegate {
     }
   }
 
-  package func buildTargetsChanged(_ changes: [BuildTargetEvent]?) async {
+  package func buildTargetsChanged(_ changedTargets: Set<BuildTargetIdentifier>?) async {
     await sourceKitLSPServer?.fileHandlingCapabilityChanged()
-    await semanticIndexManager?.buildTargetsChanged(changes)
+    await semanticIndexManager?.buildTargetsChanged(changedTargets)
     await orLog("Scheduling syntactic test re-indexing") {
       let testFiles = try await buildServerManager.testFiles()
       await syntacticTestIndex.listOfTestFilesDidChange(testFiles)

Sources/SwiftExtensions/AsyncUtils.swift

@@ -270,8 +270,8 @@ package func withTimeout<T: Sendable>(
 ///   `body` should be cancelled after `timeout`.
 package func withTimeout<T: Sendable>(
   _ timeout: Duration,
-  body: @escaping @Sendable () async throws -> T?,
-  resultReceivedAfterTimeout: @escaping @Sendable () async -> Void
+  body: @escaping @Sendable () async throws -> T,
+  resultReceivedAfterTimeout: @escaping @Sendable (_ result: T) async -> Void
 ) async throws -> T? {
   let didHitTimeout = AtomicBool(initialValue: false)
 
@@ -286,7 +286,7 @@ package func withTimeout<T: Sendable>(
       do {
         let result = try await body()
         if didHitTimeout.value {
-          await resultReceivedAfterTimeout()
+          await resultReceivedAfterTimeout(result)
         }
         continuation.yield(result)
       } catch {
@@ -306,3 +306,17 @@ package func withTimeout<T: Sendable>(
     preconditionFailure("Continuation never finishes")
   }
 }
+
+/// Same as `withTimeout` above but allows `body` to return an optional value.
+package func withTimeout<T: Sendable>(
+  _ timeout: Duration,
+  body: @escaping @Sendable () async throws -> T?,
+  resultReceivedAfterTimeout: @escaping @Sendable (_ result: T?) async -> Void
+) async throws -> T? {
+  let result: T?? = try await withTimeout(timeout, body: body, resultReceivedAfterTimeout: resultReceivedAfterTimeout)
+  switch result {
+  case .none: return nil
+  case .some(.none): return nil
+  case .some(.some(let value)): return value
+  }
+}

Sources/SwiftExtensions/Cache.swift

@@ -33,6 +33,15 @@ package class Cache<Key: Sendable & Hashable, Result: Sendable> {
     return try await task.value
   }
 
+  /// Force the value for a specific key to a value.
+  ///
+  /// This should only be used if a value for this key is received by means that aren't covered through the `compute`
+  /// function in `get`. An example of this is receiving the results of a BSP request after a timeout, in which case we
+  /// would have cached the timeout result through `get` but now we have an updated value.
+  package func set(_ key: Key, to value: Result) {
+    storage[key] = Task { value }
+  }
+
   /// Get the value cached for `key`. If no value exists for `key`, try deriving the result from an existing cache entry
   /// that satisfies `canReuseKey` by applying `transform` to that result.
   package func getDerived(

Tests/BuildServerIntegrationTests/BuildSystemManagerTests.swift

@@ -428,7 +428,7 @@ private actor BSMDelegate: BuildServerManagerDelegate {
 
   func filesDependenciesUpdated(_ changedFiles: Set<DocumentURI>) {}
 
-  func buildTargetsChanged(_ changes: [BuildTargetEvent]?) async {}
+  func buildTargetsChanged(_ changedTargets: Set<BuildTargetIdentifier>?) async {}
 
   var clientSupportsWorkDoneProgress: Bool { false }