Skip to content

Commit dcd72bf

Browse files
tomerdtomerd
committed
improve manifest loader concurrency control (#4178)
motivation: more correct and robust concurrency control for manifest loading changes: * use a separate queue to schedule the evaluation tasks to ensure concurrency control is done correctly * move concurrency control to the methods that forks the child processes so that its more targeted * clarify what is the delegate queue and what is the callback queue * move a handful of data structure defintions around to make code easier to read * make concurrency tests more challenging
1 parent bd87e99 commit dcd72bf

10 files changed

+483
-445
lines changed

‎Sources/PackageLoading/ManifestLoader.swift

Lines changed: 283 additions & 224 deletions
Large diffs are not rendered by default.

‎Sources/SPMTestSupport/MockManifestLoader.swift

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,11 @@ public final class MockManifestLoader: ManifestLoaderProtocol {
5757
identityResolver: IdentityResolver,
5858
fileSystem: FileSystem,
5959
observabilityScope: ObservabilityScope,
60-
on queue: DispatchQueue,
60+
delegateQueue: DispatchQueue,
61+
callbackQueue: DispatchQueue,
6162
completion: @escaping (Result<Manifest, Error>) -> Void
6263
) {
63-
queue.async {
64+
callbackQueue.async {
6465
let key = Key(url: packageLocation, version: version)
6566
if let result = self.manifests[key] {
6667
return completion(.success(result))
@@ -115,7 +116,8 @@ extension ManifestLoader {
115116
identityResolver: identityResolver,
116117
fileSystem: fileSystem,
117118
observabilityScope: observabilityScope,
118-
on: .sharedConcurrent,
119+
delegateQueue: .sharedConcurrent,
120+
callbackQueue: .sharedConcurrent,
119121
completion: $0
120122
)
121123
}

‎Sources/Workspace/FileSystemPackageContainer.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,8 @@ public struct FileSystemPackageContainer: PackageContainer {
9191
identityResolver: self.identityResolver,
9292
fileSystem: self.fileSystem,
9393
observabilityScope: self.observabilityScope,
94-
on: .sharedConcurrent,
94+
delegateQueue: .sharedConcurrent,
95+
callbackQueue: .sharedConcurrent,
9596
completion: $0
9697
)
9798
}

‎Sources/Workspace/RegistryPackageContainer.swift

Lines changed: 76 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -120,87 +120,89 @@ public class RegistryPackageContainer: PackageContainer {
120120
}
121121
}
122122
}
123-
123+
124124
private func loadManifest(version: Version, completion: @escaping (Result<Manifest, Error>) -> Void) {
125125
self.getAvailableManifestsFilesystem(version: version) { result in
126-
switch result {
127-
case .failure(let error):
128-
return completion(.failure(error))
129-
case .success(let result):
130-
do {
131-
let manifests = result.manifests
132-
let fileSystem = result.fileSystem
133-
134-
// first, decide the tools-version we should use
135-
let preferredToolsVersion = try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
136-
// validate preferred the tools version is compatible with the current toolchain
137-
try preferredToolsVersion.validateToolsVersion(
138-
self.currentToolsVersion,
139-
packageIdentity: self.package.identity
126+
switch result {
127+
case .failure(let error):
128+
return completion(.failure(error))
129+
case .success(let result):
130+
do {
131+
let manifests = result.manifests
132+
let fileSystem = result.fileSystem
133+
134+
// first, decide the tools-version we should use
135+
let preferredToolsVersion = try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
136+
// validate preferred the tools version is compatible with the current toolchain
137+
try preferredToolsVersion.validateToolsVersion(
138+
self.currentToolsVersion,
139+
packageIdentity: self.package.identity
140+
)
141+
// load the manifest content
142+
guard let defaultManifestToolsVersion = manifests.first(where: { $0.key == Manifest.filename })?.value.toolsVersion else {
143+
throw StringError("Could not find the '\(Manifest.filename)' file for '\(self.package.identity)' '\(version)'")
144+
}
145+
if preferredToolsVersion == defaultManifestToolsVersion {
146+
// default tools version - we already have the content on disk from getAvailableManifestsFileSystem()
147+
self.manifestLoader.load(
148+
at: .root,
149+
packageIdentity: self.package.identity,
150+
packageKind: self.package.kind,
151+
packageLocation: self.package.locationString,
152+
version: version,
153+
revision: nil,
154+
toolsVersion: preferredToolsVersion,
155+
identityResolver: self.identityResolver,
156+
fileSystem: result.fileSystem,
157+
observabilityScope: self.observabilityScope,
158+
delegateQueue: .sharedConcurrent,
159+
callbackQueue: .sharedConcurrent,
160+
completion: completion
140161
)
141-
// load the manifest content
142-
guard let defaultManifestToolsVersion = manifests.first(where: { $0.key == Manifest.filename })?.value.toolsVersion else {
143-
throw StringError("Could not find the '\(Manifest.filename)' file for '\(self.package.identity)' '\(version)'")
144-
}
145-
if preferredToolsVersion == defaultManifestToolsVersion {
146-
// default tools version - we already have the content on disk from getAvailableManifestsFileSystem()
147-
self.manifestLoader.load(
148-
at: .root,
149-
packageIdentity: self.package.identity,
150-
packageKind: self.package.kind,
151-
packageLocation: self.package.locationString,
152-
version: version,
153-
revision: nil,
154-
toolsVersion: preferredToolsVersion,
155-
identityResolver: self.identityResolver,
156-
fileSystem: result.fileSystem,
157-
observabilityScope: self.observabilityScope,
158-
on: .sharedConcurrent,
159-
completion: completion
160-
)
161-
} else {
162-
// custom tools-version, we need to fetch the content from the server
163-
self.registryClient.getManifestContent(
164-
package: self.package.identity,
165-
version: version,
166-
customToolsVersion: preferredToolsVersion,
167-
observabilityScope: self.observabilityScope,
168-
callbackQueue: .sharedConcurrent
169-
) { result in
170-
switch result {
171-
case .failure(let error):
172-
return completion(.failure(error))
173-
case .success(let manifestContent):
174-
do {
175-
// replace the fake manifest with the real manifest content
176-
let manifestPath = AbsolutePath.root.appending(component: Manifest.basename + "@swift-\(preferredToolsVersion).swift")
177-
try fileSystem.removeFileTree(manifestPath)
178-
try fileSystem.writeFileContents(manifestPath, string: manifestContent)
179-
// finally, load the manifest
180-
self.manifestLoader.load(
181-
at: .root,
182-
packageIdentity: self.package.identity,
183-
packageKind: self.package.kind,
184-
packageLocation: self.package.locationString,
185-
version: version,
186-
revision: nil,
187-
toolsVersion: preferredToolsVersion,
188-
identityResolver: self.identityResolver,
189-
fileSystem: fileSystem,
190-
observabilityScope: self.observabilityScope,
191-
on: .sharedConcurrent,
192-
completion: completion
193-
)
194-
} catch {
195-
return completion(.failure(error))
196-
}
197-
}
162+
} else {
163+
// custom tools-version, we need to fetch the content from the server
164+
self.registryClient.getManifestContent(
165+
package: self.package.identity,
166+
version: version,
167+
customToolsVersion: preferredToolsVersion,
168+
observabilityScope: self.observabilityScope,
169+
callbackQueue: .sharedConcurrent
170+
) { result in
171+
switch result {
172+
case .failure(let error):
173+
return completion(.failure(error))
174+
case .success(let manifestContent):
175+
do {
176+
// replace the fake manifest with the real manifest content
177+
let manifestPath = AbsolutePath.root.appending(component: Manifest.basename + "@swift-\(preferredToolsVersion).swift")
178+
try fileSystem.removeFileTree(manifestPath)
179+
try fileSystem.writeFileContents(manifestPath, string: manifestContent)
180+
// finally, load the manifest
181+
self.manifestLoader.load(
182+
at: .root,
183+
packageIdentity: self.package.identity,
184+
packageKind: self.package.kind,
185+
packageLocation: self.package.locationString,
186+
version: version,
187+
revision: nil,
188+
toolsVersion: preferredToolsVersion,
189+
identityResolver: self.identityResolver,
190+
fileSystem: fileSystem,
191+
observabilityScope: self.observabilityScope,
192+
delegateQueue: .sharedConcurrent,
193+
callbackQueue: .sharedConcurrent,
194+
completion: completion
195+
)
196+
} catch {
197+
return completion(.failure(error))
198+
}
198199
}
199200
}
200-
} catch {
201-
return completion(.failure(error))
202201
}
202+
} catch {
203+
return completion(.failure(error))
203204
}
205+
}
204206
}
205207
}
206208

‎Sources/Workspace/SourceControlPackageContainer.swift

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -397,18 +397,21 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
397397
// Load the manifest.
398398
// FIXME: this should not block
399399
return try temp_await {
400-
self.manifestLoader.load(at: AbsolutePath.root,
401-
packageIdentity: self.package.identity,
402-
packageKind: self.package.kind,
403-
packageLocation: self.package.locationString,
404-
version: version,
405-
revision: nil,
406-
toolsVersion: toolsVersion,
407-
identityResolver: self.identityResolver,
408-
fileSystem: fileSystem,
409-
observabilityScope: self.observabilityScope,
410-
on: .sharedConcurrent,
411-
completion: $0)
400+
self.manifestLoader.load(
401+
at: AbsolutePath.root,
402+
packageIdentity: self.package.identity,
403+
packageKind: self.package.kind,
404+
packageLocation: self.package.locationString,
405+
version: version,
406+
revision: nil,
407+
toolsVersion: toolsVersion,
408+
identityResolver: self.identityResolver,
409+
fileSystem: fileSystem,
410+
observabilityScope: self.observabilityScope,
411+
delegateQueue: .sharedConcurrent,
412+
callbackQueue: .sharedConcurrent,
413+
completion: $0
414+
)
412415
}
413416
}
414417

0 commit comments

Comments
 (0)