Revert "Merge pull request #2137 from ahoppen/terminate-unresponsive-clangd-sourcekitd"

This change is too risky for 6.2 at this point.

Author: bnbarham

Documentation/Configuration File.md

@@ -58,4 +58,3 @@ The structure of the file is currently not guaranteed to be stable. Options may
 - `swiftPublishDiagnosticsDebounceDuration: number`: The time that `SwiftLanguageService` should wait after an edit before starting to compute diagnostics and sending a `PublishDiagnosticsNotification`.
 - `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.

Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift

@@ -185,11 +185,11 @@ actor ExternalBuildSystemAdapter {
       protocol: bspRegistry,
       stderrLoggingCategory: "bsp-server-stderr",
       client: messagesToSourceKitLSPHandler,
-      terminationHandler: { [weak self] terminationReason in
+      terminationHandler: { [weak self] terminationStatus in
         guard let self else {
           return
         }
-        if terminationReason != .exited(exitCode: 0) {
+        if terminationStatus != 0 {
           Task {
             await orLog("Restarting BSP server") {
               try await self.handleBspServerCrash()

Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift

@@ -31,14 +31,6 @@ import struct CDispatch.dispatch_fd_t
 /// For example, inside a language server, the `JSONRPCConnection` takes the language service implementation as its
 // `receiveHandler` and itself provides the client connection for sending notifications and callbacks.
 public final class JSONRPCConnection: Connection {
-  public enum TerminationReason: Sendable, Equatable {
-    /// The process on the other end of the `JSONRPCConnection` terminated with the given exit code.
-    case exited(exitCode: Int32)
-
-    /// The process on the other end of the `JSONRPCConnection` terminated with a signal. The signal that it terminated
-    /// with is not known.
-    case uncaughtSignal
-  }
 
   /// A name of the endpoint for this connection, used for logging, e.g. `clangd`.
   private let name: String
@@ -206,7 +198,7 @@ public final class JSONRPCConnection: Connection {
     protocol messageRegistry: MessageRegistry,
     stderrLoggingCategory: String,
     client: MessageHandler,
-    terminationHandler: @Sendable @escaping (_ terminationReason: TerminationReason) -> Void
+    terminationHandler: @Sendable @escaping (_ terminationStatus: Int32) -> Void
   ) throws -> (connection: JSONRPCConnection, process: Process) {
     let clientToServer = Pipe()
     let serverToClient = Pipe()
@@ -246,22 +238,10 @@ public final class JSONRPCConnection: Connection {
     process.terminationHandler = { process in
       logger.log(
         level: process.terminationReason == .exit ? .default : .error,
-        "\(name) exited: \(process.terminationReason.rawValue) \(process.terminationStatus)"
+        "\(name) exited: \(String(reflecting: process.terminationReason)) \(process.terminationStatus)"
       )
       connection.close()
-      let terminationReason: TerminationReason
-      switch process.terminationReason {
-      case .exit:
-        terminationReason = .exited(exitCode: process.terminationStatus)
-      case .uncaughtSignal:
-        terminationReason = .uncaughtSignal
-      @unknown default:
-        logger.fault(
-          "Process terminated with unknown termination reason: \(process.terminationReason.rawValue, privacy: .public)"
-        )
-        terminationReason = .exited(exitCode: 0)
-      }
-      terminationHandler(terminationReason)
+      terminationHandler(process.terminationStatus)
     }
     try process.run()
 

Sources/SKOptions/SourceKitLSPOptions.swift

@@ -422,17 +422,6 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
     return .seconds(120)
   }
 
-  /// 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.
-  public var semanticServiceRestartTimeout: Double? = nil
-
-  public var semanticServiceRestartTimeoutOrDefault: Duration {
-    if let semanticServiceRestartTimeout {
-      return .seconds(semanticServiceRestartTimeout)
-    }
-    return .seconds(300)
-  }
-
   public init(
     swiftPM: SwiftPMOptions? = .init(),
     fallbackBuildSystem: FallbackBuildSystemOptions? = .init(),
@@ -450,8 +439,7 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
     experimentalFeatures: Set<ExperimentalFeature>? = nil,
     swiftPublishDiagnosticsDebounceDuration: Double? = nil,
     workDoneProgressDebounceDuration: Double? = nil,
-    sourcekitdRequestTimeout: Double? = nil,
-    semanticServiceRestartTimeout: Double? = nil
+    sourcekitdRequestTimeout: Double? = nil
   ) {
     self.swiftPM = swiftPM
     self.fallbackBuildSystem = fallbackBuildSystem
@@ -470,7 +458,6 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
     self.swiftPublishDiagnosticsDebounceDuration = swiftPublishDiagnosticsDebounceDuration
     self.workDoneProgressDebounceDuration = workDoneProgressDebounceDuration
     self.sourcekitdRequestTimeout = sourcekitdRequestTimeout
-    self.semanticServiceRestartTimeout = semanticServiceRestartTimeout
   }
 
   public init?(fromLSPAny lspAny: LSPAny?) throws {
@@ -530,8 +517,7 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable {
         ?? base.swiftPublishDiagnosticsDebounceDuration,
       workDoneProgressDebounceDuration: override?.workDoneProgressDebounceDuration
         ?? base.workDoneProgressDebounceDuration,
-      sourcekitdRequestTimeout: override?.sourcekitdRequestTimeout ?? base.sourcekitdRequestTimeout,
-      semanticServiceRestartTimeout: override?.semanticServiceRestartTimeout ?? base.semanticServiceRestartTimeout
+      sourcekitdRequestTimeout: override?.sourcekitdRequestTimeout ?? base.sourcekitdRequestTimeout
     )
   }
 

Sources/SKTestSupport/SkipUnless.swift

@@ -265,7 +265,8 @@ package actor SkipUnless {
               sourcekitd.keys.useNewAPI: 1
             ],
           ]),
-          timeout: defaultTimeoutDuration
+          timeout: defaultTimeoutDuration,
+          fileContents: nil
         )
         return response[sourcekitd.keys.useNewAPI] == 1
       } catch {

Sources/SKTestSupport/SourceKitD+send.swift

@@ -162,10 +162,7 @@ package actor SourceKitD {
   /// List of notification handlers that will be called for each notification.
   private var notificationHandlers: [WeakSKDNotificationHandler] = []
 
-  /// List of hooks that should be executed and that need to finish executing before a request is sent to sourcekitd.
-  private var preRequestHandlingHooks: [UUID: @Sendable (SKDRequestDictionary) async -> Void] = [:]
-
-  /// List of hooks that should be executed after a request sent to sourcekitd.
+  /// List of hooks that should be executed for every request sent to sourcekitd.
   private var requestHandlingHooks: [UUID: (SKDRequestDictionary) -> Void] = [:]
 
   package static func getOrCreate(
@@ -268,30 +265,6 @@ package actor SourceKitD {
     notificationHandlers.removeAll(where: { $0.value == nil || $0.value === handler })
   }
 
-  /// Execute `body` and invoke `hook` for every sourcekitd request that is sent during the execution time of `body`.
-  ///
-  /// Note that `hook` will not only be executed for requests sent *by* body but this may also include sourcekitd
-  /// requests that were sent by other clients of the same `DynamicallyLoadedSourceKitD` instance that just happen to
-  /// send a request during that time.
-  ///
-  /// This is intended for testing only.
-  package func withPreRequestHandlingHook(
-    body: () async throws -> Void,
-    hook: @escaping @Sendable (SKDRequestDictionary) async -> Void
-  ) async rethrows {
-    let id = UUID()
-    preRequestHandlingHooks[id] = hook
-    defer { preRequestHandlingHooks[id] = nil }
-    try await body()
-  }
-
-  func willSend(request: SKDRequestDictionary) async {
-    let request = request
-    for hook in preRequestHandlingHooks.values {
-      await hook(request)
-    }
-  }
-
   /// Execute `body` and invoke `hook` for every sourcekitd request that is sent during the execution time of `body`.
   ///
   /// Note that `hook` will not only be executed for requests sent *by* body but this may also include sourcekitd
@@ -324,56 +297,37 @@ package actor SourceKitD {
   package func send(
     _ request: SKDRequestDictionary,
     timeout: Duration,
-    restartTimeout: Duration,
     fileContents: String?
   ) async throws -> SKDResponseDictionary {
     let sourcekitdResponse = try await withTimeout(timeout) {
-      let restartTimeoutHandle = TimeoutHandle()
-      do {
-        return try await withTimeout(restartTimeout, handle: restartTimeoutHandle) {
-          await self.willSend(request: request)
-          return try await withCancellableCheckedThrowingContinuation { (continuation) -> SourceKitDRequestHandle? in
-            logger.info(
-              """
-              Sending sourcekitd request:
-              \(request.forLogging)
-              """
-            )
-            var handle: sourcekitd_api_request_handle_t? = nil
-            self.api.send_request(request.dict, &handle) { response in
-              continuation.resume(returning: SKDResponse(response!, sourcekitd: self))
-            }
-            Task {
-              await self.didSend(request: request)
-            }
-            if let handle {
-              return SourceKitDRequestHandle(handle: handle)
-            }
-            return nil
-          } cancel: { (handle: SourceKitDRequestHandle?) in
-            if let handle {
-              logger.info(
-                """
-                Cancelling sourcekitd request:
-                \(request.forLogging)
-                """
-              )
-              self.api.cancel_request(handle.handle)
-            }
-          }
+      return try await withCancellableCheckedThrowingContinuation { (continuation) -> SourceKitDRequestHandle? in
+        logger.info(
+          """
+          Sending sourcekitd request:
+          \(request.forLogging)
+          """
+        )
+        var handle: sourcekitd_api_request_handle_t? = nil
+        self.api.send_request(request.dict, &handle) { response in
+          continuation.resume(returning: SKDResponse(response!, sourcekitd: self))
         }
-      } catch let error as TimeoutError where error.handle == restartTimeoutHandle {
-        if !self.path.lastPathComponent.contains("InProc") {
-          logger.fault(
-            "Did not receive reply from sourcekitd after \(restartTimeout, privacy: .public). Terminating and restarting sourcekitd."
-          )
-          await self.crash()
-        } else {
-          logger.fault(
-            "Did not receive reply from sourcekitd after \(restartTimeout, privacy: .public). Not terminating sourcekitd because it is run in-process."
+        Task {
+          await self.didSend(request: request)
+        }
+        if let handle {
+          return SourceKitDRequestHandle(handle: handle)
+        }
+        return nil
+      } cancel: { (handle: SourceKitDRequestHandle?) in
+        if let handle {
+          logger.info(
+            """
+            Cancelling sourcekitd request:
+            \(request.forLogging)
+            """
           )
+          self.api.cancel_request(handle.handle)
         }
-        throw error
       }
     }
 
@@ -412,13 +366,6 @@ package actor SourceKitD {
 
     return dict
   }
-
-  package func crash() async {
-    let req = dictionary([
-      keys.request: requests.crashWithExit
-    ])
-    _ = try? await send(req, timeout: .seconds(60), restartTimeout: .seconds(24 * 60 * 60), fileContents: nil)
-  }
 }
 
 /// A sourcekitd notification handler in a class to allow it to be uniquely referenced.

Sources/SourceKitD/SourceKitD.swift

@@ -30,7 +30,7 @@ package actor SourceKitDRegistry<SourceKitDType: AnyObject> {
   private var active: [URL: (pluginPaths: PluginPaths?, sourcekitd: SourceKitDType)] = [:]
 
   /// Instances that have been unregistered, but may be resurrected if accessed before destruction.
-  private var cemetery: [URL: (pluginPaths: PluginPaths?, sourcekitd: WeakSourceKitD<SourceKitDType>)] = [:]
+  private var cemetary: [URL: (pluginPaths: PluginPaths?, sourcekitd: WeakSourceKitD<SourceKitDType>)] = [:]
 
   /// Initialize an empty registry.
   package init() {}
@@ -49,8 +49,8 @@ package actor SourceKitDRegistry<SourceKitDType: AnyObject> {
       }
       return existing.sourcekitd
     }
-    if let resurrected = cemetery[key], let resurrectedSourcekitD = resurrected.sourcekitd.value {
-      cemetery[key] = nil
+    if let resurrected = cemetary[key], let resurrectedSourcekitD = resurrected.sourcekitd.value {
+      cemetary[key] = nil
       if resurrected.pluginPaths != pluginPaths {
         logger.fault(
           "Already created SourceKitD with plugin paths \(resurrected.pluginPaths?.forLogging), now requesting incompatible plugin paths \(pluginPaths.forLogging)"
@@ -74,8 +74,8 @@ package actor SourceKitDRegistry<SourceKitDType: AnyObject> {
   package func remove(_ key: URL) -> SourceKitDType? {
     let existing = active.removeValue(forKey: key)
     if let existing = existing {
-      assert(self.cemetery[key]?.sourcekitd.value == nil)
-      cemetery[key] = (existing.pluginPaths, WeakSourceKitD(value: existing.sourcekitd))
+      assert(self.cemetary[key]?.sourcekitd.value == nil)
+      cemetary[key] = (existing.pluginPaths, WeakSourceKitD(value: existing.sourcekitd))
     }
     return existing?.sourcekitd
   }