From 4d1fa7a7ee49cf954e3f2ee8a9635cb1dfaf0d56 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 9 Sep 2024 11:49:54 -0700 Subject: [PATCH] Fix a race condition that could cause the build graph to not be generated when doing initial background indexing We were making the initial `generateBuildGraph` call in `SourceKitLSPServer` from a `Task`. This means that `generateBuildGraph` could be executed after `waitForUpToDateBuildGraph` was called by `SemanticIndexManager`. Thus `waitForUpToDateBuildGraph` returned immediately and no files were background indexed. Make `generateBuildGraph` immediately schedule a package reload for SwiftPM build systems instead of doing the hop through a `Task`, fixing the race condition. rdar://135551812 --- .../BuildServerBuildSystem.swift | 2 +- .../BuildSystemIntegration/BuildSystem.swift | 6 ++-- .../BuildSystemManager.swift | 4 +-- .../CompilationDatabaseBuildSystem.swift | 2 +- .../SwiftPMBuildSystem.swift | 13 +++++---- Sources/SourceKitLSP/SourceKitLSPServer.swift | 12 ++++---- .../BuildSystemManagerTests.swift | 2 +- .../SwiftPMBuildSystemTests.swift | 28 +++++++++---------- .../SourceKitLSPTests/BuildSystemTests.swift | 2 +- 9 files changed, 36 insertions(+), 35 deletions(-) diff --git a/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift b/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift index fa435fd9..f84ce598 100644 --- a/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift +++ b/Sources/BuildSystemIntegration/BuildServerBuildSystem.swift @@ -281,7 +281,7 @@ extension BuildServerBuildSystem: BuildSystem { return [ConfiguredTarget(targetID: "dummy", runDestinationID: "dummy")] } - package func generateBuildGraph() {} + package func scheduleBuildGraphGeneration() {} package func waitForUpToDateBuildGraph() async {} diff --git a/Sources/BuildSystemIntegration/BuildSystem.swift b/Sources/BuildSystemIntegration/BuildSystem.swift index 014e665a..6c248e4d 100644 --- a/Sources/BuildSystemIntegration/BuildSystem.swift +++ b/Sources/BuildSystemIntegration/BuildSystem.swift @@ -137,8 +137,10 @@ package protocol BuildSystem: AnyObject, Sendable { /// Return the list of targets and run destinations that the given document can be built for. func configuredTargets(for document: DocumentURI) async -> [ConfiguredTarget] - /// Re-generate the build graph. - func generateBuildGraph() async throws + /// Schedule a task that re-generates the build graph. The function may return before the build graph has finished + /// being generated. If clients need to wait for an up-to-date build graph, they should call + /// `waitForUpToDateBuildGraph` afterwards. + func scheduleBuildGraphGeneration() async throws /// Wait until the build graph has been loaded. func waitForUpToDateBuildGraph() async diff --git a/Sources/BuildSystemIntegration/BuildSystemManager.swift b/Sources/BuildSystemIntegration/BuildSystemManager.swift index 16685bf5..02273db4 100644 --- a/Sources/BuildSystemIntegration/BuildSystemManager.swift +++ b/Sources/BuildSystemIntegration/BuildSystemManager.swift @@ -265,8 +265,8 @@ extension BuildSystemManager { return settings } - package func generateBuildGraph() async throws { - try await self.buildSystem?.generateBuildGraph() + package func scheduleBuildGraphGeneration() async throws { + try await self.buildSystem?.scheduleBuildGraphGeneration() } package func waitForUpToDateBuildGraph() async { diff --git a/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift b/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift index 7e337af2..1ee6b880 100644 --- a/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift +++ b/Sources/BuildSystemIntegration/CompilationDatabaseBuildSystem.swift @@ -133,7 +133,7 @@ extension CompilationDatabaseBuildSystem: BuildSystem { throw PrepareNotSupportedError() } - package func generateBuildGraph() {} + package func scheduleBuildGraphGeneration() {} package func waitForUpToDateBuildGraph() async {} diff --git a/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift b/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift index 02a70a0d..4a038fca 100644 --- a/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift +++ b/Sources/BuildSystemIntegration/SwiftPMBuildSystem.swift @@ -365,10 +365,11 @@ package actor SwiftPMBuildSystem { extension SwiftPMBuildSystem { /// (Re-)load the package settings by parsing the manifest and resolving all the targets and /// dependencies. - package func reloadPackage() async throws { - try await packageLoadingQueue.asyncThrowing { + @discardableResult + package func schedulePackageReload() -> Task { + return packageLoadingQueue.asyncThrowing { try await self.reloadPackageImpl() - }.valuePropagatingCancellation + } } /// - Important: Must only be called on `packageLoadingQueue`. @@ -549,8 +550,8 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuildSystem { return [] } - package func generateBuildGraph() async throws { - try await self.reloadPackage() + package func scheduleBuildGraphGeneration() async throws { + self.schedulePackageReload() } package func waitForUpToDateBuildGraph() async { @@ -743,7 +744,7 @@ extension SwiftPMBuildSystem: BuildSystemIntegration.BuildSystem { if events.contains(where: { self.fileEventShouldTriggerPackageReload(event: $0) }) { logger.log("Reloading package because of file change") await orLog("Reloading package") { - try await self.reloadPackage() + try await self.schedulePackageReload().value } } diff --git a/Sources/SourceKitLSP/SourceKitLSPServer.swift b/Sources/SourceKitLSP/SourceKitLSPServer.swift index e7695161..f330ac51 100644 --- a/Sources/SourceKitLSP/SourceKitLSPServer.swift +++ b/Sources/SourceKitLSP/SourceKitLSPServer.swift @@ -967,13 +967,11 @@ extension SourceKitLSPServer { guard await condition(buildSystem) else { return nil } - Task { - await orLog("Initial build graph generation") { - // Schedule an initial generation of the build graph. Once the build graph is loaded, the build system will send - // call `fileHandlingCapabilityChanged`, which allows us to move documents to a workspace with this build - // system. - try await buildSystem?.generateBuildGraph() - } + await orLog("Initial build graph generation") { + // Schedule an initial generation of the build graph. Once the build graph is loaded, the build system will send + // call `fileHandlingCapabilityChanged`, which allows us to move documents to a workspace with this build + // system. + try await buildSystem?.scheduleBuildGraphGeneration() } let projectRoot = await buildSystem?.projectRoot.pathString diff --git a/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift b/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift index c8e1a12f..547fd0c1 100644 --- a/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift +++ b/Tests/BuildSystemIntegrationTests/BuildSystemManagerTests.swift @@ -482,7 +482,7 @@ class ManualBuildSystem: BuildSystem { throw PrepareNotSupportedError() } - package func generateBuildGraph() {} + package func scheduleBuildGraphGeneration() {} package func waitForUpToDateBuildGraph() async {} diff --git a/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift b/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift index dfc2c59a..e9b22dcd 100644 --- a/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift +++ b/Tests/BuildSystemIntegrationTests/SwiftPMBuildSystemTests.swift @@ -83,7 +83,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - await assertThrowsError(try await buildSystem.generateBuildGraph()) + await assertThrowsError(try await buildSystem.schedulePackageReload().value) } } @@ -144,7 +144,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift = packageRoot.appending(components: "Sources", "lib", "a.swift") let hostTriple = await swiftpmBuildSystem.destinationBuildParameters.triple @@ -209,7 +209,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aPlusSomething = packageRoot.appending(components: "Sources", "lib", "a+something.swift") let hostTriple = await swiftpmBuildSystem.destinationBuildParameters.triple @@ -272,7 +272,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(swiftPM: options), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift = packageRoot.appending(components: "Sources", "lib", "a.swift") let hostTriple = await swiftpmBuildSystem.destinationBuildParameters.triple @@ -354,7 +354,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let source = try resolveSymlinks(packageRoot.appending(component: "Package.swift")) let arguments = try await unwrap(swiftpmBuildSystem.buildSettings(for: source.asURI, language: .swift)) @@ -391,7 +391,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift = packageRoot.appending(components: "Sources", "lib", "a.swift") let bswift = packageRoot.appending(components: "Sources", "lib", "b.swift") @@ -440,7 +440,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift = packageRoot.appending(components: "Sources", "libA", "a.swift") let bswift = packageRoot.appending(components: "Sources", "libB", "b.swift") @@ -495,7 +495,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift = packageRoot.appending(components: "Sources", "libA", "a.swift") let bswift = packageRoot.appending(components: "Sources", "libB", "b.swift") @@ -539,7 +539,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let acxx = packageRoot.appending(components: "Sources", "lib", "a.cpp") let bcxx = packageRoot.appending(components: "Sources", "lib", "b.cpp") @@ -620,7 +620,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift = packageRoot.appending(components: "Sources", "lib", "a.swift") let arguments = try await unwrap(swiftpmBuildSystem.buildSettings(for: aswift.asURI, language: .swift)) @@ -672,7 +672,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift1 = packageRoot.appending(components: "Sources", "lib", "a.swift") let aswift2 = @@ -740,7 +740,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value for file in [acpp, ah] { let args = try unwrap( @@ -780,7 +780,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift = packageRoot.appending(components: "Sources", "lib", "a.swift") let arguments = try await unwrap(swiftpmBuildSystem.buildSettings(for: aswift.asURI, language: .swift)) @@ -856,7 +856,7 @@ final class SwiftPMBuildSystemTests: XCTestCase { options: SourceKitLSPOptions(), testHooks: SwiftPMTestHooks() ) - try await swiftpmBuildSystem.generateBuildGraph() + try await swiftpmBuildSystem.schedulePackageReload().value let aswift = packageRoot.appending(components: "Plugins", "MyPlugin", "a.swift") let hostTriple = await swiftpmBuildSystem.destinationBuildParameters.triple diff --git a/Tests/SourceKitLSPTests/BuildSystemTests.swift b/Tests/SourceKitLSPTests/BuildSystemTests.swift index 2cf4db2d..95f3101f 100644 --- a/Tests/SourceKitLSPTests/BuildSystemTests.swift +++ b/Tests/SourceKitLSPTests/BuildSystemTests.swift @@ -73,7 +73,7 @@ actor TestBuildSystem: BuildSystem { throw PrepareNotSupportedError() } - func generateBuildGraph() {} + func scheduleBuildGraphGeneration() {} package func waitForUpToDateBuildGraph() async {}