diff --git a/Sources/SKCore/TaskScheduler.swift b/Sources/SKCore/TaskScheduler.swift index 23be76912..fe3637d5a 100644 --- a/Sources/SKCore/TaskScheduler.swift +++ b/Sources/SKCore/TaskScheduler.swift @@ -147,6 +147,9 @@ public actor QueuedTask { /// Gets reset every time `executionTask` finishes. nonisolated(unsafe) private var cancelledToBeRescheduled: AtomicBool = .init(initialValue: false) + /// Whether `resultTask` has been cancelled. + private nonisolated(unsafe) var resultTaskCancelled: AtomicBool = .init(initialValue: false) + private nonisolated(unsafe) var _isExecuting: AtomicBool = .init(initialValue: false) /// Whether the task is currently executing or still queued to be executed later. @@ -154,6 +157,10 @@ public actor QueuedTask { return _isExecuting.value } + public nonisolated func cancel() { + resultTask.cancel() + } + /// Wait for the task to finish. /// /// If the tasks that waits for this queued task to finished is cancelled, the QueuedTask will still continue @@ -197,31 +204,35 @@ public actor QueuedTask { self.executionTaskCreatedContinuation = executionTaskCreatedContinuation self.resultTask = Task.detached(priority: priority) { - await withTaskGroup(of: Void.self) { taskGroup in - taskGroup.addTask { - for await _ in updatePriorityStream { - self.priority = Task.currentPriority + await withTaskCancellationHandler { + await withTaskGroup(of: Void.self) { taskGroup in + taskGroup.addTask { + for await _ in updatePriorityStream { + self.priority = Task.currentPriority + } } - } - taskGroup.addTask { - for await task in executionTaskCreatedStream { - switch await task.value { - case .cancelledToBeRescheduled: - // Break the switch and wait for a new `executionTask` to be placed into `executionTaskCreatedStream`. - break - case .terminated: - // The task finished. We are done with this `QueuedTask` - return + taskGroup.addTask { + for await task in executionTaskCreatedStream { + switch await task.valuePropagatingCancellation { + case .cancelledToBeRescheduled: + // Break the switch and wait for a new `executionTask` to be placed into `executionTaskCreatedStream`. + break + case .terminated: + // The task finished. We are done with this `QueuedTask` + return + } } } + // The first (update priority) task never finishes, so this waits for the second (wait for execution) task + // to terminate. + // Afterwards we also cancel the update priority task. + for await _ in taskGroup { + taskGroup.cancelAll() + return + } } - // The first (update priority) task never finishes, so this waits for the second (wait for execution) task - // to terminate. - // Afterwards we also cancel the update priority task. - for await _ in taskGroup { - taskGroup.cancelAll() - return - } + } onCancel: { + self.resultTaskCancelled.value = true } } } @@ -233,7 +244,9 @@ public actor QueuedTask { func execute() async -> ExecutionTaskFinishStatus { precondition(executionTask == nil, "Task started twice") let task = Task.detached(priority: self.priority) { - await self.description.execute() + if !Task.isCancelled && !self.resultTaskCancelled.value { + await self.description.execute() + } return await self.finalizeExecution() } executionTask = task diff --git a/Sources/SKSupport/LineTable.swift b/Sources/SKSupport/LineTable.swift index e13488a43..9c57866de 100644 --- a/Sources/SKSupport/LineTable.swift +++ b/Sources/SKSupport/LineTable.swift @@ -11,6 +11,7 @@ //===----------------------------------------------------------------------===// import LSPLogging + #if canImport(os) import os #endif diff --git a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift index 95eeddfff..7b0844037 100644 --- a/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift +++ b/Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift @@ -489,6 +489,9 @@ extension SwiftPMBuildSystem: SKCore.BuildSystem { "--disable-index-store", "--target", target.targetID, ] + if Task.isCancelled { + return + } let process = try Process.launch(arguments: arguments, workingDirectory: nil) let result = try await process.waitUntilExitSendingSigIntOnTaskCancellation() switch result.exitStatus.exhaustivelySwitchable { diff --git a/Sources/SemanticIndex/PreparationTaskDescription.swift b/Sources/SemanticIndex/PreparationTaskDescription.swift index 049ce896c..8e7927e6b 100644 --- a/Sources/SemanticIndex/PreparationTaskDescription.swift +++ b/Sources/SemanticIndex/PreparationTaskDescription.swift @@ -70,6 +70,7 @@ public struct PreparationTaskDescription: IndexTaskDescription { // See comment in `withLoggingScope`. // The last 2 digits should be sufficient to differentiate between multiple concurrently running preparation operations await withLoggingScope("preparation-\(id % 100)") { + await testHooks.preparationTaskDidStart?(self) let targetsToPrepare = await targetsToPrepare.asyncFilter { await !preparationUpToDateStatus.isUpToDate($0) }.sorted(by: { diff --git a/Sources/SemanticIndex/SemanticIndexManager.swift b/Sources/SemanticIndex/SemanticIndexManager.swift index 6ce61adfc..1500ab3b1 100644 --- a/Sources/SemanticIndex/SemanticIndexManager.swift +++ b/Sources/SemanticIndex/SemanticIndexManager.swift @@ -86,6 +86,17 @@ public final actor SemanticIndexManager { /// After the file is indexed, it is removed from this dictionary. private var inProgressIndexTasks: [DocumentURI: InProgressIndexStore] = [:] + /// The currently running task that prepares a document for editor functionality. + /// + /// This is used so we can cancel preparation tasks for documents that the user is no longer interacting with and + /// avoid the following scenario: The user browses through documents from targets A, B, and C in quick succession. We + /// don't want stack preparation of A, B, and C. Instead we want to only prepare target C - and also finish + /// preparation of A if it has already started when the user opens C. + /// + /// `id` is a unique ID that identifies the preparation task and is used to set `inProgressPrepareForEditorTask` to + /// `nil` when the current in progress task finishes. + private var inProgressPrepareForEditorTask: (id: UUID, document: DocumentURI, task: Task)? = nil + /// The `TaskScheduler` that manages the scheduling of index tasks. This is shared among all `SemanticIndexManager`s /// in the process, to ensure that we don't schedule more index operations than processor cores from multiple /// workspaces. @@ -287,20 +298,39 @@ public final actor SemanticIndexManager { /// Schedule preparation of the target that contains the given URI, building all modules that the file depends on. /// /// This is intended to be called when the user is interacting with the document at the given URI. - public func schedulePreparation(of uri: DocumentURI, priority: TaskPriority? = nil) { - Task(priority: priority) { + public func schedulePreparationForEditorFunctionality( + of uri: DocumentURI, + priority: TaskPriority? = nil + ) { + if inProgressPrepareForEditorTask?.document == uri { + // We are already preparing this document, so nothing to do. This is necessary to avoid the following scenario: + // Determining the canonical configured target for a document takes 1s and we get a new document request for the + // document ever 0.5s, which would cancel the previous in-progress preparation task, cancelling the canonical + // configured target configuration, never actually getting to the actual preparation. + return + } + let id = UUID() + let task = Task(priority: priority) { await withLoggingScope("preparation") { guard let target = await buildSystemManager.canonicalConfiguredTarget(for: uri) else { return } + if Task.isCancelled { + return + } await self.prepare(targets: [target], priority: priority) + if inProgressPrepareForEditorTask?.id == id { + inProgressPrepareForEditorTask = nil + } } } + inProgressPrepareForEditorTask?.task.cancel() + inProgressPrepareForEditorTask = (id, uri, task) } // MARK: - Helper functions - /// Prepare the given targets for indexing + /// Prepare the given targets for indexing. private func prepare(targets: [ConfiguredTarget], priority: TaskPriority?) async { // Perform a quick initial check whether the target is up-to-date, in which case we don't need to schedule a // preparation operation at all. @@ -323,6 +353,9 @@ public final actor SemanticIndexManager { testHooks: testHooks ) ) + if Task.isCancelled { + return + } let preparationTask = await indexTaskScheduler.schedule(priority: priority, taskDescription) { task, newState in guard case .finished = newState else { return @@ -337,7 +370,17 @@ public final actor SemanticIndexManager { for target in targetsToPrepare { inProgressPreparationTasks[target] = OpaqueQueuedIndexTask(preparationTask) } - return await preparationTask.waitToFinishPropagatingCancellation() + await withTaskCancellationHandler { + return await preparationTask.waitToFinish() + } onCancel: { + // Only cancel the preparation task if it hasn't started executing yet. This ensures that we always make progress + // during preparation and can't get into the following scenario: The user has two target A and B that both take + // 10s to prepare. The user is now switching between the files every 5 seconds, which would always cause + // preparation for one target to get cancelled, never resulting in an up-to-date preparation status. + if !preparationTask.isExecuting { + preparationTask.cancel() + } + } } /// Update the index store for the given files, assuming that their targets have already been prepared. diff --git a/Sources/SemanticIndex/TestHooks.swift b/Sources/SemanticIndex/TestHooks.swift index c3de7326c..3e3730f7d 100644 --- a/Sources/SemanticIndex/TestHooks.swift +++ b/Sources/SemanticIndex/TestHooks.swift @@ -12,15 +12,19 @@ /// Callbacks that allow inspection of internal state modifications during testing. public struct IndexTestHooks: Sendable { + public var preparationTaskDidStart: (@Sendable (PreparationTaskDescription) async -> Void)? + public var preparationTaskDidFinish: (@Sendable (PreparationTaskDescription) async -> Void)? /// A callback that is called when an index task finishes. public var updateIndexStoreTaskDidFinish: (@Sendable (UpdateIndexStoreTaskDescription) async -> Void)? public init( + preparationTaskDidStart: (@Sendable (PreparationTaskDescription) async -> Void)? = nil, preparationTaskDidFinish: (@Sendable (PreparationTaskDescription) async -> Void)? = nil, updateIndexStoreTaskDidFinish: (@Sendable (UpdateIndexStoreTaskDescription) async -> Void)? = nil ) { + self.preparationTaskDidStart = preparationTaskDidStart self.preparationTaskDidFinish = preparationTaskDidFinish self.updateIndexStoreTaskDidFinish = updateIndexStoreTaskDidFinish } diff --git a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift index 3863a27ec..495fd9c2f 100644 --- a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift +++ b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift @@ -301,6 +301,9 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { processArguments: [String], workingDirectory: AbsolutePath? ) async throws { + if Task.isCancelled { + return + } let process = try Process.launch( arguments: processArguments, workingDirectory: workingDirectory diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index f5c679c57..210723007 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -1000,7 +1000,9 @@ extension SourceKitLSPServer: MessageHandler { // which prepares the files. For files that are open but aren't being worked on (eg. a different tab), we don't // get requests, ensuring that we don't unnecessarily prepare them. let workspace = await self.workspaceForDocument(uri: textDocumentRequest.textDocument.uri) - await workspace?.semanticIndexManager?.schedulePreparation(of: textDocumentRequest.textDocument.uri) + await workspace?.semanticIndexManager?.schedulePreparationForEditorFunctionality( + of: textDocumentRequest.textDocument.uri + ) } switch request { @@ -1543,7 +1545,7 @@ extension SourceKitLSPServer { ) return } - await workspace.semanticIndexManager?.schedulePreparation(of: uri) + await workspace.semanticIndexManager?.schedulePreparationForEditorFunctionality(of: uri) await openDocument(notification, workspace: workspace) } @@ -1599,7 +1601,7 @@ extension SourceKitLSPServer { ) return } - await workspace.semanticIndexManager?.schedulePreparation(of: uri) + await workspace.semanticIndexManager?.schedulePreparationForEditorFunctionality(of: uri) // If the document is ready, we can handle the change right now. documentManager.edit(notification) diff --git a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift index 8efb8b041..6427e53e1 100644 --- a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift +++ b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift @@ -22,31 +22,89 @@ fileprivate let backgroundIndexingOptions = SourceKitLSPServer.Options( indexOptions: IndexOptions(enableBackgroundIndexing: true) ) +fileprivate struct ExpectedPreparation { + let targetID: String + let runDestinationID: String + + /// A closure that will be executed when a preparation task starts. + /// This allows the artificial delay of a preparation task to force two preparation task to race. + let didStart: (() -> Void)? + + /// A closure that will be executed when a preparation task finishes. + /// This allows the artificial delay of a preparation task to force two preparation task to race. + let didFinish: (() -> Void)? + + internal init( + targetID: String, + runDestinationID: String, + didStart: (() -> Void)? = nil, + didFinish: (() -> Void)? = nil + ) { + self.targetID = targetID + self.runDestinationID = runDestinationID + self.didStart = didStart + self.didFinish = didFinish + } + + var configuredTarget: ConfiguredTarget { + return ConfiguredTarget(targetID: targetID, runDestinationID: runDestinationID) + } +} + fileprivate actor ExpectedPreparationTracker { /// The targets we expect to be prepared. For targets within the same set, we don't care about the exact order. - private var expectedPreparations: [Set] + private var expectedPreparations: [[ExpectedPreparation]] /// Implicitly-unwrapped optional so we can reference `self` when creating `IndexTestHooks`. /// `nonisolated(unsafe)` is fine because this is not modified after `testHooks` is created. nonisolated(unsafe) var testHooks: IndexTestHooks! - init(expectedPreparations: [Set]) { + init(expectedPreparations: [[ExpectedPreparation]]) { self.expectedPreparations = expectedPreparations - self.testHooks = IndexTestHooks(preparationTaskDidFinish: { [weak self] in - await self?.preparationTaskDidFinish(taskDescription: $0) - }) + self.testHooks = IndexTestHooks( + preparationTaskDidStart: { [weak self] in + await self?.preparationTaskDidStart(taskDescription: $0) + }, + preparationTaskDidFinish: { [weak self] in + await self?.preparationTaskDidFinish(taskDescription: $0) + } + ) + } + + func preparationTaskDidStart(taskDescription: PreparationTaskDescription) -> Void { + if Task.isCancelled { + return + } + guard let expectedTargetsToPrepare = expectedPreparations.first else { + return + } + for expectedPreparation in expectedTargetsToPrepare { + if taskDescription.targetsToPrepare.contains(expectedPreparation.configuredTarget) { + expectedPreparation.didStart?() + } + } } func preparationTaskDidFinish(taskDescription: PreparationTaskDescription) -> Void { + if Task.isCancelled { + return + } guard let expectedTargetsToPrepare = expectedPreparations.first else { - XCTFail("Didn't expect a preparation but received \(taskDescription)") + XCTFail("Didn't expect a preparation but received \(taskDescription.targetsToPrepare)") return } - guard Set(taskDescription.targetsToPrepare).isSubset(of: expectedTargetsToPrepare) else { - XCTFail("Received unexpected preparation of \(taskDescription)") + guard Set(taskDescription.targetsToPrepare).isSubset(of: expectedTargetsToPrepare.map(\.configuredTarget)) else { + XCTFail("Received unexpected preparation of \(taskDescription.targetsToPrepare)") return } - let remainingExpectedTargetsToPrepare = expectedTargetsToPrepare.subtracting(taskDescription.targetsToPrepare) + var remainingExpectedTargetsToPrepare: [ExpectedPreparation] = [] + for expectedPreparation in expectedTargetsToPrepare { + if taskDescription.targetsToPrepare.contains(expectedPreparation.configuredTarget) { + expectedPreparation.didFinish?() + } else { + remainingExpectedTargetsToPrepare.append(expectedPreparation) + } + } if remainingExpectedTargetsToPrepare.isEmpty { expectedPreparations.remove(at: 0) } else { @@ -54,6 +112,10 @@ fileprivate actor ExpectedPreparationTracker { } } + nonisolated func keepAlive() { + withExtendedLifetime(self) { _ in } + } + deinit { let expectedPreparations = self.expectedPreparations XCTAssert( @@ -549,16 +611,16 @@ final class BackgroundIndexingTests: XCTestCase { ) } - func testPrepareTarget() async throws { + func testPrepareTargetAfterEditToDependency() async throws { try await SkipUnless.swiftpmStoresModulesInSubdirectory() var serverOptions = backgroundIndexingOptions let expectedPreparationTracker = ExpectedPreparationTracker(expectedPreparations: [ [ - ConfiguredTarget(targetID: "LibA", runDestinationID: "dummy"), - ConfiguredTarget(targetID: "LibB", runDestinationID: "dummy"), + ExpectedPreparation(targetID: "LibA", runDestinationID: "dummy"), + ExpectedPreparation(targetID: "LibB", runDestinationID: "dummy"), ], [ - ConfiguredTarget(targetID: "LibB", runDestinationID: "dummy") + ExpectedPreparation(targetID: "LibB", runDestinationID: "dummy") ], ]) serverOptions.indexTestHooks = expectedPreparationTracker.testHooks @@ -587,7 +649,7 @@ final class BackgroundIndexingTests: XCTestCase { ) """, serverOptions: serverOptions, - cleanUp: { /* Keep expectedPreparationTracker alive */ _ = expectedPreparationTracker } + cleanUp: { expectedPreparationTracker.keepAlive() } ) let (uri, _) = try project.openDocument("MyOtherFile.swift") @@ -636,4 +698,86 @@ final class BackgroundIndexingTests: XCTestCase { try await fulfillmentOfOrThrow([receivedEmptyDiagnostics]) } + + func testDontStackTargetPreparationForEditorFunctionality() async throws { + let allDocumentsOpened = self.expectation(description: "All documents opened") + let libBStartedPreparation = self.expectation(description: "LibB started preparing") + let libDPreparedForEditing = self.expectation(description: "LibD prepared for editing") + + try await SkipUnless.swiftpmStoresModulesInSubdirectory() + var serverOptions = backgroundIndexingOptions + let expectedPreparationTracker = ExpectedPreparationTracker(expectedPreparations: [ + // Preparation of targets during the initial of the target + [ + ExpectedPreparation(targetID: "LibA", runDestinationID: "dummy"), + ExpectedPreparation(targetID: "LibB", runDestinationID: "dummy"), + ExpectedPreparation(targetID: "LibC", runDestinationID: "dummy"), + ExpectedPreparation(targetID: "LibD", runDestinationID: "dummy"), + ], + // LibB's preparation has already started by the time we browse through the other files, so we finish its preparation + [ + ExpectedPreparation( + targetID: "LibB", + runDestinationID: "dummy", + didStart: { libBStartedPreparation.fulfill() }, + didFinish: { self.wait(for: [allDocumentsOpened], timeout: defaultTimeout) } + ) + ], + // And now we just want to prepare LibD, and not LibC + [ + ExpectedPreparation( + targetID: "LibD", + runDestinationID: "dummy", + didFinish: { libDPreparedForEditing.fulfill() } + ) + ], + ]) + serverOptions.indexTestHooks = expectedPreparationTracker.testHooks + + let project = try await SwiftPMTestProject( + files: [ + "LibA/LibA.swift": "", + "LibB/LibB.swift": "", + "LibC/LibC.swift": "", + "LibD/LibD.swift": "", + ], + manifest: """ + // swift-tools-version: 5.7 + + import PackageDescription + + let package = Package( + name: "MyLibrary", + targets: [ + .target(name: "LibA"), + .target(name: "LibB", dependencies: ["LibA"]), + .target(name: "LibC", dependencies: ["LibA"]), + .target(name: "LibD", dependencies: ["LibA"]), + ] + ) + """, + serverOptions: serverOptions, + cleanUp: { expectedPreparationTracker.keepAlive() } + ) + + // Clean the preparation status of all libraries + project.testClient.send( + DidChangeWatchedFilesNotification(changes: [FileEvent(uri: try project.uri(for: "LibA.swift"), type: .changed)]) + ) + + // Quickly flip through all files + _ = try project.openDocument("LibB.swift") + try await self.fulfillmentOfOrThrow([libBStartedPreparation]) + + _ = try project.openDocument("LibC.swift") + + // Ensure that LibC gets opened before LibD, so that LibD is the latest document. Two open requests don't have + // dependencies between each other, so SourceKit-LSP is free to execute them in parallel or re-order them without + // the barrier. + _ = try await project.testClient.send(BarrierRequest()) + _ = try project.openDocument("LibD.swift") + + allDocumentsOpened.fulfill() + try await self.fulfillmentOfOrThrow([libDPreparedForEditing]) + } }