From 17327e611c29ef18677ba594fa1b737401bb21c6 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Thu, 27 Mar 2025 12:57:03 -0700 Subject: [PATCH 1/7] Ensure the `DLHandle` passed to `SourceKitD.init(dlhandle:path:pluginPaths:initialize:)` gets closed if the initializer fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Just something I noticed while looking through code. I am not aware of any issues caused by this. Also relax the `precondition` in `DLHandle.deinit` to a fault. If we do not close a `DLHandle`, that’s a bug but it’s not so bad that we should crash sourcekit-lsp for it. --- Sources/SourceKitD/SourceKitD.swift | 80 +++++++++++++++-------------- Sources/SourceKitD/dlopen.swift | 5 +- 2 files changed, 46 insertions(+), 39 deletions(-) diff --git a/Sources/SourceKitD/SourceKitD.swift b/Sources/SourceKitD/SourceKitD.swift index 8ad0ed857..4e634e019 100644 --- a/Sources/SourceKitD/SourceKitD.swift +++ b/Sources/SourceKitD/SourceKitD.swift @@ -193,52 +193,56 @@ package actor SourceKitD { let dlopenModes: DLOpenFlags = [.lazy, .local, .first] #endif let dlhandle = try dlopen(path.filePath, mode: dlopenModes) - do { - try self.init( - dlhandle: dlhandle, - path: path, - pluginPaths: pluginPaths, - initialize: initialize - ) - } catch { - try? dlhandle.close() - throw error - } + try self.init( + dlhandle: dlhandle, + path: path, + pluginPaths: pluginPaths, + initialize: initialize + ) } + /// Create a `SourceKitD` instance from an existing `DLHandle`. `SourceKitD` takes over ownership of the `DLHandler` + /// and will close it when the `SourceKitD` instance gets deinitialized or if the initializer throws. package init(dlhandle: DLHandle, path: URL, pluginPaths: PluginPaths?, initialize: Bool) throws { - self.path = path - self.dylib = dlhandle - let api = try sourcekitd_api_functions_t(dlhandle) - self.api = api - - // We load the plugin-related functions eagerly so the members are initialized and we don't have data races on first - // access to eg. `pluginApi`. But if one of the functions is missing, we will only emit that error when that family - // of functions is being used. For example, it is expected that the plugin functions are not available in - // SourceKit-LSP. - self.ideApiResult = Result(catching: { try sourcekitd_ide_api_functions_t(dlhandle) }) - self.pluginApiResult = Result(catching: { try sourcekitd_plugin_api_functions_t(dlhandle) }) - self.servicePluginApiResult = Result(catching: { try sourcekitd_service_plugin_api_functions_t(dlhandle) }) - - if let pluginPaths { - api.register_plugin_path?(pluginPaths.clientPlugin.path, pluginPaths.servicePlugin.path) - } - if initialize { - self.api.initialize() - } + do { + self.path = path + self.dylib = dlhandle + let api = try sourcekitd_api_functions_t(dlhandle) + self.api = api + + // We load the plugin-related functions eagerly so the members are initialized and we don't have data races on first + // access to eg. `pluginApi`. But if one of the functions is missing, we will only emit that error when that family + // of functions is being used. For example, it is expected that the plugin functions are not available in + // SourceKit-LSP. + self.ideApiResult = Result(catching: { try sourcekitd_ide_api_functions_t(dlhandle) }) + self.pluginApiResult = Result(catching: { try sourcekitd_plugin_api_functions_t(dlhandle) }) + self.servicePluginApiResult = Result(catching: { try sourcekitd_service_plugin_api_functions_t(dlhandle) }) - if initialize { - self.api.set_notification_handler { [weak self] rawResponse in - guard let self, let rawResponse else { return } - let response = SKDResponse(rawResponse, sourcekitd: self) - self.notificationHandlingQueue.async { - let handlers = await self.notificationHandlers.compactMap(\.value) + if let pluginPaths { + api.register_plugin_path?(pluginPaths.clientPlugin.path, pluginPaths.servicePlugin.path) + } + if initialize { + self.api.initialize() + } + + if initialize { + self.api.set_notification_handler { [weak self] rawResponse in + guard let self, let rawResponse else { return } + let response = SKDResponse(rawResponse, sourcekitd: self) + self.notificationHandlingQueue.async { + let handlers = await self.notificationHandlers.compactMap(\.value) - for handler in handlers { - handler.notification(response) + for handler in handlers { + handler.notification(response) + } } } } + } catch { + orLog("Closing dlhandle after opening sourcekitd failed") { + try? dlhandle.close() + } + throw error } } diff --git a/Sources/SourceKitD/dlopen.swift b/Sources/SourceKitD/dlopen.swift index fa4ed6cc8..dd34e4f97 100644 --- a/Sources/SourceKitD/dlopen.swift +++ b/Sources/SourceKitD/dlopen.swift @@ -10,6 +10,7 @@ // //===----------------------------------------------------------------------===// +import SKLogging import SwiftExtensions #if os(Windows) @@ -45,7 +46,9 @@ package final class DLHandle: Sendable { } deinit { - precondition(rawValue.value == nil, "DLHandle must be closed or explicitly leaked before destroying") + if rawValue.value != nil { + logger.fault("DLHandle must be closed or explicitly leaked before destroying") + } } /// The handle must not be used anymore after calling `close`. From e840daba20ba693e07202833413d42db5804d52a Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Tue, 6 May 2025 22:56:58 -0400 Subject: [PATCH 2/7] Add SwiftASN1 as dep in CMakeLists file SwiftPM has added Crypto related functionality to Workspace to support signing of the swift-syntax prebuilts manifest signing. This breaks sourcekit-lsp builds on Windows complaining about SwiftASN1 being missing. Adding a clause here to match the one in SwiftPM's CMakeLists.txt file. --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index b53e270bf..fa14a484d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -25,6 +25,7 @@ find_package(ArgumentParser CONFIG REQUIRED) find_package(SwiftCollections QUIET) find_package(SwiftSyntax CONFIG REQUIRED) find_package(SwiftCrypto CONFIG REQUIRED) +find_package(SwiftASN1 CONFIG REQUIRED) include(SwiftSupport) From 14cfd50582874f28ad08e16dcca62377c920e10d Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Tue, 6 May 2025 20:18:47 +0200 Subject: [PATCH 3/7] =?UTF-8?q?If=20sourcekitd=20or=20clangd=20don?= =?UTF-8?q?=E2=80=99t=20respond=20to=20a=20request=20for=205=20minutes,=20?= =?UTF-8?q?terminate=20them=20and=20use=20crash=20recovery=20to=20restore?= =?UTF-8?q?=20behavior?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This should be a last stop-gap measure in case sourcekitd or clangd get stuck, don’t respond to any requests anymore and don’t honor cancellation either. In that case we can restore SourceKit-LSP behavior by killing them and using the crash recovery logic to restore functionality. rdar://149492159 --- Documentation/Configuration File.md | 1 + .../ExternalBuildSystemAdapter.swift | 4 +- .../JSONRPCConnection.swift | 26 ++++- Sources/SKOptions/SourceKitLSPOptions.swift | 18 ++- Sources/SKTestSupport/SkipUnless.swift | 3 +- Sources/SKTestSupport/SourceKitD+send.swift | 29 +++++ Sources/SourceKitD/SourceKitD.swift | 105 +++++++++++++----- Sources/SourceKitD/SourceKitDRegistry.swift | 10 +- .../Clang/ClangLanguageService.swift | 35 +++--- Sources/SourceKitLSP/Hooks.swift | 9 +- .../Swift/CodeCompletionSession.swift | 38 ++++--- .../Swift/DiagnosticReportManager.swift | 1 + .../Swift/SwiftLanguageService.swift | 3 +- Sources/SwiftExtensions/AsyncUtils.swift | 22 +++- Tests/SourceKitDTests/SourceKitDTests.swift | 4 +- Tests/SourceKitLSPTests/ClangdTests.swift | 59 ++++++++++ .../CrashRecoveryTests.swift | 67 +++++++++++ .../SwiftSourceKitPluginTests.swift | 22 ++-- config.schema.json | 5 + 19 files changed, 377 insertions(+), 84 deletions(-) create mode 100644 Sources/SKTestSupport/SourceKitD+send.swift rename Tests/{SourceKitDTests => SourceKitLSPTests}/CrashRecoveryTests.swift (81%) diff --git a/Documentation/Configuration File.md b/Documentation/Configuration File.md index fedc0b746..055bb4c26 100644 --- a/Documentation/Configuration File.md +++ b/Documentation/Configuration File.md @@ -58,3 +58,4 @@ 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. diff --git a/Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift b/Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift index 2fc20e1ea..372a4d211 100644 --- a/Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift +++ b/Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift @@ -185,11 +185,11 @@ actor ExternalBuildSystemAdapter { protocol: bspRegistry, stderrLoggingCategory: "bsp-server-stderr", client: messagesToSourceKitLSPHandler, - terminationHandler: { [weak self] terminationStatus in + terminationHandler: { [weak self] terminationReason in guard let self else { return } - if terminationStatus != 0 { + if terminationReason != .exited(exitCode: 0) { Task { await orLog("Restarting BSP server") { try await self.handleBspServerCrash() diff --git a/Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift b/Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift index e6ba2ca39..0c1853511 100644 --- a/Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift +++ b/Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift @@ -31,6 +31,14 @@ 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 @@ -198,7 +206,7 @@ public final class JSONRPCConnection: Connection { protocol messageRegistry: MessageRegistry, stderrLoggingCategory: String, client: MessageHandler, - terminationHandler: @Sendable @escaping (_ terminationStatus: Int32) -> Void + terminationHandler: @Sendable @escaping (_ terminationReason: TerminationReason) -> Void ) throws -> (connection: JSONRPCConnection, process: Process) { let clientToServer = Pipe() let serverToClient = Pipe() @@ -238,10 +246,22 @@ public final class JSONRPCConnection: Connection { process.terminationHandler = { process in logger.log( level: process.terminationReason == .exit ? .default : .error, - "\(name) exited: \(String(reflecting: process.terminationReason)) \(process.terminationStatus)" + "\(name) exited: \(process.terminationReason.rawValue) \(process.terminationStatus)" ) connection.close() - terminationHandler(process.terminationStatus) + 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) } try process.run() diff --git a/Sources/SKOptions/SourceKitLSPOptions.swift b/Sources/SKOptions/SourceKitLSPOptions.swift index 208017ef5..0adccbb17 100644 --- a/Sources/SKOptions/SourceKitLSPOptions.swift +++ b/Sources/SKOptions/SourceKitLSPOptions.swift @@ -422,6 +422,17 @@ 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(), @@ -439,7 +450,8 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable { experimentalFeatures: Set? = nil, swiftPublishDiagnosticsDebounceDuration: Double? = nil, workDoneProgressDebounceDuration: Double? = nil, - sourcekitdRequestTimeout: Double? = nil + sourcekitdRequestTimeout: Double? = nil, + semanticServiceRestartTimeout: Double? = nil ) { self.swiftPM = swiftPM self.fallbackBuildSystem = fallbackBuildSystem @@ -458,6 +470,7 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable { self.swiftPublishDiagnosticsDebounceDuration = swiftPublishDiagnosticsDebounceDuration self.workDoneProgressDebounceDuration = workDoneProgressDebounceDuration self.sourcekitdRequestTimeout = sourcekitdRequestTimeout + self.semanticServiceRestartTimeout = semanticServiceRestartTimeout } public init?(fromLSPAny lspAny: LSPAny?) throws { @@ -517,7 +530,8 @@ public struct SourceKitLSPOptions: Sendable, Codable, Equatable { ?? base.swiftPublishDiagnosticsDebounceDuration, workDoneProgressDebounceDuration: override?.workDoneProgressDebounceDuration ?? base.workDoneProgressDebounceDuration, - sourcekitdRequestTimeout: override?.sourcekitdRequestTimeout ?? base.sourcekitdRequestTimeout + sourcekitdRequestTimeout: override?.sourcekitdRequestTimeout ?? base.sourcekitdRequestTimeout, + semanticServiceRestartTimeout: override?.semanticServiceRestartTimeout ?? base.semanticServiceRestartTimeout ) } diff --git a/Sources/SKTestSupport/SkipUnless.swift b/Sources/SKTestSupport/SkipUnless.swift index 4c25b324b..1c8830e23 100644 --- a/Sources/SKTestSupport/SkipUnless.swift +++ b/Sources/SKTestSupport/SkipUnless.swift @@ -265,8 +265,7 @@ package actor SkipUnless { sourcekitd.keys.useNewAPI: 1 ], ]), - timeout: defaultTimeoutDuration, - fileContents: nil + timeout: defaultTimeoutDuration ) return response[sourcekitd.keys.useNewAPI] == 1 } catch { diff --git a/Sources/SKTestSupport/SourceKitD+send.swift b/Sources/SKTestSupport/SourceKitD+send.swift new file mode 100644 index 000000000..c27287ab9 --- /dev/null +++ b/Sources/SKTestSupport/SourceKitD+send.swift @@ -0,0 +1,29 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2025 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 +// +//===----------------------------------------------------------------------===// + +package import SourceKitD + +extension SourceKitD { + /// Convenience overload of the `send` function for testing that doesn't restart sourcekitd if it does not respond + /// and doesn't pass any file contents. + package func send( + _ request: SKDRequestDictionary, + timeout: Duration + ) async throws -> SKDResponseDictionary { + return try await self.send( + request, + timeout: timeout, + restartTimeout: .seconds(60 * 60 * 24), + fileContents: nil + ) + } +} diff --git a/Sources/SourceKitD/SourceKitD.swift b/Sources/SourceKitD/SourceKitD.swift index 8ad0ed857..b757e70ae 100644 --- a/Sources/SourceKitD/SourceKitD.swift +++ b/Sources/SourceKitD/SourceKitD.swift @@ -162,7 +162,10 @@ 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 for every request sent to sourcekitd. + /// 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. private var requestHandlingHooks: [UUID: (SKDRequestDictionary) -> Void] = [:] package static func getOrCreate( @@ -261,6 +264,30 @@ 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 @@ -293,37 +320,56 @@ package actor SourceKitD { package func send( _ request: SKDRequestDictionary, timeout: Duration, + restartTimeout: Duration, fileContents: String? ) async throws -> SKDResponseDictionary { let sourcekitdResponse = try await withTimeout(timeout) { - 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) + 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 nil - } cancel: { (handle: SourceKitDRequestHandle?) in - if let handle { - logger.info( - """ - Cancelling sourcekitd request: - \(request.forLogging) - """ + } 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." ) - self.api.cancel_request(handle.handle) } + throw error } } @@ -362,6 +408,13 @@ 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. diff --git a/Sources/SourceKitD/SourceKitDRegistry.swift b/Sources/SourceKitD/SourceKitDRegistry.swift index b09e399a1..420fdc2f8 100644 --- a/Sources/SourceKitD/SourceKitDRegistry.swift +++ b/Sources/SourceKitD/SourceKitDRegistry.swift @@ -30,7 +30,7 @@ package actor SourceKitDRegistry { private var active: [URL: (pluginPaths: PluginPaths?, sourcekitd: SourceKitDType)] = [:] /// Instances that have been unregistered, but may be resurrected if accessed before destruction. - private var cemetary: [URL: (pluginPaths: PluginPaths?, sourcekitd: WeakSourceKitD)] = [:] + private var cemetery: [URL: (pluginPaths: PluginPaths?, sourcekitd: WeakSourceKitD)] = [:] /// Initialize an empty registry. package init() {} @@ -49,8 +49,8 @@ package actor SourceKitDRegistry { } return existing.sourcekitd } - if let resurrected = cemetary[key], let resurrectedSourcekitD = resurrected.sourcekitd.value { - cemetary[key] = nil + if let resurrected = cemetery[key], let resurrectedSourcekitD = resurrected.sourcekitd.value { + cemetery[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 { package func remove(_ key: URL) -> SourceKitDType? { let existing = active.removeValue(forKey: key) if let existing = existing { - assert(self.cemetary[key]?.sourcekitd.value == nil) - cemetary[key] = (existing.pluginPaths, WeakSourceKitD(value: existing.sourcekitd)) + assert(self.cemetery[key]?.sourcekitd.value == nil) + cemetery[key] = (existing.pluginPaths, WeakSourceKitD(value: existing.sourcekitd)) } return existing?.sourcekitd } diff --git a/Sources/SourceKitLSP/Clang/ClangLanguageService.swift b/Sources/SourceKitLSP/Clang/ClangLanguageService.swift index 47ebb09fd..5fa94b365 100644 --- a/Sources/SourceKitLSP/Clang/ClangLanguageService.swift +++ b/Sources/SourceKitLSP/Clang/ClangLanguageService.swift @@ -62,7 +62,7 @@ actor ClangLanguageService: LanguageService, MessageHandler { /// Path to the `clangd` binary. let clangdPath: URL - let clangdOptions: [String] + let options: SourceKitLSPOptions /// The current state of the `clangd` language server. /// Changing the property automatically notified the state change handlers. @@ -117,7 +117,7 @@ actor ClangLanguageService: LanguageService, MessageHandler { } self.clangPath = toolchain.clang self.clangdPath = clangdPath - self.clangdOptions = options.clangdOptions ?? [] + self.options = options self.workspace = WeakWorkspace(workspace) self.state = .connected self.sourceKitLSPServer = sourceKitLSPServer @@ -154,10 +154,11 @@ actor ClangLanguageService: LanguageService, MessageHandler { /// Restarts `clangd` if it has crashed. /// /// - Parameter terminationStatus: The exit code of `clangd`. - private func handleClangdTermination(terminationStatus: Int32) { + private func handleClangdTermination(terminationReason: JSONRPCConnection.TerminationReason) { self.clangdProcess = nil - if terminationStatus != 0 { + if terminationReason != .exited(exitCode: 0) { self.state = .connectionInterrupted + logger.info("clangd crashed. Restarting it.") self.restartClangd() } } @@ -173,15 +174,15 @@ actor ClangLanguageService: LanguageService, MessageHandler { "-compile_args_from=lsp", // Provide compiler args programmatically. "-background-index=false", // Disable clangd indexing, we use the build "-index=false", // system index store instead. - ] + clangdOptions, + ] + (options.clangdOptions ?? []), name: "clangd", protocol: MessageRegistry.lspProtocol, stderrLoggingCategory: "clangd-stderr", client: self, - terminationHandler: { [weak self] terminationStatus in + terminationHandler: { [weak self] terminationReason in guard let self = self else { return } Task { - await self.handleClangdTermination(terminationStatus: terminationStatus) + await self.handleClangdTermination(terminationReason: terminationReason) } } @@ -291,14 +292,20 @@ actor ClangLanguageService: LanguageService, MessageHandler { } /// Forward the given request to `clangd`. - /// - /// This method calls `readyToHandleNextRequest` once the request has been - /// transmitted to `clangd` and another request can be safely transmitted to - /// `clangd` while guaranteeing ordering. - /// - /// The response of the request is returned asynchronously as the return value. func forwardRequestToClangd(_ request: R) async throws -> R.Response { - return try await clangd.send(request) + let timeoutHandle = TimeoutHandle() + do { + return try await withTimeout(options.semanticServiceRestartTimeoutOrDefault, handle: timeoutHandle) { + await self.sourceKitLSPServer?.hooks.preForwardRequestToClangd?(request) + return try await self.clangd.send(request) + } + } catch let error as TimeoutError where error.handle == timeoutHandle { + logger.fault( + "Did not receive reply from clangd after \(self.options.semanticServiceRestartTimeoutOrDefault, privacy: .public). Terminating and restarting clangd." + ) + self.crash() + throw error + } } package func canonicalDeclarationPosition(of position: Position, in uri: DocumentURI) async -> Position? { diff --git a/Sources/SourceKitLSP/Hooks.swift b/Sources/SourceKitLSP/Hooks.swift index 5b685b088..bd891c20e 100644 --- a/Sources/SourceKitLSP/Hooks.swift +++ b/Sources/SourceKitLSP/Hooks.swift @@ -30,6 +30,11 @@ public struct Hooks: Sendable { /// This allows requests to be artificially delayed. package var preHandleRequest: (@Sendable (any RequestType) async -> Void)? + /// Closure that is executed before a request is forwarded to clangd. + /// + /// This allows tests to simulate a `clangd` process that's unresponsive. + package var preForwardRequestToClangd: (@Sendable (any RequestType) async -> Void)? + public init() { self.init(indexHooks: IndexHooks(), buildSystemHooks: BuildSystemHooks()) } @@ -37,10 +42,12 @@ public struct Hooks: Sendable { package init( indexHooks: IndexHooks = IndexHooks(), buildSystemHooks: BuildSystemHooks = BuildSystemHooks(), - preHandleRequest: (@Sendable (any RequestType) async -> Void)? = nil + preHandleRequest: (@Sendable (any RequestType) async -> Void)? = nil, + preForwardRequestToClangd: (@Sendable (any RequestType) async -> Void)? = nil ) { self.indexHooks = indexHooks self.buildSystemHooks = buildSystemHooks self.preHandleRequest = preHandleRequest + self.preForwardRequestToClangd = preForwardRequestToClangd } } diff --git a/Sources/SourceKitLSP/Swift/CodeCompletionSession.swift b/Sources/SourceKitLSP/Swift/CodeCompletionSession.swift index da8d93ddc..299785445 100644 --- a/Sources/SourceKitLSP/Swift/CodeCompletionSession.swift +++ b/Sources/SourceKitLSP/Swift/CodeCompletionSession.swift @@ -201,6 +201,7 @@ class CodeCompletionSession { return await Self.resolveDocumentation( in: item, timeout: session.options.sourcekitdRequestTimeoutOrDefault, + restartTimeout: session.options.semanticServiceRestartTimeoutOrDefault, sourcekitd: sourcekitd ) } @@ -287,11 +288,7 @@ class CodeCompletionSession { keys.codeCompleteOptions: optionsDictionary(filterText: filterText), ]) - let dict = try await sourcekitd.send( - req, - timeout: options.sourcekitdRequestTimeoutOrDefault, - fileContents: snapshot.text - ) + let dict = try await sendSourceKitdRequest(req, snapshot: snapshot) self.state = .open guard let completions: SKDResponseArray = dict[keys.results] else { @@ -325,11 +322,7 @@ class CodeCompletionSession { keys.codeCompleteOptions: optionsDictionary(filterText: filterText), ]) - let dict = try await sourcekitd.send( - req, - timeout: options.sourcekitdRequestTimeoutOrDefault, - fileContents: snapshot.text - ) + let dict = try await sendSourceKitdRequest(req, snapshot: snapshot) guard let completions: SKDResponseArray = dict[keys.results] else { return CompletionList(isIncomplete: false, items: []) } @@ -378,13 +371,25 @@ class CodeCompletionSession { keys.codeCompleteOptions: [keys.useNewAPI: 1], ]) logger.info("Closing code completion session: \(self.description)") - _ = try? await sourcekitd.send(req, timeout: options.sourcekitdRequestTimeoutOrDefault, fileContents: nil) + _ = try? await sendSourceKitdRequest(req, snapshot: nil) self.state = .closed } } // MARK: - Helpers + private func sendSourceKitdRequest( + _ request: SKDRequestDictionary, + snapshot: DocumentSnapshot? + ) async throws -> SKDResponseDictionary { + try await sourcekitd.send( + request, + timeout: options.sourcekitdRequestTimeoutOrDefault, + restartTimeout: options.semanticServiceRestartTimeoutOrDefault, + fileContents: snapshot?.text + ) + } + private func expandClosurePlaceholders(insertText: String) -> String? { guard insertText.contains("<#") && insertText.contains("->") else { // Fast path: There is no closure placeholder to expand @@ -532,8 +537,14 @@ class CodeCompletionSession { } if !clientSupportsDocumentationResolve { + let semanticServiceRestartTimeoutOrDefault = self.options.semanticServiceRestartTimeoutOrDefault completionItems = await completionItems.asyncMap { item in - return await Self.resolveDocumentation(in: item, timeout: .seconds(1), sourcekitd: sourcekitd) + return await Self.resolveDocumentation( + in: item, + timeout: .seconds(1), + restartTimeout: semanticServiceRestartTimeoutOrDefault, + sourcekitd: sourcekitd + ) } } @@ -543,6 +554,7 @@ class CodeCompletionSession { private static func resolveDocumentation( in item: CompletionItem, timeout: Duration, + restartTimeout: Duration, sourcekitd: SourceKitD ) async -> CompletionItem { var item = item @@ -552,7 +564,7 @@ class CodeCompletionSession { sourcekitd.keys.identifier: itemId, ]) let documentationResponse = await orLog("Retrieving documentation for completion item") { - try await sourcekitd.send(req, timeout: timeout, fileContents: nil) + try await sourcekitd.send(req, timeout: timeout, restartTimeout: restartTimeout, fileContents: nil) } if let docString: String = documentationResponse?[sourcekitd.keys.docBrief] { item.documentation = .markupContent(MarkupContent(kind: .markdown, value: docString)) diff --git a/Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift b/Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift index 6156bcc41..3fb74ee21 100644 --- a/Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift +++ b/Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift @@ -124,6 +124,7 @@ actor DiagnosticReportManager { dict = try await self.sourcekitd.send( skreq, timeout: options.sourcekitdRequestTimeoutOrDefault, + restartTimeout: options.semanticServiceRestartTimeoutOrDefault, fileContents: snapshot.text ) } catch SKDError.requestFailed(let sourcekitdError) { diff --git a/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift index 9fcb132f2..85ab48a7c 100644 --- a/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift +++ b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift @@ -101,7 +101,7 @@ package actor SwiftLanguageService: LanguageService, Sendable { private let sourcekitdPath: URL - let sourcekitd: SourceKitD + package let sourcekitd: SourceKitD /// Path to the swift-format executable if it exists in the toolchain. let swiftFormat: URL? @@ -307,6 +307,7 @@ package actor SwiftLanguageService: LanguageService, Sendable { try await sourcekitd.send( request, timeout: options.sourcekitdRequestTimeoutOrDefault, + restartTimeout: options.semanticServiceRestartTimeoutOrDefault, fileContents: fileContents ) } diff --git a/Sources/SwiftExtensions/AsyncUtils.swift b/Sources/SwiftExtensions/AsyncUtils.swift index 76bbce12d..cc587e769 100644 --- a/Sources/SwiftExtensions/AsyncUtils.swift +++ b/Sources/SwiftExtensions/AsyncUtils.swift @@ -181,12 +181,30 @@ extension Collection where Element: Sendable { package struct TimeoutError: Error, CustomStringConvertible { package var description: String { "Timed out" } + package let handle: TimeoutHandle? + + package init(handle: TimeoutHandle?) { + self.handle = handle + } +} + +package final class TimeoutHandle: Equatable, Sendable { package init() {} + + static package func == (_ lhs: TimeoutHandle, _ rhs: TimeoutHandle) -> Bool { + return lhs === rhs + } } -/// Executes `body`. If it doesn't finish after `duration`, throws a `TimeoutError`. +/// Executes `body`. If it doesn't finish after `duration`, throws a `TimeoutError` and cancels `body`. +/// +/// `TimeoutError` is thrown immediately an the function does not wait for `body` to honor the cancellation. +/// +/// If a `handle` is passed in and this `withTimeout` call times out, the thrown `TimeoutError` contains this handle. +/// This way a caller can identify whether this call to `withTimeout` timed out or if a nested call timed out. package func withTimeout( _ duration: Duration, + handle: TimeoutHandle? = nil, _ body: @escaping @Sendable () async throws -> T ) async throws -> T { // Get the priority with which to launch the body task here so that we can pass the same priority as the initial @@ -207,7 +225,7 @@ package func withTimeout( let timeoutTask = Task(priority: priority) { try await Task.sleep(for: duration) - continuation.yield(with: .failure(TimeoutError())) + continuation.yield(with: .failure(TimeoutError(handle: handle))) bodyTask.cancel() } mutableTasks = [bodyTask, timeoutTask] diff --git a/Tests/SourceKitDTests/SourceKitDTests.swift b/Tests/SourceKitDTests/SourceKitDTests.swift index 12688c151..8a9650cd2 100644 --- a/Tests/SourceKitDTests/SourceKitDTests.swift +++ b/Tests/SourceKitDTests/SourceKitDTests.swift @@ -84,7 +84,7 @@ final class SourceKitDTests: XCTestCase { keys.compilerArgs: args, ]) - _ = try await sourcekitd.send(req, timeout: defaultTimeoutDuration, fileContents: nil) + _ = try await sourcekitd.send(req, timeout: defaultTimeoutDuration) try await fulfillmentOfOrThrow(expectation1, expectation2) @@ -92,7 +92,7 @@ final class SourceKitDTests: XCTestCase { keys.request: sourcekitd.requests.editorClose, keys.name: path, ]) - _ = try await sourcekitd.send(close, timeout: defaultTimeoutDuration, fileContents: nil) + _ = try await sourcekitd.send(close, timeout: defaultTimeoutDuration) } } diff --git a/Tests/SourceKitLSPTests/ClangdTests.swift b/Tests/SourceKitLSPTests/ClangdTests.swift index 8583e459f..10d2aa2a9 100644 --- a/Tests/SourceKitLSPTests/ClangdTests.swift +++ b/Tests/SourceKitLSPTests/ClangdTests.swift @@ -11,7 +11,12 @@ //===----------------------------------------------------------------------===// import LanguageServerProtocol +import SKLogging +import SKOptions import SKTestSupport +import SourceKitLSP +import SwiftExtensions +import TSCBasic import XCTest final class ClangdTests: XCTestCase { @@ -127,4 +132,58 @@ final class ClangdTests: XCTestCase { } } } + + func testRestartClangdIfItDoesntReply() async throws { + // We simulate clangd not replying until it is restarted using a hook. + let clangdRestarted = AtomicBool(initialValue: false) + let clangdRestartedExpectation = self.expectation(description: "clangd restarted") + let hooks = Hooks(preForwardRequestToClangd: { request in + if !clangdRestarted.value { + try? await Task.sleep(for: .seconds(60 * 60)) + } + }) + + let testClient = try await TestSourceKitLSPClient( + options: SourceKitLSPOptions(semanticServiceRestartTimeout: 1), + hooks: hooks + ) + let uri = DocumentURI(for: .c) + let positions = testClient.openDocument( + """ + void test() { + int x1️⃣; + } + """, + uri: uri + ) + + // Monitor clangd to notice when it gets restarted + let clangdServer = try await unwrap( + testClient.server.languageService(for: uri, .c, in: unwrap(testClient.server.workspaceForDocument(uri: uri))) + ) + await clangdServer.addStateChangeHandler { oldState, newState in + if oldState == .connectionInterrupted, newState == .connected { + clangdRestarted.value = true + clangdRestartedExpectation.fulfill() + } + } + + // The first hover request should get cancelled by `semanticServiceRestartTimeout` + await assertThrowsError( + try await testClient.send(HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])) + ) { error in + XCTAssert( + (error as? ResponseError)?.message.contains("Timed out") ?? false, + "Received unexpected error: \(error)" + ) + } + + try await fulfillmentOfOrThrow(clangdRestartedExpectation) + + // After clangd gets restarted + let hover = try await testClient.send( + HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) + ) + assertContains(hover?.contents.markupContent?.value ?? "", "Type: int") + } } diff --git a/Tests/SourceKitDTests/CrashRecoveryTests.swift b/Tests/SourceKitLSPTests/CrashRecoveryTests.swift similarity index 81% rename from Tests/SourceKitDTests/CrashRecoveryTests.swift rename to Tests/SourceKitLSPTests/CrashRecoveryTests.swift index add109f85..55b313d85 100644 --- a/Tests/SourceKitDTests/CrashRecoveryTests.swift +++ b/Tests/SourceKitLSPTests/CrashRecoveryTests.swift @@ -13,6 +13,7 @@ import LanguageServerProtocol import LanguageServerProtocolExtensions import SKLogging +import SKOptions import SKTestSupport import SourceKitD @_spi(Testing) import SourceKitLSP @@ -349,4 +350,70 @@ final class CrashRecoveryTests: XCTestCase { return true } } + + func testRestartSourceKitDIfItDoesntReply() async throws { + try SkipUnless.longTestsEnabled() + try SkipUnless.platformIsDarwin("Linux and Windows use in-process sourcekitd") + + let sourcekitdTerminatedExpectation = self.expectation(description: "sourcekitd terminated") + + let testClient = try await TestSourceKitLSPClient(options: SourceKitLSPOptions(semanticServiceRestartTimeout: 2)) + let uri = DocumentURI(for: .swift) + let positions = testClient.openDocument( + """ + func test() { + let 1️⃣x = 1 + } + """, + uri: uri + ) + + // Monitor sourcekitd to notice when it gets terminated + let swiftService = try await unwrap( + testClient.server.languageService(for: uri, .swift, in: unwrap(testClient.server.workspaceForDocument(uri: uri))) + as? SwiftLanguageService + ) + await swiftService.addStateChangeHandler { oldState, newState in + logger.debug("sourcekitd changed state: \(String(describing: oldState)) -> \(String(describing: newState))") + if newState == .connectionInterrupted { + sourcekitdTerminatedExpectation.fulfill() + } + } + + try await swiftService.sourcekitd.withPreRequestHandlingHook { + // The first hover request should get cancelled by `semanticServiceRestartTimeout` + await assertThrowsError( + try await testClient.send(HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])) + ) { error in + XCTAssert( + (error as? ResponseError)?.message.contains("Timed out") ?? false, + "Received unexpected error: \(error)" + ) + } + } hook: { request in + // Simulate a stuck sourcekitd that only gets unstuck when sourcekitd is terminated. + if request.description.contains("cursorinfo") { + // Use a detached task here so that a cancellation of the Task that runs this doesn't cancel the await of + // sourcekitdTerminatedExpectation. We want to simulate a sourecekitd that is stuck and doesn't listen to + // cancellation. + await Task.detached { + await orLog("awaiting sourcekitdTerminatedExpectation") { + try await fulfillmentOfOrThrow(sourcekitdTerminatedExpectation) + } + }.value + } + } + + // After sourcekitd is restarted, we should get a hover result once the semantic editor is enabled again. + try await repeatUntilExpectedResult { + do { + let hover = try await testClient.send( + HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) + ) + return hover?.contents.markupContent?.value != nil + } catch { + return false + } + } + } } diff --git a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift index 17cd1c8f9..b99e69b36 100644 --- a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift +++ b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift @@ -1830,7 +1830,7 @@ fileprivate extension SourceKitD { keys.syntacticOnly: 1, keys.compilerArgs: compilerArguments as [SKDRequestValue], ]) - _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + _ = try await send(req, timeout: defaultTimeoutDuration) return DocumentPositions(markers: markers, textWithoutMarkers: textWithoutMarkers) } @@ -1844,7 +1844,7 @@ fileprivate extension SourceKitD { keys.syntacticOnly: 1, ]) - _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + _ = try await send(req, timeout: defaultTimeoutDuration) } nonisolated func closeDocument(_ name: String) async throws { @@ -1853,7 +1853,7 @@ fileprivate extension SourceKitD { keys.name: name, ]) - _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + _ = try await send(req, timeout: defaultTimeoutDuration) } nonisolated func completeImpl( @@ -1888,7 +1888,7 @@ fileprivate extension SourceKitD { keys.compilerArgs: compilerArguments as [SKDRequestValue]?, ]) - let res = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + let res = try await send(req, timeout: defaultTimeoutDuration) return try CompletionResultSet(res) } @@ -1946,7 +1946,7 @@ fileprivate extension SourceKitD { keys.codeCompleteOptions: dictionary([keys.useNewAPI: 1]), ]) - _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + _ = try await send(req, timeout: defaultTimeoutDuration) } nonisolated func completeDocumentation(id: Int) async throws -> CompletionDocumentation { @@ -1955,7 +1955,7 @@ fileprivate extension SourceKitD { keys.identifier: id, ]) - let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + let resp = try await send(req, timeout: defaultTimeoutDuration) return CompletionDocumentation(resp) } @@ -1964,7 +1964,7 @@ fileprivate extension SourceKitD { keys.request: requests.codeCompleteDiagnostic, keys.identifier: id, ]) - let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + let resp = try await send(req, timeout: defaultTimeoutDuration) return CompletionDiagnostic(resp) } @@ -1973,7 +1973,7 @@ fileprivate extension SourceKitD { let req = dictionary([ keys.request: requests.dependencyUpdated ]) - _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + _ = try await send(req, timeout: defaultTimeoutDuration) } nonisolated func setPopularAPI(popular: [String], unpopular: [String]) async throws { @@ -1984,7 +1984,7 @@ fileprivate extension SourceKitD { keys.unpopular: unpopular as [SKDRequestValue], ]) - let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + let resp = try await send(req, timeout: defaultTimeoutDuration) XCTAssertEqual(resp[keys.useNewAPI], 1) } @@ -2001,7 +2001,7 @@ fileprivate extension SourceKitD { keys.notoriousModules: notoriousModules as [SKDRequestValue], ]) - let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + let resp = try await send(req, timeout: defaultTimeoutDuration) XCTAssertEqual(resp[keys.useNewAPI], 1) } @@ -2025,7 +2025,7 @@ fileprivate extension SourceKitD { keys.modulePopularity: modulePopularity as [SKDRequestValue], ]) - let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) + let resp = try await send(req, timeout: defaultTimeoutDuration) XCTAssertEqual(resp[keys.useNewAPI], 1) } diff --git a/config.schema.json b/config.schema.json index 2fb4b7ba7..17a6d5965 100644 --- a/config.schema.json +++ b/config.schema.json @@ -181,6 +181,11 @@ }, "type" : "object" }, + "semanticServiceRestartTimeout" : { + "description" : "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.", + "markdownDescription" : "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.", + "type" : "number" + }, "sourcekitd" : { "description" : "Options modifying the behavior of sourcekitd.", "markdownDescription" : "Options modifying the behavior of sourcekitd.", From 3113bdbcc0afa0240443f028672d15e9d7177415 Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Thu, 8 May 2025 15:07:07 -0400 Subject: [PATCH 4/7] Add SwiftASN1 as an explicit dependency on BuildSystemIntegration The SwiftPM smoke test still fails to build. I see the SwiftASN1 modules aren't getting added to the include path. Hopefully an explicit dependency will fix that. --- Sources/BuildSystemIntegration/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Sources/BuildSystemIntegration/CMakeLists.txt b/Sources/BuildSystemIntegration/CMakeLists.txt index eae79056f..7de03d3d6 100644 --- a/Sources/BuildSystemIntegration/CMakeLists.txt +++ b/Sources/BuildSystemIntegration/CMakeLists.txt @@ -35,7 +35,8 @@ target_link_libraries(BuildSystemIntegration PUBLIC PackageModel TSCBasic Build - SourceKitLSPAPI) + SourceKitLSPAPI + SwiftASN1) target_link_libraries(BuildSystemIntegration PRIVATE SKUtilities From a26b0b7b3a4478c7d8d47106b2b386c07e12098e Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 9 May 2025 09:12:03 +0200 Subject: [PATCH 5/7] Resolve swiftly when referenced in compile commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the compiler in `compile_commands.json` references a `swift` executable that’s a symlink to `swiftly`, SourceKit-LSP got confused because the `swift` executable doesn’t reside in a real toolchain, causing us to not provide any semantic functionality. When we discover that the `swift` executable reference in compile commands references a `swiftly` executable, use `swiftly use -p` to resolve the binary in the real toolchain and continue operating based on that. Fixes #2128 rdar://150301344 --- .../BuildTargetIdentifierExtensions.swift | 1 - Sources/BuildSystemIntegration/CMakeLists.txt | 1 + .../JSONCompilationDatabaseBuildSystem.swift | 46 +++++-- .../SwiftlyResolver.swift | 74 +++++++++++ Sources/SKTestSupport/CreateBinary.swift | 38 ++++++ .../CompilationDatabaseTests.swift | 118 +++++++++++++++++- Tests/SourceKitLSPTests/FormattingTests.swift | 17 +-- 7 files changed, 272 insertions(+), 23 deletions(-) create mode 100644 Sources/BuildSystemIntegration/SwiftlyResolver.swift create mode 100644 Sources/SKTestSupport/CreateBinary.swift diff --git a/Sources/BuildSystemIntegration/BuildTargetIdentifierExtensions.swift b/Sources/BuildSystemIntegration/BuildTargetIdentifierExtensions.swift index 10185b899..db38c3f4e 100644 --- a/Sources/BuildSystemIntegration/BuildTargetIdentifierExtensions.swift +++ b/Sources/BuildSystemIntegration/BuildTargetIdentifierExtensions.swift @@ -99,7 +99,6 @@ extension BuildTargetIdentifier { // MARK: BuildTargetIdentifier for CompileCommands extension BuildTargetIdentifier { - /// - Important: *For testing only* package static func createCompileCommands(compiler: String) throws -> BuildTargetIdentifier { var components = URLComponents() components.scheme = "compilecommands" diff --git a/Sources/BuildSystemIntegration/CMakeLists.txt b/Sources/BuildSystemIntegration/CMakeLists.txt index eae79056f..92249b358 100644 --- a/Sources/BuildSystemIntegration/CMakeLists.txt +++ b/Sources/BuildSystemIntegration/CMakeLists.txt @@ -19,6 +19,7 @@ add_library(BuildSystemIntegration STATIC LegacyBuildServerBuildSystem.swift MainFilesProvider.swift SplitShellCommand.swift + SwiftlyResolver.swift SwiftPMBuildSystem.swift) set_target_properties(BuildSystemIntegration PROPERTIES INTERFACE_INCLUDE_DIRECTORIES ${CMAKE_Swift_MODULE_DIRECTORY}) diff --git a/Sources/BuildSystemIntegration/JSONCompilationDatabaseBuildSystem.swift b/Sources/BuildSystemIntegration/JSONCompilationDatabaseBuildSystem.swift index 45e03f870..4227bcc0f 100644 --- a/Sources/BuildSystemIntegration/JSONCompilationDatabaseBuildSystem.swift +++ b/Sources/BuildSystemIntegration/JSONCompilationDatabaseBuildSystem.swift @@ -25,8 +25,23 @@ fileprivate extension CompilationDatabaseCompileCommand { /// without specifying a path. /// /// The absence of a compiler means we have an empty command line, which should never happen. - var compiler: String? { - return commandLine.first + /// + /// If the compiler is a symlink to `swiftly`, it uses `swiftlyResolver` to find the corresponding executable in a + /// real toolchain and returns that executable. + func compiler(swiftlyResolver: SwiftlyResolver) async -> String? { + guard let compiler = commandLine.first else { + return nil + } + let swiftlyResolved = await orLog("Resolving swiftly") { + try await swiftlyResolver.resolve( + compiler: URL(fileURLWithPath: compiler), + workingDirectory: URL(fileURLWithPath: directory) + )?.filePath + } + if let swiftlyResolved { + return swiftlyResolved + } + return compiler } } @@ -49,6 +64,8 @@ package actor JSONCompilationDatabaseBuildSystem: BuiltInBuildSystem { package let configPath: URL + private let swiftlyResolver = SwiftlyResolver() + // Watch for all all changes to `compile_commands.json` and `compile_flags.txt` instead of just the one at // `configPath` so that we cover the following semi-common scenario: // The user has a build that stores `compile_commands.json` in `mybuild`. In order to pick it up, they create a @@ -56,7 +73,8 @@ package actor JSONCompilationDatabaseBuildSystem: BuiltInBuildSystem { // about the change to `mybuild/compile_commands.json` because it effectively changes the contents of // `/compile_commands.json`. package let fileWatchers: [FileSystemWatcher] = [ - FileSystemWatcher(globPattern: "**/compile_commands.json", kind: [.create, .change, .delete]) + FileSystemWatcher(globPattern: "**/compile_commands.json", kind: [.create, .change, .delete]), + FileSystemWatcher(globPattern: "**/.swift-version", kind: [.create, .change, .delete]), ] private var _indexStorePath: LazyValue = .uninitialized @@ -92,7 +110,11 @@ package actor JSONCompilationDatabaseBuildSystem: BuiltInBuildSystem { } package func buildTargets(request: WorkspaceBuildTargetsRequest) async throws -> WorkspaceBuildTargetsResponse { - let compilers = Set(compdb.commands.compactMap(\.compiler)).sorted { $0 < $1 } + let compilers = Set( + await compdb.commands.asyncCompactMap { (command) -> String? in + await command.compiler(swiftlyResolver: swiftlyResolver) + } + ).sorted { $0 < $1 } let targets = try await compilers.asyncMap { compiler in let toolchainUri: URI? = if let toolchainPath = await toolchainRegistry.toolchain(withCompiler: URL(fileURLWithPath: compiler))?.path { @@ -115,12 +137,12 @@ package actor JSONCompilationDatabaseBuildSystem: BuiltInBuildSystem { } package func buildTargetSources(request: BuildTargetSourcesRequest) async throws -> BuildTargetSourcesResponse { - let items = request.targets.compactMap { (target) -> SourcesItem? in + let items = await request.targets.asyncCompactMap { (target) -> SourcesItem? in guard let targetCompiler = orLog("Compiler for target", { try target.compileCommandsCompiler }) else { return nil } - let commandsWithRequestedCompilers = compdb.commands.lazy.filter { command in - return targetCompiler == command.compiler + let commandsWithRequestedCompilers = await compdb.commands.lazy.asyncFilter { command in + return await targetCompiler == command.compiler(swiftlyResolver: swiftlyResolver) } let sources = commandsWithRequestedCompilers.map { SourceItem(uri: $0.uri, kind: .file, generated: false) @@ -131,10 +153,14 @@ package actor JSONCompilationDatabaseBuildSystem: BuiltInBuildSystem { return BuildTargetSourcesResponse(items: items) } - package func didChangeWatchedFiles(notification: OnWatchedFilesDidChangeNotification) { + package func didChangeWatchedFiles(notification: OnWatchedFilesDidChangeNotification) async { if notification.changes.contains(where: { $0.uri.fileURL?.lastPathComponent == Self.dbName }) { self.reloadCompilationDatabase() } + if notification.changes.contains(where: { $0.uri.fileURL?.lastPathComponent == ".swift-version" }) { + await swiftlyResolver.clearCache() + connectionToSourceKitLSP.send(OnBuildTargetDidChangeNotification(changes: nil)) + } } package func prepare(request: BuildTargetPrepareRequest) async throws -> VoidResponse { @@ -145,8 +171,8 @@ package actor JSONCompilationDatabaseBuildSystem: BuiltInBuildSystem { request: TextDocumentSourceKitOptionsRequest ) async throws -> TextDocumentSourceKitOptionsResponse? { let targetCompiler = try request.target.compileCommandsCompiler - let command = compdb[request.textDocument.uri].filter { - $0.compiler == targetCompiler + let command = await compdb[request.textDocument.uri].asyncFilter { + return await $0.compiler(swiftlyResolver: swiftlyResolver) == targetCompiler }.first guard let command else { return nil diff --git a/Sources/BuildSystemIntegration/SwiftlyResolver.swift b/Sources/BuildSystemIntegration/SwiftlyResolver.swift new file mode 100644 index 000000000..52372a5d1 --- /dev/null +++ b/Sources/BuildSystemIntegration/SwiftlyResolver.swift @@ -0,0 +1,74 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2025 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 +// +//===----------------------------------------------------------------------===// + +import Foundation +import SKUtilities +import SwiftExtensions +import TSCExtensions + +import struct TSCBasic.AbsolutePath +import class TSCBasic.Process + +/// Given a path to a compiler, which might be a symlink to `swiftly`, this type determines the compiler executable in +/// an actual toolchain. It also caches the results. The client needs to invalidate the cache if the path that swiftly +/// might resolve to has changed, eg. because `.swift-version` has been updated. +actor SwiftlyResolver { + private struct CacheKey: Hashable { + let compiler: URL + let workingDirectory: URL? + } + + private var cache: LRUCache> = LRUCache(capacity: 100) + + /// Check if `compiler` is a symlink to `swiftly`. If so, find the executable in the toolchain that swiftly resolves + /// to within the given working directory and return the URL of the corresponding compiler in that toolchain. + /// If `compiler` does not resolve to `swiftly`, return `nil`. + func resolve(compiler: URL, workingDirectory: URL?) async throws -> URL? { + let cacheKey = CacheKey(compiler: compiler, workingDirectory: workingDirectory) + if let cached = cache[cacheKey] { + return try cached.get() + } + let computed: Result + do { + computed = .success( + try await resolveSwiftlyTrampolineImpl(compiler: compiler, workingDirectory: workingDirectory) + ) + } catch { + computed = .failure(error) + } + cache[cacheKey] = computed + return try computed.get() + } + + private func resolveSwiftlyTrampolineImpl(compiler: URL, workingDirectory: URL?) async throws -> URL? { + let realpath = try compiler.realpath + guard realpath.lastPathComponent == "swiftly" else { + return nil + } + let swiftlyResult = try await Process.run( + arguments: [realpath.filePath, "use", "-p"], + workingDirectory: try AbsolutePath(validatingOrNil: workingDirectory?.filePath) + ) + let swiftlyToolchain = URL( + fileURLWithPath: try swiftlyResult.utf8Output().trimmingCharacters(in: .whitespacesAndNewlines) + ) + let resolvedCompiler = swiftlyToolchain.appending(components: "usr", "bin", compiler.lastPathComponent) + if FileManager.default.fileExists(at: resolvedCompiler) { + return resolvedCompiler + } + return nil + } + + func clearCache() { + cache.removeAll() + } +} diff --git a/Sources/SKTestSupport/CreateBinary.swift b/Sources/SKTestSupport/CreateBinary.swift new file mode 100644 index 000000000..fba3aec79 --- /dev/null +++ b/Sources/SKTestSupport/CreateBinary.swift @@ -0,0 +1,38 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2025 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 +// +//===----------------------------------------------------------------------===// + +package import Foundation +import SwiftExtensions +import ToolchainRegistry +import XCTest + +import class TSCBasic.Process + +/// Compiles the given Swift source code into a binary at `executablePath`. +package func createBinary(_ sourceCode: String, at executablePath: URL) async throws { + try await withTestScratchDir { scratchDir in + let sourceFile = scratchDir.appending(component: "source.swift") + try await sourceCode.writeWithRetry(to: sourceFile) + + var compilerArguments = try [ + sourceFile.filePath, + "-o", + executablePath.filePath, + ] + if let defaultSDKPath { + compilerArguments += ["-sdk", defaultSDKPath] + } + try await Process.checkNonZeroExit( + arguments: [unwrap(ToolchainRegistry.forTesting.default?.swiftc?.filePath)] + compilerArguments + ) + } +} diff --git a/Tests/SourceKitLSPTests/CompilationDatabaseTests.swift b/Tests/SourceKitLSPTests/CompilationDatabaseTests.swift index 92a3a9f89..72c5143e0 100644 --- a/Tests/SourceKitLSPTests/CompilationDatabaseTests.swift +++ b/Tests/SourceKitLSPTests/CompilationDatabaseTests.swift @@ -11,10 +11,11 @@ //===----------------------------------------------------------------------===// import BuildSystemIntegration -import Foundation import LanguageServerProtocol import SKTestSupport +import SwiftExtensions import TSCBasic +import TSCExtensions import ToolchainRegistry import XCTest @@ -238,6 +239,121 @@ final class CompilationDatabaseTests: XCTestCase { assertContains(error.message, "No language service") } } + + func testLookThroughSwiftly() async throws { + try await withTestScratchDir { scratchDirectory in + let defaultToolchain = try await unwrap(ToolchainRegistry.forTesting.default) + + // We create a toolchain registry with the default toolchain, which is able to provide semantic functionality and + // a dummy toolchain that can't provide semantic functionality. + let fakeToolchainURL = scratchDirectory.appending(components: "fakeToolchain") + let fakeToolchain = Toolchain( + identifier: "fake", + displayName: "fake", + path: fakeToolchainURL, + clang: nil, + swift: fakeToolchainURL.appending(components: "usr", "bin", "swift"), + swiftc: fakeToolchainURL.appending(components: "usr", "bin", "swiftc"), + swiftFormat: nil, + clangd: nil, + sourcekitd: fakeToolchainURL.appending(components: "usr", "lib", "sourcekitd.framework", "sourcekitd"), + libIndexStore: nil + ) + let toolchainRegistry = ToolchainRegistry(toolchains: [ + try await unwrap(ToolchainRegistry.forTesting.default), fakeToolchain, + ]) + + // We need to create a file for the swift executable because `SwiftlyResolver` checks for its presence. + try FileManager.default.createDirectory( + at: XCTUnwrap(fakeToolchain.swift).deletingLastPathComponent(), + withIntermediateDirectories: true + ) + try await "".writeWithRetry(to: XCTUnwrap(fakeToolchain.swift)) + + // Create a dummy swiftly executable that picks the default toolchain for all file unless `fakeToolchain` is in + // the source file's path. + let dummySwiftlyExecutableUrl = scratchDirectory.appendingPathComponent("swiftly") + let dummySwiftExecutableUrl = scratchDirectory.appendingPathComponent("swift") + try FileManager.default.createSymbolicLink( + at: dummySwiftExecutableUrl, + withDestinationURL: dummySwiftlyExecutableUrl + ) + try await createBinary( + """ + import Foundation + + if FileManager.default.currentDirectoryPath.contains("fakeToolchain") { + print(#"\(fakeToolchain.path.filePath)"#) + } else { + print(#"\(defaultToolchain.path.filePath)"#) + } + """, + at: dummySwiftlyExecutableUrl + ) + + // Now create a project in which we have one file in a `realToolchain` directory for which our fake swiftly will + // pick the default toolchain and one in `fakeToolchain` for which swiftly will pick the fake toolchain. We should + // be able to get semantic functionality for the file in `realToolchain` but not for `fakeToolchain` because + // sourcekitd can't be launched for that toolchain (since it doesn't exist). + let dummySwiftExecutablePathForJSON = try dummySwiftExecutableUrl.filePath.replacing(#"\"#, with: #"\\"#) + + let project = try await MultiFileTestProject( + files: [ + "realToolchain/realToolchain.swift": """ + #warning("Test warning") + """, + "fakeToolchain/fakeToolchain.swift": """ + #warning("Test warning") + """, + "compile_commands.json": """ + [ + { + "directory": "$TEST_DIR_BACKSLASH_ESCAPED/realToolchain", + "arguments": [ + "\(dummySwiftExecutablePathForJSON)", + "$TEST_DIR_BACKSLASH_ESCAPED/realToolchain/realToolchain.swift", + \(defaultSDKArgs) + ], + "file": "realToolchain.swift", + "output": "$TEST_DIR_BACKSLASH_ESCAPED/realToolchain/test.swift.o" + }, + { + "directory": "$TEST_DIR_BACKSLASH_ESCAPED/fakeToolchain", + "arguments": [ + "\(dummySwiftExecutablePathForJSON)", + "$TEST_DIR_BACKSLASH_ESCAPED/fakeToolchain/fakeToolchain.swift", + \(defaultSDKArgs) + ], + "file": "fakeToolchain.swift", + "output": "$TEST_DIR_BACKSLASH_ESCAPED/fakeToolchain/test.swift.o" + } + ] + """, + ], + toolchainRegistry: toolchainRegistry + ) + + let (forRealToolchainUri, _) = try project.openDocument("realToolchain.swift") + let diagnostics = try await project.testClient.send( + DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(forRealToolchainUri)) + ) + XCTAssertEqual(diagnostics.fullReport?.items.map(\.message), ["Test warning"]) + + let (forDummyToolchainUri, _) = try project.openDocument("fakeToolchain.swift") + await assertThrowsError( + try await project.testClient.send( + DocumentDiagnosticsRequest(textDocument: TextDocumentIdentifier(forDummyToolchainUri)) + ) + ) { error in + guard let error = error as? ResponseError else { + XCTFail("Expected ResponseError, got \(error)") + return + } + // The actual error message here doesn't matter too much, we just need to check that we don't get diagnostics. + assertContains(error.message, "No language service") + } + } + } } fileprivate let defaultSDKArgs: String = { diff --git a/Tests/SourceKitLSPTests/FormattingTests.swift b/Tests/SourceKitLSPTests/FormattingTests.swift index d996a5c8a..fd11442e5 100644 --- a/Tests/SourceKitLSPTests/FormattingTests.swift +++ b/Tests/SourceKitLSPTests/FormattingTests.swift @@ -299,18 +299,13 @@ final class FormattingTests: XCTestCase { try await withTestScratchDir { scratchDir in let toolchain = try await unwrap(ToolchainRegistry.forTesting.default) - let crashingSwiftFilePath = scratchDir.appendingPathComponent("crashing-executable.swift") let crashingExecutablePath = scratchDir.appendingPathComponent("crashing-executable") - try await "fatalError()".writeWithRetry(to: crashingSwiftFilePath) - var compilerArguments = try [ - crashingSwiftFilePath.filePath, - "-o", - crashingExecutablePath.filePath, - ] - if let defaultSDKPath { - compilerArguments += ["-sdk", defaultSDKPath] - } - try await Process.checkNonZeroExit(arguments: [XCTUnwrap(toolchain.swiftc?.filePath)] + compilerArguments) + try await createBinary( + """ + fatalError() + """, + at: crashingExecutablePath + ) let toolchainRegistry = ToolchainRegistry(toolchains: [ Toolchain( From 1bc7d85dbdae0ffcf1d7f19fa08da2c7a8a2e547 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Fri, 9 May 2025 13:33:35 +0200 Subject: [PATCH 6/7] Send `workspace/semanticTokens/refresh` to client when build settings have changed Similar to how we send a `workspace/diagnostic/refresh` request to the client when we get new build settings, we need to send a `workspace/semanticTokens/refresh` to the client to reload semantic tokens when builds settings change. This is particularly important so that we reload semantic tokens from real build setting after a SwiftPM package has finished loading (we previously used fallback settings). Fixes #2141 rdar://150934682 --- .../Swift/SwiftLanguageService.swift | 36 ++++++++++++------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift index d98bb6b13..41a435654 100644 --- a/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift +++ b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift @@ -183,14 +183,14 @@ package actor SwiftLanguageService: LanguageService, Sendable { /// `buildSettings(for:)` returns `nil` are not included. private var buildSettingsForOpenFiles: [DocumentURI: SwiftCompileCommand] = [:] - /// Calling `scheduleCall` on `refreshDiagnosticsDebouncer` schedules a `DiagnosticsRefreshRequest` to be sent to - /// to the client. + /// Calling `scheduleCall` on `refreshDiagnosticsAndSemanticTokensDebouncer` schedules a `DiagnosticsRefreshRequest` + /// and `WorkspaceSemanticTokensRefreshRequest` to be sent to to the client. /// /// We debounce these calls because the `DiagnosticsRefreshRequest` is a workspace-wide request. If we discover that /// the client should update diagnostics for file A and then discover that it should also update diagnostics for file /// B, we don't want to send two `DiagnosticsRefreshRequest`s. Instead, the two should be unified into a single /// request. - private let refreshDiagnosticsDebouncer: Debouncer + private let refreshDiagnosticsAndSemanticTokensDebouncer: Debouncer /// Creates a language server for the given client using the sourcekitd dylib specified in `toolchain`. /// `reopenDocuments` is a closure that will be called if sourcekitd crashes and the `SwiftLanguageService` asks its @@ -229,19 +229,29 @@ package actor SwiftLanguageService: LanguageService, Sendable { self.options = options // The debounce duration of 500ms was chosen arbitrarily without scientific research. - self.refreshDiagnosticsDebouncer = Debouncer(debounceDuration: .milliseconds(500)) { [weak sourceKitLSPServer] in + self.refreshDiagnosticsAndSemanticTokensDebouncer = Debouncer(debounceDuration: .milliseconds(500)) { + [weak sourceKitLSPServer] in guard let sourceKitLSPServer else { - logger.fault("Not sending DiagnosticRefreshRequest to client because sourceKitLSPServer has been deallocated") + logger.fault( + "Not sending diagnostic and semantic token refresh request to client because sourceKitLSPServer has been deallocated" + ) return } - guard - await sourceKitLSPServer.capabilityRegistry?.clientCapabilities.workspace?.diagnostics?.refreshSupport ?? false - else { + let clientCapabilities = await sourceKitLSPServer.capabilityRegistry?.clientCapabilities + if clientCapabilities?.workspace?.diagnostics?.refreshSupport ?? false { + _ = await orLog("Sending DiagnosticRefreshRequest to client after document dependencies updated") { + try await sourceKitLSPServer.sendRequestToClient(DiagnosticsRefreshRequest()) + } + } else { logger.debug("Not sending DiagnosticRefreshRequest because the client doesn't support it") - return } - _ = await orLog("Sending DiagnosticRefreshRequest to client after document dependencies updated") { - try await sourceKitLSPServer.sendRequestToClient(DiagnosticsRefreshRequest()) + + if clientCapabilities?.workspace?.semanticTokens?.refreshSupport ?? false { + _ = await orLog("Sending WorkspaceSemanticTokensRefreshRequest to client after document dependencies updated") { + try await sourceKitLSPServer.sendRequestToClient(WorkspaceSemanticTokensRefreshRequest()) + } + } else { + logger.debug("Not sending WorkspaceSemanticTokensRefreshRequest because the client doesn't support it") } } @@ -336,7 +346,7 @@ package actor SwiftLanguageService: LanguageService, Sendable { await sourceKitLSPServer.sourcekitdCrashedWorkDoneProgress.end() // We can provide diagnostics again now. Send a diagnostic refresh request to prompt the editor to reload // diagnostics. - await refreshDiagnosticsDebouncer.scheduleCall() + await refreshDiagnosticsAndSemanticTokensDebouncer.scheduleCall() case (.connected, .connected), (.connectionInterrupted, .connectionInterrupted), (.connectionInterrupted, .semanticFunctionalityDisabled), @@ -468,7 +478,7 @@ extension SwiftLanguageService { } if await capabilityRegistry.clientSupportsPullDiagnostics(for: .swift) { - await self.refreshDiagnosticsDebouncer.scheduleCall() + await self.refreshDiagnosticsAndSemanticTokensDebouncer.scheduleCall() } else { await publishDiagnosticsIfNeeded(for: snapshot.uri) } From fde7d70db18bbf8ba9513016c1aaf3ce8400df4b Mon Sep 17 00:00:00 2001 From: Ben Barham Date: Mon, 12 May 2025 16:35:59 -0700 Subject: [PATCH 7/7] Revert "Merge pull request #2137 from ahoppen/terminate-unresponsive-clangd-sourcekitd" This change is too risky for 6.2 at this point. --- Documentation/Configuration File.md | 1 - .../ExternalBuildSystemAdapter.swift | 4 +- .../JSONRPCConnection.swift | 26 +---- Sources/SKOptions/SourceKitLSPOptions.swift | 18 +-- Sources/SKTestSupport/SkipUnless.swift | 3 +- Sources/SKTestSupport/SourceKitD+send.swift | 29 ----- Sources/SourceKitD/SourceKitD.swift | 105 +++++------------- Sources/SourceKitD/SourceKitDRegistry.swift | 10 +- .../Clang/ClangLanguageService.swift | 35 +++--- Sources/SourceKitLSP/Hooks.swift | 9 +- .../Swift/CodeCompletionSession.swift | 38 +++---- .../Swift/DiagnosticReportManager.swift | 1 - .../Swift/SwiftLanguageService.swift | 3 +- Sources/SwiftExtensions/AsyncUtils.swift | 22 +--- .../CrashRecoveryTests.swift | 67 ----------- Tests/SourceKitDTests/SourceKitDTests.swift | 4 +- Tests/SourceKitLSPTests/ClangdTests.swift | 59 ---------- .../SwiftSourceKitPluginTests.swift | 22 ++-- config.schema.json | 5 - 19 files changed, 84 insertions(+), 377 deletions(-) delete mode 100644 Sources/SKTestSupport/SourceKitD+send.swift rename Tests/{SourceKitLSPTests => SourceKitDTests}/CrashRecoveryTests.swift (81%) diff --git a/Documentation/Configuration File.md b/Documentation/Configuration File.md index 055bb4c26..fedc0b746 100644 --- a/Documentation/Configuration File.md +++ b/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. diff --git a/Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift b/Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift index 372a4d211..2fc20e1ea 100644 --- a/Sources/BuildSystemIntegration/ExternalBuildSystemAdapter.swift +++ b/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() diff --git a/Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift b/Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift index 0c1853511..e6ba2ca39 100644 --- a/Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift +++ b/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() diff --git a/Sources/SKOptions/SourceKitLSPOptions.swift b/Sources/SKOptions/SourceKitLSPOptions.swift index 0adccbb17..208017ef5 100644 --- a/Sources/SKOptions/SourceKitLSPOptions.swift +++ b/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? = 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 ) } diff --git a/Sources/SKTestSupport/SkipUnless.swift b/Sources/SKTestSupport/SkipUnless.swift index 1c8830e23..4c25b324b 100644 --- a/Sources/SKTestSupport/SkipUnless.swift +++ b/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 { diff --git a/Sources/SKTestSupport/SourceKitD+send.swift b/Sources/SKTestSupport/SourceKitD+send.swift deleted file mode 100644 index c27287ab9..000000000 --- a/Sources/SKTestSupport/SourceKitD+send.swift +++ /dev/null @@ -1,29 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// This source file is part of the Swift.org open source project -// -// Copyright (c) 2014 - 2025 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 -// -//===----------------------------------------------------------------------===// - -package import SourceKitD - -extension SourceKitD { - /// Convenience overload of the `send` function for testing that doesn't restart sourcekitd if it does not respond - /// and doesn't pass any file contents. - package func send( - _ request: SKDRequestDictionary, - timeout: Duration - ) async throws -> SKDResponseDictionary { - return try await self.send( - request, - timeout: timeout, - restartTimeout: .seconds(60 * 60 * 24), - fileContents: nil - ) - } -} diff --git a/Sources/SourceKitD/SourceKitD.swift b/Sources/SourceKitD/SourceKitD.swift index 48fd94cab..4e634e019 100644 --- a/Sources/SourceKitD/SourceKitD.swift +++ b/Sources/SourceKitD/SourceKitD.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. diff --git a/Sources/SourceKitD/SourceKitDRegistry.swift b/Sources/SourceKitD/SourceKitDRegistry.swift index 420fdc2f8..b09e399a1 100644 --- a/Sources/SourceKitD/SourceKitDRegistry.swift +++ b/Sources/SourceKitD/SourceKitDRegistry.swift @@ -30,7 +30,7 @@ package actor SourceKitDRegistry { 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)] = [:] + private var cemetary: [URL: (pluginPaths: PluginPaths?, sourcekitd: WeakSourceKitD)] = [:] /// Initialize an empty registry. package init() {} @@ -49,8 +49,8 @@ package actor SourceKitDRegistry { } 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 { 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 } diff --git a/Sources/SourceKitLSP/Clang/ClangLanguageService.swift b/Sources/SourceKitLSP/Clang/ClangLanguageService.swift index 6b75bc788..c4bd6c3b5 100644 --- a/Sources/SourceKitLSP/Clang/ClangLanguageService.swift +++ b/Sources/SourceKitLSP/Clang/ClangLanguageService.swift @@ -66,7 +66,7 @@ actor ClangLanguageService: LanguageService, MessageHandler { /// Path to the `clangd` binary. let clangdPath: URL - let options: SourceKitLSPOptions + let clangdOptions: [String] /// The current state of the `clangd` language server. /// Changing the property automatically notified the state change handlers. @@ -121,7 +121,7 @@ actor ClangLanguageService: LanguageService, MessageHandler { } self.clangPath = toolchain.clang self.clangdPath = clangdPath - self.options = options + self.clangdOptions = options.clangdOptions ?? [] self.workspace = WeakWorkspace(workspace) self.state = .connected self.sourceKitLSPServer = sourceKitLSPServer @@ -158,11 +158,10 @@ actor ClangLanguageService: LanguageService, MessageHandler { /// Restarts `clangd` if it has crashed. /// /// - Parameter terminationStatus: The exit code of `clangd`. - private func handleClangdTermination(terminationReason: JSONRPCConnection.TerminationReason) { + private func handleClangdTermination(terminationStatus: Int32) { self.clangdProcess = nil - if terminationReason != .exited(exitCode: 0) { + if terminationStatus != 0 { self.state = .connectionInterrupted - logger.info("clangd crashed. Restarting it.") self.restartClangd() } } @@ -178,15 +177,15 @@ actor ClangLanguageService: LanguageService, MessageHandler { "-compile_args_from=lsp", // Provide compiler args programmatically. "-background-index=false", // Disable clangd indexing, we use the build "-index=false", // system index store instead. - ] + (options.clangdOptions ?? []), + ] + clangdOptions, name: "clangd", protocol: MessageRegistry.lspProtocol, stderrLoggingCategory: "clangd-stderr", client: self, - terminationHandler: { [weak self] terminationReason in + terminationHandler: { [weak self] terminationStatus in guard let self = self else { return } Task { - await self.handleClangdTermination(terminationReason: terminationReason) + await self.handleClangdTermination(terminationStatus: terminationStatus) } } @@ -296,20 +295,14 @@ actor ClangLanguageService: LanguageService, MessageHandler { } /// Forward the given request to `clangd`. + /// + /// This method calls `readyToHandleNextRequest` once the request has been + /// transmitted to `clangd` and another request can be safely transmitted to + /// `clangd` while guaranteeing ordering. + /// + /// The response of the request is returned asynchronously as the return value. func forwardRequestToClangd(_ request: R) async throws -> R.Response { - let timeoutHandle = TimeoutHandle() - do { - return try await withTimeout(options.semanticServiceRestartTimeoutOrDefault, handle: timeoutHandle) { - await self.sourceKitLSPServer?.hooks.preForwardRequestToClangd?(request) - return try await self.clangd.send(request) - } - } catch let error as TimeoutError where error.handle == timeoutHandle { - logger.fault( - "Did not receive reply from clangd after \(self.options.semanticServiceRestartTimeoutOrDefault, privacy: .public). Terminating and restarting clangd." - ) - self.crash() - throw error - } + return try await clangd.send(request) } package func canonicalDeclarationPosition(of position: Position, in uri: DocumentURI) async -> Position? { diff --git a/Sources/SourceKitLSP/Hooks.swift b/Sources/SourceKitLSP/Hooks.swift index bd891c20e..5b685b088 100644 --- a/Sources/SourceKitLSP/Hooks.swift +++ b/Sources/SourceKitLSP/Hooks.swift @@ -30,11 +30,6 @@ public struct Hooks: Sendable { /// This allows requests to be artificially delayed. package var preHandleRequest: (@Sendable (any RequestType) async -> Void)? - /// Closure that is executed before a request is forwarded to clangd. - /// - /// This allows tests to simulate a `clangd` process that's unresponsive. - package var preForwardRequestToClangd: (@Sendable (any RequestType) async -> Void)? - public init() { self.init(indexHooks: IndexHooks(), buildSystemHooks: BuildSystemHooks()) } @@ -42,12 +37,10 @@ public struct Hooks: Sendable { package init( indexHooks: IndexHooks = IndexHooks(), buildSystemHooks: BuildSystemHooks = BuildSystemHooks(), - preHandleRequest: (@Sendable (any RequestType) async -> Void)? = nil, - preForwardRequestToClangd: (@Sendable (any RequestType) async -> Void)? = nil + preHandleRequest: (@Sendable (any RequestType) async -> Void)? = nil ) { self.indexHooks = indexHooks self.buildSystemHooks = buildSystemHooks self.preHandleRequest = preHandleRequest - self.preForwardRequestToClangd = preForwardRequestToClangd } } diff --git a/Sources/SourceKitLSP/Swift/CodeCompletionSession.swift b/Sources/SourceKitLSP/Swift/CodeCompletionSession.swift index 299785445..da8d93ddc 100644 --- a/Sources/SourceKitLSP/Swift/CodeCompletionSession.swift +++ b/Sources/SourceKitLSP/Swift/CodeCompletionSession.swift @@ -201,7 +201,6 @@ class CodeCompletionSession { return await Self.resolveDocumentation( in: item, timeout: session.options.sourcekitdRequestTimeoutOrDefault, - restartTimeout: session.options.semanticServiceRestartTimeoutOrDefault, sourcekitd: sourcekitd ) } @@ -288,7 +287,11 @@ class CodeCompletionSession { keys.codeCompleteOptions: optionsDictionary(filterText: filterText), ]) - let dict = try await sendSourceKitdRequest(req, snapshot: snapshot) + let dict = try await sourcekitd.send( + req, + timeout: options.sourcekitdRequestTimeoutOrDefault, + fileContents: snapshot.text + ) self.state = .open guard let completions: SKDResponseArray = dict[keys.results] else { @@ -322,7 +325,11 @@ class CodeCompletionSession { keys.codeCompleteOptions: optionsDictionary(filterText: filterText), ]) - let dict = try await sendSourceKitdRequest(req, snapshot: snapshot) + let dict = try await sourcekitd.send( + req, + timeout: options.sourcekitdRequestTimeoutOrDefault, + fileContents: snapshot.text + ) guard let completions: SKDResponseArray = dict[keys.results] else { return CompletionList(isIncomplete: false, items: []) } @@ -371,25 +378,13 @@ class CodeCompletionSession { keys.codeCompleteOptions: [keys.useNewAPI: 1], ]) logger.info("Closing code completion session: \(self.description)") - _ = try? await sendSourceKitdRequest(req, snapshot: nil) + _ = try? await sourcekitd.send(req, timeout: options.sourcekitdRequestTimeoutOrDefault, fileContents: nil) self.state = .closed } } // MARK: - Helpers - private func sendSourceKitdRequest( - _ request: SKDRequestDictionary, - snapshot: DocumentSnapshot? - ) async throws -> SKDResponseDictionary { - try await sourcekitd.send( - request, - timeout: options.sourcekitdRequestTimeoutOrDefault, - restartTimeout: options.semanticServiceRestartTimeoutOrDefault, - fileContents: snapshot?.text - ) - } - private func expandClosurePlaceholders(insertText: String) -> String? { guard insertText.contains("<#") && insertText.contains("->") else { // Fast path: There is no closure placeholder to expand @@ -537,14 +532,8 @@ class CodeCompletionSession { } if !clientSupportsDocumentationResolve { - let semanticServiceRestartTimeoutOrDefault = self.options.semanticServiceRestartTimeoutOrDefault completionItems = await completionItems.asyncMap { item in - return await Self.resolveDocumentation( - in: item, - timeout: .seconds(1), - restartTimeout: semanticServiceRestartTimeoutOrDefault, - sourcekitd: sourcekitd - ) + return await Self.resolveDocumentation(in: item, timeout: .seconds(1), sourcekitd: sourcekitd) } } @@ -554,7 +543,6 @@ class CodeCompletionSession { private static func resolveDocumentation( in item: CompletionItem, timeout: Duration, - restartTimeout: Duration, sourcekitd: SourceKitD ) async -> CompletionItem { var item = item @@ -564,7 +552,7 @@ class CodeCompletionSession { sourcekitd.keys.identifier: itemId, ]) let documentationResponse = await orLog("Retrieving documentation for completion item") { - try await sourcekitd.send(req, timeout: timeout, restartTimeout: restartTimeout, fileContents: nil) + try await sourcekitd.send(req, timeout: timeout, fileContents: nil) } if let docString: String = documentationResponse?[sourcekitd.keys.docBrief] { item.documentation = .markupContent(MarkupContent(kind: .markdown, value: docString)) diff --git a/Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift b/Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift index a8aa78159..ccaa6ed68 100644 --- a/Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift +++ b/Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift @@ -118,7 +118,6 @@ actor DiagnosticReportManager { dict = try await self.sourcekitd.send( skreq, timeout: options.sourcekitdRequestTimeoutOrDefault, - restartTimeout: options.semanticServiceRestartTimeoutOrDefault, fileContents: snapshot.text ) } catch SKDError.requestFailed(let sourcekitdError) { diff --git a/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift index 06685db50..41a435654 100644 --- a/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift +++ b/Sources/SourceKitLSP/Swift/SwiftLanguageService.swift @@ -101,7 +101,7 @@ package actor SwiftLanguageService: LanguageService, Sendable { private let sourcekitdPath: URL - package let sourcekitd: SourceKitD + let sourcekitd: SourceKitD /// Path to the swift-format executable if it exists in the toolchain. let swiftFormat: URL? @@ -321,7 +321,6 @@ package actor SwiftLanguageService: LanguageService, Sendable { try await sourcekitd.send( request, timeout: options.sourcekitdRequestTimeoutOrDefault, - restartTimeout: options.semanticServiceRestartTimeoutOrDefault, fileContents: fileContents ) } diff --git a/Sources/SwiftExtensions/AsyncUtils.swift b/Sources/SwiftExtensions/AsyncUtils.swift index cc587e769..76bbce12d 100644 --- a/Sources/SwiftExtensions/AsyncUtils.swift +++ b/Sources/SwiftExtensions/AsyncUtils.swift @@ -181,30 +181,12 @@ extension Collection where Element: Sendable { package struct TimeoutError: Error, CustomStringConvertible { package var description: String { "Timed out" } - package let handle: TimeoutHandle? - - package init(handle: TimeoutHandle?) { - self.handle = handle - } -} - -package final class TimeoutHandle: Equatable, Sendable { package init() {} - - static package func == (_ lhs: TimeoutHandle, _ rhs: TimeoutHandle) -> Bool { - return lhs === rhs - } } -/// Executes `body`. If it doesn't finish after `duration`, throws a `TimeoutError` and cancels `body`. -/// -/// `TimeoutError` is thrown immediately an the function does not wait for `body` to honor the cancellation. -/// -/// If a `handle` is passed in and this `withTimeout` call times out, the thrown `TimeoutError` contains this handle. -/// This way a caller can identify whether this call to `withTimeout` timed out or if a nested call timed out. +/// Executes `body`. If it doesn't finish after `duration`, throws a `TimeoutError`. package func withTimeout( _ duration: Duration, - handle: TimeoutHandle? = nil, _ body: @escaping @Sendable () async throws -> T ) async throws -> T { // Get the priority with which to launch the body task here so that we can pass the same priority as the initial @@ -225,7 +207,7 @@ package func withTimeout( let timeoutTask = Task(priority: priority) { try await Task.sleep(for: duration) - continuation.yield(with: .failure(TimeoutError(handle: handle))) + continuation.yield(with: .failure(TimeoutError())) bodyTask.cancel() } mutableTasks = [bodyTask, timeoutTask] diff --git a/Tests/SourceKitLSPTests/CrashRecoveryTests.swift b/Tests/SourceKitDTests/CrashRecoveryTests.swift similarity index 81% rename from Tests/SourceKitLSPTests/CrashRecoveryTests.swift rename to Tests/SourceKitDTests/CrashRecoveryTests.swift index 55b313d85..add109f85 100644 --- a/Tests/SourceKitLSPTests/CrashRecoveryTests.swift +++ b/Tests/SourceKitDTests/CrashRecoveryTests.swift @@ -13,7 +13,6 @@ import LanguageServerProtocol import LanguageServerProtocolExtensions import SKLogging -import SKOptions import SKTestSupport import SourceKitD @_spi(Testing) import SourceKitLSP @@ -350,70 +349,4 @@ final class CrashRecoveryTests: XCTestCase { return true } } - - func testRestartSourceKitDIfItDoesntReply() async throws { - try SkipUnless.longTestsEnabled() - try SkipUnless.platformIsDarwin("Linux and Windows use in-process sourcekitd") - - let sourcekitdTerminatedExpectation = self.expectation(description: "sourcekitd terminated") - - let testClient = try await TestSourceKitLSPClient(options: SourceKitLSPOptions(semanticServiceRestartTimeout: 2)) - let uri = DocumentURI(for: .swift) - let positions = testClient.openDocument( - """ - func test() { - let 1️⃣x = 1 - } - """, - uri: uri - ) - - // Monitor sourcekitd to notice when it gets terminated - let swiftService = try await unwrap( - testClient.server.languageService(for: uri, .swift, in: unwrap(testClient.server.workspaceForDocument(uri: uri))) - as? SwiftLanguageService - ) - await swiftService.addStateChangeHandler { oldState, newState in - logger.debug("sourcekitd changed state: \(String(describing: oldState)) -> \(String(describing: newState))") - if newState == .connectionInterrupted { - sourcekitdTerminatedExpectation.fulfill() - } - } - - try await swiftService.sourcekitd.withPreRequestHandlingHook { - // The first hover request should get cancelled by `semanticServiceRestartTimeout` - await assertThrowsError( - try await testClient.send(HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])) - ) { error in - XCTAssert( - (error as? ResponseError)?.message.contains("Timed out") ?? false, - "Received unexpected error: \(error)" - ) - } - } hook: { request in - // Simulate a stuck sourcekitd that only gets unstuck when sourcekitd is terminated. - if request.description.contains("cursorinfo") { - // Use a detached task here so that a cancellation of the Task that runs this doesn't cancel the await of - // sourcekitdTerminatedExpectation. We want to simulate a sourecekitd that is stuck and doesn't listen to - // cancellation. - await Task.detached { - await orLog("awaiting sourcekitdTerminatedExpectation") { - try await fulfillmentOfOrThrow(sourcekitdTerminatedExpectation) - } - }.value - } - } - - // After sourcekitd is restarted, we should get a hover result once the semantic editor is enabled again. - try await repeatUntilExpectedResult { - do { - let hover = try await testClient.send( - HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) - ) - return hover?.contents.markupContent?.value != nil - } catch { - return false - } - } - } } diff --git a/Tests/SourceKitDTests/SourceKitDTests.swift b/Tests/SourceKitDTests/SourceKitDTests.swift index 8a9650cd2..12688c151 100644 --- a/Tests/SourceKitDTests/SourceKitDTests.swift +++ b/Tests/SourceKitDTests/SourceKitDTests.swift @@ -84,7 +84,7 @@ final class SourceKitDTests: XCTestCase { keys.compilerArgs: args, ]) - _ = try await sourcekitd.send(req, timeout: defaultTimeoutDuration) + _ = try await sourcekitd.send(req, timeout: defaultTimeoutDuration, fileContents: nil) try await fulfillmentOfOrThrow(expectation1, expectation2) @@ -92,7 +92,7 @@ final class SourceKitDTests: XCTestCase { keys.request: sourcekitd.requests.editorClose, keys.name: path, ]) - _ = try await sourcekitd.send(close, timeout: defaultTimeoutDuration) + _ = try await sourcekitd.send(close, timeout: defaultTimeoutDuration, fileContents: nil) } } diff --git a/Tests/SourceKitLSPTests/ClangdTests.swift b/Tests/SourceKitLSPTests/ClangdTests.swift index 10d2aa2a9..8583e459f 100644 --- a/Tests/SourceKitLSPTests/ClangdTests.swift +++ b/Tests/SourceKitLSPTests/ClangdTests.swift @@ -11,12 +11,7 @@ //===----------------------------------------------------------------------===// import LanguageServerProtocol -import SKLogging -import SKOptions import SKTestSupport -import SourceKitLSP -import SwiftExtensions -import TSCBasic import XCTest final class ClangdTests: XCTestCase { @@ -132,58 +127,4 @@ final class ClangdTests: XCTestCase { } } } - - func testRestartClangdIfItDoesntReply() async throws { - // We simulate clangd not replying until it is restarted using a hook. - let clangdRestarted = AtomicBool(initialValue: false) - let clangdRestartedExpectation = self.expectation(description: "clangd restarted") - let hooks = Hooks(preForwardRequestToClangd: { request in - if !clangdRestarted.value { - try? await Task.sleep(for: .seconds(60 * 60)) - } - }) - - let testClient = try await TestSourceKitLSPClient( - options: SourceKitLSPOptions(semanticServiceRestartTimeout: 1), - hooks: hooks - ) - let uri = DocumentURI(for: .c) - let positions = testClient.openDocument( - """ - void test() { - int x1️⃣; - } - """, - uri: uri - ) - - // Monitor clangd to notice when it gets restarted - let clangdServer = try await unwrap( - testClient.server.languageService(for: uri, .c, in: unwrap(testClient.server.workspaceForDocument(uri: uri))) - ) - await clangdServer.addStateChangeHandler { oldState, newState in - if oldState == .connectionInterrupted, newState == .connected { - clangdRestarted.value = true - clangdRestartedExpectation.fulfill() - } - } - - // The first hover request should get cancelled by `semanticServiceRestartTimeout` - await assertThrowsError( - try await testClient.send(HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"])) - ) { error in - XCTAssert( - (error as? ResponseError)?.message.contains("Timed out") ?? false, - "Received unexpected error: \(error)" - ) - } - - try await fulfillmentOfOrThrow(clangdRestartedExpectation) - - // After clangd gets restarted - let hover = try await testClient.send( - HoverRequest(textDocument: TextDocumentIdentifier(uri), position: positions["1️⃣"]) - ) - assertContains(hover?.contents.markupContent?.value ?? "", "Type: int") - } } diff --git a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift index b99e69b36..17cd1c8f9 100644 --- a/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift +++ b/Tests/SwiftSourceKitPluginTests/SwiftSourceKitPluginTests.swift @@ -1830,7 +1830,7 @@ fileprivate extension SourceKitD { keys.syntacticOnly: 1, keys.compilerArgs: compilerArguments as [SKDRequestValue], ]) - _ = try await send(req, timeout: defaultTimeoutDuration) + _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) return DocumentPositions(markers: markers, textWithoutMarkers: textWithoutMarkers) } @@ -1844,7 +1844,7 @@ fileprivate extension SourceKitD { keys.syntacticOnly: 1, ]) - _ = try await send(req, timeout: defaultTimeoutDuration) + _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) } nonisolated func closeDocument(_ name: String) async throws { @@ -1853,7 +1853,7 @@ fileprivate extension SourceKitD { keys.name: name, ]) - _ = try await send(req, timeout: defaultTimeoutDuration) + _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) } nonisolated func completeImpl( @@ -1888,7 +1888,7 @@ fileprivate extension SourceKitD { keys.compilerArgs: compilerArguments as [SKDRequestValue]?, ]) - let res = try await send(req, timeout: defaultTimeoutDuration) + let res = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) return try CompletionResultSet(res) } @@ -1946,7 +1946,7 @@ fileprivate extension SourceKitD { keys.codeCompleteOptions: dictionary([keys.useNewAPI: 1]), ]) - _ = try await send(req, timeout: defaultTimeoutDuration) + _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) } nonisolated func completeDocumentation(id: Int) async throws -> CompletionDocumentation { @@ -1955,7 +1955,7 @@ fileprivate extension SourceKitD { keys.identifier: id, ]) - let resp = try await send(req, timeout: defaultTimeoutDuration) + let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) return CompletionDocumentation(resp) } @@ -1964,7 +1964,7 @@ fileprivate extension SourceKitD { keys.request: requests.codeCompleteDiagnostic, keys.identifier: id, ]) - let resp = try await send(req, timeout: defaultTimeoutDuration) + let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) return CompletionDiagnostic(resp) } @@ -1973,7 +1973,7 @@ fileprivate extension SourceKitD { let req = dictionary([ keys.request: requests.dependencyUpdated ]) - _ = try await send(req, timeout: defaultTimeoutDuration) + _ = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) } nonisolated func setPopularAPI(popular: [String], unpopular: [String]) async throws { @@ -1984,7 +1984,7 @@ fileprivate extension SourceKitD { keys.unpopular: unpopular as [SKDRequestValue], ]) - let resp = try await send(req, timeout: defaultTimeoutDuration) + let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) XCTAssertEqual(resp[keys.useNewAPI], 1) } @@ -2001,7 +2001,7 @@ fileprivate extension SourceKitD { keys.notoriousModules: notoriousModules as [SKDRequestValue], ]) - let resp = try await send(req, timeout: defaultTimeoutDuration) + let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) XCTAssertEqual(resp[keys.useNewAPI], 1) } @@ -2025,7 +2025,7 @@ fileprivate extension SourceKitD { keys.modulePopularity: modulePopularity as [SKDRequestValue], ]) - let resp = try await send(req, timeout: defaultTimeoutDuration) + let resp = try await send(req, timeout: defaultTimeoutDuration, fileContents: nil) XCTAssertEqual(resp[keys.useNewAPI], 1) } diff --git a/config.schema.json b/config.schema.json index 17a6d5965..2fb4b7ba7 100644 --- a/config.schema.json +++ b/config.schema.json @@ -181,11 +181,6 @@ }, "type" : "object" }, - "semanticServiceRestartTimeout" : { - "description" : "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.", - "markdownDescription" : "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.", - "type" : "number" - }, "sourcekitd" : { "description" : "Options modifying the behavior of sourcekitd.", "markdownDescription" : "Options modifying the behavior of sourcekitd.",