Skip to content

Commit b2342f1

Browse files
tomerdtomerd
authored
fix thread safety issue in package collections (#3136)
motivation: a failing test hinted at a thread safety issue, which was then diagnosed to be an incorrect lock on an array. this prompted creating a thread safe utility for array to avoid such cases changes: * create a thread-safe utlitiy for array * replace array + lock with the new utility * add test
1 parent b83834e commit b2342f1

File tree

7 files changed

+135
-78
lines changed

7 files changed

+135
-78
lines changed

‎Sources/Basics/ConcurrencyHelpers.swift

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,64 @@ public final class ThreadSafeKeyValueStore<Key, Value> where Key: Hashable {
6767
}
6868
}
6969

70+
/// Thread-safe array like structure
71+
public final class ThreadSafeArrayStore<Value> {
72+
private var underlying: [Value]
73+
private let lock = Lock()
74+
75+
public init(_ seed: [Value] = []) {
76+
self.underlying = seed
77+
}
78+
79+
public subscript(index: Int) -> Value? {
80+
self.lock.withLock {
81+
self.underlying[index]
82+
}
83+
}
84+
85+
public func get() -> [Value] {
86+
self.lock.withLock {
87+
self.underlying
88+
}
89+
}
90+
91+
public func clear() {
92+
self.lock.withLock {
93+
self.underlying = []
94+
}
95+
}
96+
97+
public func append(_ item: Value) {
98+
self.lock.withLock {
99+
self.underlying.append(item)
100+
}
101+
}
102+
103+
public var count: Int {
104+
self.lock.withLock {
105+
self.underlying.count
106+
}
107+
}
108+
109+
public var isEmpty: Bool {
110+
self.lock.withLock {
111+
self.underlying.isEmpty
112+
}
113+
}
114+
115+
public func map<NewValue>(_ transform: (Value) -> NewValue) -> [NewValue] {
116+
self.lock.withLock {
117+
self.underlying.map(transform)
118+
}
119+
}
120+
121+
public func compactMap<NewValue>(_ transform: (Value) throws -> NewValue?) rethrows -> [NewValue] {
122+
try self.lock.withLock {
123+
try self.underlying.compactMap(transform)
124+
}
125+
}
126+
}
127+
70128
/// Thread-safe value boxing structure
71129
public final class ThreadSafeBox<Value> {
72130
private var underlying: Value?

‎Sources/PackageCollections/JSONModel/JSONCollection+v1.swift

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -248,14 +248,14 @@ extension JSONPackageCollectionModel.V1 {
248248
extension JSONPackageCollectionModel.V1 {
249249
public struct Validator {
250250
public let configuration: Configuration
251-
251+
252252
public init(configuration: Configuration = .init()) {
253253
self.configuration = configuration
254254
}
255-
255+
256256
public func validate(collection: Collection) -> [ValidationMessage]? {
257257
var messages = [ValidationMessage]()
258-
258+
259259
let packages = collection.packages
260260
// Stop validating if collection doesn't pass basic checks
261261
if packages.isEmpty {
@@ -265,24 +265,24 @@ extension JSONPackageCollectionModel.V1 {
265265
} else {
266266
packages.forEach { self.validate(package: $0, messages: &messages) }
267267
}
268-
268+
269269
guard messages.isEmpty else {
270270
return messages
271271
}
272-
272+
273273
return nil
274274
}
275-
275+
276276
// TODO: validate package url?
277277
private func validate(package: Collection.Package, messages: inout [ValidationMessage]) {
278278
let packageID = PackageIdentity(url: package.url.absoluteString).description
279-
279+
280280
// Check for duplicate versions
281281
let nonUniqueVersions = Dictionary(grouping: package.versions, by: { $0.version }).filter { $1.count > 1 }.keys
282282
if !nonUniqueVersions.isEmpty {
283283
messages.append(.error("Duplicate version(s) found in package \(packageID): \(nonUniqueVersions).", property: "package.versions"))
284284
}
285-
285+
286286
var nonSemanticVersions = [String]()
287287
let semanticVersions: [TSCUtility.Version] = package.versions.compactMap {
288288
let semver = TSCUtility.Version(string: $0.version)
@@ -291,15 +291,15 @@ extension JSONPackageCollectionModel.V1 {
291291
}
292292
return semver
293293
}
294-
294+
295295
guard nonSemanticVersions.isEmpty else {
296296
messages.append(.error("Non semantic version(s) found in package \(packageID): \(nonSemanticVersions).", property: "package.versions"))
297297
// The next part of validation requires sorting the semvers. Cannot continue if non-semver.
298298
return
299299
}
300-
300+
301301
let sortedVersions = semanticVersions.sorted(by: >)
302-
302+
303303
var currentMajor: Int?
304304
var majorCount = 0
305305
var minorCount = 0
@@ -322,7 +322,7 @@ extension JSONPackageCollectionModel.V1 {
322322

323323
minorCount += 1
324324
}
325-
325+
326326
package.versions.forEach { version in
327327
if version.products.isEmpty {
328328
messages.append(.error("Package \(packageID) version \(version.version) does not contain any products.", property: "version.products"))
@@ -332,13 +332,13 @@ extension JSONPackageCollectionModel.V1 {
332332
messages.append(.error("Product \(product.name) of package \(packageID) version \(version.version) does not contain any targets.", property: "product.targets"))
333333
}
334334
}
335-
335+
336336
if version.targets.isEmpty {
337337
messages.append(.error("Package \(packageID) version \(version.version) does not contain any targets.", property: "version.targets"))
338338
}
339339
}
340340
}
341-
341+
342342
public struct Configuration {
343343
public var maximumPackageCount: Int
344344
public var maximumMajorVersionCount: Int

‎Sources/PackageCollections/PackageCollections+Validation.swift

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,26 +40,26 @@ public struct ValidationMessage: Equatable, CustomStringConvertible {
4040
public let message: String
4141
public let level: Level
4242
public let property: String?
43-
43+
4444
private init(_ message: String, level: Level, property: String? = nil) {
4545
self.message = message
4646
self.level = level
4747
self.property = property
4848
}
49-
49+
5050
static func error(_ message: String, property: String? = nil) -> ValidationMessage {
5151
.init(message, level: .error, property: property)
5252
}
53-
53+
5454
static func warning(_ message: String, property: String? = nil) -> ValidationMessage {
5555
.init(message, level: .warning, property: property)
5656
}
57-
57+
5858
public enum Level: String, Equatable {
5959
case warning
6060
case error
6161
}
62-
62+
6363
public var description: String {
6464
"[\(self.level)] \(self.property.map { "\($0): " } ?? "")\(self.message)"
6565
}
@@ -68,9 +68,9 @@ public struct ValidationMessage: Equatable, CustomStringConvertible {
6868
extension Array where Element == ValidationMessage {
6969
func errors(include levels: Set<ValidationMessage.Level> = [.error]) -> [ValidationError]? {
7070
let errors = self.filter { levels.contains($0.level) }
71-
71+
7272
guard !errors.isEmpty else { return nil }
73-
73+
7474
return errors.map {
7575
if let property = $0.property {
7676
return ValidationError.property(name: property, message: $0.message)

‎Sources/PackageCollections/PackageCollections.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
99
*/
1010

11+
import Basics
1112
import PackageModel
1213
import TSCBasic
1314

@@ -100,12 +101,11 @@ public struct PackageCollections: PackageCollectionsProtocol {
100101
if sources.isEmpty {
101102
return callback(.success([]))
102103
}
103-
let lock = Lock()
104-
var refreshResults = [Result<Model.Collection, Error>]()
104+
let refreshResults = ThreadSafeArrayStore<Result<Model.Collection, Error>>()
105105
sources.forEach { source in
106106
self.refreshCollectionFromSource(source: source) { refreshResult in
107-
lock.withLock { refreshResults.append(refreshResult) }
108-
if refreshResults.count == (lock.withLock { sources.count }) {
107+
refreshResults.append(refreshResult)
108+
if refreshResults.count == sources.count {
109109
let errors = refreshResults.compactMap { $0.failure }
110110
callback(errors.isEmpty ? .success(sources) : .failure(MultipleErrors(errors)))
111111
}

‎Sources/PackageCollections/Providers/GitHubPackageMetadataProvider.swift

Lines changed: 9 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,7 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
4848
let readmeURL = baseURL.appendingPathComponent("readme")
4949

5050
let sync = DispatchGroup()
51-
var results = [URL: Result<HTTPClientResponse, Error>]()
52-
let resultsLock = Lock()
51+
let results = ThreadSafeKeyValueStore<URL, Result<HTTPClientResponse, Error>>()
5352

5453
// get the main data
5554
sync.enter()
@@ -58,34 +57,22 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
5857
let metadataOptions = self.makeRequestOptions(validResponseCodes: [200, 401, 403, 404])
5958
httpClient.get(metadataURL, headers: metadataHeaders, options: metadataOptions) { result in
6059
defer { sync.leave() }
61-
resultsLock.withLock {
62-
results[metadataURL] = result
63-
}
60+
results[metadataURL] = result
6461
if case .success(let response) = result {
6562
let apiLimit = response.headers.get("X-RateLimit-Limit").first.flatMap(Int.init) ?? -1
6663
let apiRemaining = response.headers.get("X-RateLimit-Remaining").first.flatMap(Int.init) ?? -1
6764
switch (response.statusCode, metadataHeaders.contains("Authorization"), apiRemaining) {
6865
case (_, _, 0):
6966
self.diagnosticsEngine?.emit(warning: "Exceeded API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
70-
resultsLock.withLock {
71-
results[metadataURL] = .failure(Errors.apiLimitsExceeded(metadataURL, apiLimit))
72-
}
67+
results[metadataURL] = .failure(Errors.apiLimitsExceeded(metadataURL, apiLimit))
7368
case (401, true, _):
74-
resultsLock.withLock {
75-
results[metadataURL] = .failure(Errors.invalidAuthToken(metadataURL))
76-
}
69+
results[metadataURL] = .failure(Errors.invalidAuthToken(metadataURL))
7770
case (401, false, _):
78-
resultsLock.withLock {
79-
results[metadataURL] = .failure(Errors.permissionDenied(metadataURL))
80-
}
71+
results[metadataURL] = .failure(Errors.permissionDenied(metadataURL))
8172
case (403, _, _):
82-
resultsLock.withLock {
83-
results[metadataURL] = .failure(Errors.permissionDenied(metadataURL))
84-
}
73+
results[metadataURL] = .failure(Errors.permissionDenied(metadataURL))
8574
case (404, _, _):
86-
resultsLock.withLock {
87-
results[metadataURL] = .failure(NotFoundError("\(baseURL)"))
88-
}
75+
results[metadataURL] = .failure(NotFoundError("\(baseURL)"))
8976
case (200, _, _):
9077
if apiRemaining < self.configuration.apiLimitWarningThreshold {
9178
self.diagnosticsEngine?.emit(warning: "Approaching API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
@@ -98,15 +85,11 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
9885
let options = self.makeRequestOptions(validResponseCodes: [200])
9986
self.httpClient.get(url, headers: headers, options: options) { result in
10087
defer { sync.leave() }
101-
resultsLock.withLock {
102-
results[url] = result
103-
}
88+
results[url] = result
10489
}
10590
}
10691
default:
107-
resultsLock.withLock {
108-
results[metadataURL] = .failure(Errors.invalidResponse(metadataURL, "Invalid status code: \(response.statusCode)"))
109-
}
92+
results[metadataURL] = .failure(Errors.invalidResponse(metadataURL, "Invalid status code: \(response.statusCode)"))
11093
}
11194
}
11295
}

‎Sources/PackageCollections/Storage/SQLitePackageCollectionsStorage.swift

Lines changed: 12 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,7 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
3434
private var state = State.idle
3535
private let stateLock = Lock()
3636

37-
private var cache = [Model.CollectionIdentifier: Model.Collection]()
38-
private let cacheLock = Lock()
37+
private let cache = ThreadSafeKeyValueStore<Model.CollectionIdentifier, Model.Collection>()
3938

4039
init(location: SQLite.Location? = nil, diagnosticsEngine: DiagnosticsEngine? = nil) {
4140
self.location = location ?? .path(localFileSystem.swiftPMCacheDirectory.appending(components: "package-collection.db"))
@@ -86,9 +85,7 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
8685
try statement.step()
8786
}
8887
// write to cache
89-
self.cacheLock.withLock {
90-
self.cache[collection.identifier] = collection
91-
}
88+
self.cache[collection.identifier] = collection
9289
callback(.success(collection))
9390
} catch {
9491
callback(.failure(error))
@@ -110,9 +107,7 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
110107
try statement.step()
111108
}
112109
// write to cache
113-
self.cacheLock.withLock {
114-
self.cache[identifier] = nil
115-
}
110+
self.cache[identifier] = nil
116111
callback(.success(()))
117112
} catch {
118113
callback(.failure(error))
@@ -123,7 +118,7 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
123118
func get(identifier: Model.CollectionIdentifier,
124119
callback: @escaping (Result<Model.Collection, Error>) -> Void) {
125120
// try read to cache
126-
if let collection = (self.cacheLock.withLock { self.cache[identifier] }) {
121+
if let collection = self.cache[identifier] {
127122
return callback(.success(collection))
128123
}
129124

@@ -152,11 +147,7 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
152147
func list(identifiers: [Model.CollectionIdentifier]? = nil,
153148
callback: @escaping (Result<[Model.Collection], Error>) -> Void) {
154149
// try read to cache
155-
let cached = self.cacheLock.withLock {
156-
identifiers?.compactMap { identifier in
157-
self.cache[identifier]
158-
}
159-
}
150+
let cached = identifiers?.compactMap { self.cache[$0] }
160151
if let cached = cached, cached.count > 0, cached.count == identifiers?.count {
161152
return callback(.success(cached))
162153
}
@@ -190,20 +181,17 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
190181
// decoding is a performance bottleneck (10+s for 1000 collections)
191182
// workaround is to decode in parallel if list is large enough to justify it
192183
let sync = DispatchGroup()
193-
var collections: [Model.Collection]
184+
let collections: ThreadSafeArrayStore<Model.Collection>
194185
if blobs.count < Self.batchSize {
195-
collections = blobs.compactMap { data -> Model.Collection? in
186+
collections = .init(blobs.compactMap { data -> Model.Collection? in
196187
try? self.decoder.decode(Model.Collection.self, from: data)
197-
}
188+
})
198189
} else {
199-
let lock = Lock()
200-
collections = [Model.Collection]()
190+
collections = .init()
201191
blobs.forEach { data in
202192
self.queue.async(group: sync) {
203193
if let collection = try? self.decoder.decode(Model.Collection.self, from: data) {
204-
lock.withLock {
205-
collections.append(collection)
206-
}
194+
collections.append(collection)
207195
}
208196
}
209197
}
@@ -213,7 +201,7 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
213201
if collections.count != blobs.count {
214202
self.diagnosticsEngine?.emit(warning: "Some stored collections could not be deserialized. Please refresh the collections to resolve this issue.")
215203
}
216-
callback(.success(collections))
204+
callback(.success(collections.get()))
217205
}
218206

219207
} catch {
@@ -370,9 +358,7 @@ final class SQLitePackageCollectionsStorage: PackageCollectionsStorage, Closable
370358

371359
// for testing
372360
internal func resetCache() {
373-
self.cacheLock.withLock {
374-
self.cache = [:]
375-
}
361+
self.cache.clear()
376362
}
377363

378364
// MARK: - Private

0 commit comments

Comments
 (0)