averello/sourcekit-lsp
1
0
Fork 0
mirror of https://github.com/apple/sourcekit-lsp.git synced 2026-03-02 18:23:24 +01:00
Code Issues Packages Projects Releases Wiki Activity

Cancel preparation tasks for editor functionality if the preparation task hasn't been started yet and the document is no longer active

Browse Source 
When the user opens documents from three targets A, B, and C in quick succession, then we don’t want to schedule preparation of wait until A *and* B are finished preparing before preparing C.

Instead, we want to
- Finish for preparation of A to finish if it has already started by the time the file in C is opened. This is done so we always make progress during preparation and don’t get into a scenario where preparation is always cancelled if a user switches between two targets more quickly than it takes to prepare those targets.
- Not prepare B because it is no longer relevant and we haven’t started any progress here. Essentially, we pretend that the hop to B never happened.
This commit is contained in:
Alex Hoppen
2024-05-20 21:41:44 -07:00
parent 0e58ab1ec9
commit 41b810b80c
9 changed files with 258 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

59
Sources/SKCore/TaskScheduler.swift
View File

@@ -147,6 +147,9 @@ public actor QueuedTask<TaskDescription: TaskDescriptionProtocol> {
/// 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<TaskDescription: TaskDescriptionProtocol> {
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<TaskDescription: TaskDescriptionProtocol> {
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
}
}
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
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.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<TaskDescription: TaskDescriptionProtocol> {
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

1
Sources/SKSupport/LineTable.swift
View File

@@ -11,6 +11,7 @@
//===----------------------------------------------------------------------===//
import LSPLogging
#if canImport(os)
import os
#endif

3
Sources/SKSwiftPMWorkspace/SwiftPMBuildSystem.swift
View File

@@ -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 {

1
Sources/SemanticIndex/PreparationTaskDescription.swift
View File

@@ -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: {

51
Sources/SemanticIndex/SemanticIndexManager.swift
View File

@@ -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<Void, Never>)? = 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.

4
Sources/SemanticIndex/TestHooks.swift
View File

@@ -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
}

3
Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift
View File

@@ -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

8
Sources/SourceKitLSP/SourceKitLSPServer.swift
View File

@@ -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)

172
Tests/SourceKitLSPTests/BackgroundIndexingTests.swift
View File

@@ -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<ConfiguredTarget>]
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<ConfiguredTarget>]) {
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])
}
}