Skip to content

Commit c68fd5b

Browse files
tomerdtomerd
authored
fix concurrency issue with GithubMetadata provider (#3086)
motivation: fixes race condition changes: * remove redundant queue * change return/callback logic to update the result set and deal with errors in a non-racy way
1 parent 583b790 commit c68fd5b

File tree

1 file changed

+86
-79
lines changed

1 file changed

+86
-79
lines changed

‎Sources/PackageCollections/Providers/GitHubPackageMetadataProvider.swift

Lines changed: 86 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,12 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
2626
private let httpClient: HTTPClient
2727
private let diagnosticsEngine: DiagnosticsEngine?
2828
private let decoder: JSONDecoder
29-
private let queue: DispatchQueue
3029

3130
init(configuration: Configuration = .init(), httpClient: HTTPClient? = nil, diagnosticsEngine: DiagnosticsEngine? = nil) {
3231
self.configuration = configuration
3332
self.httpClient = httpClient ?? Self.makeDefaultHTTPClient(diagnosticsEngine: diagnosticsEngine)
3433
self.diagnosticsEngine = diagnosticsEngine
3534
self.decoder = JSONDecoder.makeWithDefaults()
36-
self.queue = DispatchQueue(label: "org.swift.swiftpm.GitHubPackageMetadataProvider", attributes: .concurrent)
3735
}
3836

3937
func get(_ reference: PackageReference, callback: @escaping (Result<Model.PackageBasicMetadata, Error>) -> Void) {
@@ -49,92 +47,101 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
4947
let contributorsURL = baseURL.appendingPathComponent("contributors")
5048
let readmeURL = baseURL.appendingPathComponent("readme")
5149

52-
self.queue.async {
53-
let sync = DispatchGroup()
54-
var results = [URL: Result<HTTPClientResponse, Error>]()
55-
let resultsLock = Lock()
56-
57-
// get the main data
58-
sync.enter()
59-
var metadataHeaders = self.makeRequestHeaders(metadataURL)
60-
metadataHeaders.add(name: "Accept", value: "application/vnd.github.mercy-preview+json")
61-
let metadataOptions = self.makeRequestOptions(validResponseCodes: [200, 401, 403, 404])
62-
httpClient.get(metadataURL, headers: metadataHeaders, options: metadataOptions) { result in
63-
defer { sync.leave() }
64-
resultsLock.withLock {
65-
results[metadataURL] = result
66-
}
67-
if case .success(let response) = result {
68-
let apiLimit = response.headers.get("X-RateLimit-Limit").first.flatMap(Int.init) ?? -1
69-
let apiRemaining = response.headers.get("X-RateLimit-Remaining").first.flatMap(Int.init) ?? -1
70-
71-
switch (response.statusCode, metadataHeaders.contains("Authorization"), apiRemaining) {
72-
case (_, _, 0):
73-
self.diagnosticsEngine?.emit(warning: "Exceeded API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
74-
return callback(.failure(Errors.apiLimitsExceeded(metadataURL, apiLimit)))
75-
case (401, true, _):
76-
return callback(.failure(Errors.invalidAuthToken(metadataURL)))
77-
case (401, false, _):
78-
return callback(.failure(Errors.permissionDenied(metadataURL)))
79-
case (403, _, _):
80-
return callback(.failure(Errors.permissionDenied(metadataURL)))
81-
case (404, _, _):
82-
return callback(.failure(NotFoundError("\(baseURL)")))
83-
case (200, _, _):
84-
if apiRemaining < self.configuration.apiLimitWarningThreshold {
85-
self.diagnosticsEngine?.emit(warning: "Approaching API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
86-
}
87-
// if successful, fan out multiple API calls
88-
[tagsURL, contributorsURL, readmeURL].forEach { url in
89-
sync.enter()
90-
var headers = self.makeRequestHeaders(url)
91-
headers.add(name: "Accept", value: "application/vnd.github.v3+json")
92-
let options = self.makeRequestOptions(validResponseCodes: [200])
93-
httpClient.get(url, headers: headers, options: options) { result in
94-
defer { sync.leave() }
95-
resultsLock.withLock {
96-
results[url] = result
97-
}
50+
let sync = DispatchGroup()
51+
var results = [URL: Result<HTTPClientResponse, Error>]()
52+
let resultsLock = Lock()
53+
54+
// get the main data
55+
sync.enter()
56+
var metadataHeaders = self.makeRequestHeaders(metadataURL)
57+
metadataHeaders.add(name: "Accept", value: "application/vnd.github.mercy-preview+json")
58+
let metadataOptions = self.makeRequestOptions(validResponseCodes: [200, 401, 403, 404])
59+
httpClient.get(metadataURL, headers: metadataHeaders, options: metadataOptions) { result in
60+
defer { sync.leave() }
61+
resultsLock.withLock {
62+
results[metadataURL] = result
63+
}
64+
if case .success(let response) = result {
65+
let apiLimit = response.headers.get("X-RateLimit-Limit").first.flatMap(Int.init) ?? -1
66+
let apiRemaining = response.headers.get("X-RateLimit-Remaining").first.flatMap(Int.init) ?? -1
67+
switch (response.statusCode, metadataHeaders.contains("Authorization"), apiRemaining) {
68+
case (_, _, 0):
69+
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+
}
73+
case (401, true, _):
74+
resultsLock.withLock {
75+
results[metadataURL] = .failure(Errors.invalidAuthToken(metadataURL))
76+
}
77+
case (401, false, _):
78+
resultsLock.withLock {
79+
results[metadataURL] = .failure(Errors.permissionDenied(metadataURL))
80+
}
81+
case (403, _, _):
82+
resultsLock.withLock {
83+
results[metadataURL] = .failure(Errors.permissionDenied(metadataURL))
84+
}
85+
case (404, _, _):
86+
resultsLock.withLock {
87+
results[metadataURL] = .failure(NotFoundError("\(baseURL)"))
88+
}
89+
case (200, _, _):
90+
if apiRemaining < self.configuration.apiLimitWarningThreshold {
91+
self.diagnosticsEngine?.emit(warning: "Approaching API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
92+
}
93+
// if successful, fan out multiple API calls
94+
[tagsURL, contributorsURL, readmeURL].forEach { url in
95+
sync.enter()
96+
var headers = self.makeRequestHeaders(url)
97+
headers.add(name: "Accept", value: "application/vnd.github.v3+json")
98+
let options = self.makeRequestOptions(validResponseCodes: [200])
99+
httpClient.get(url, headers: headers, options: options) { result in
100+
defer { sync.leave() }
101+
resultsLock.withLock {
102+
results[url] = result
98103
}
99104
}
100-
default:
101-
return callback(.failure(Errors.invalidResponse(metadataURL, "Invalid status code: \(response.statusCode)")))
105+
}
106+
default:
107+
resultsLock.withLock {
108+
results[metadataURL] = .failure(Errors.invalidResponse(metadataURL, "Invalid status code: \(response.statusCode)"))
102109
}
103110
}
104111
}
105-
sync.wait()
106-
107-
// process results
112+
}
113+
sync.wait()
108114

109-
do {
110-
// check for main request error state
111-
switch results[metadataURL] {
112-
case .none:
113-
throw Errors.invalidResponse(metadataURL, "Response missing")
114-
case .some(.failure(let error)):
115-
throw error
116-
case .some(.success(let metadataResponse)):
117-
guard let metadata = try metadataResponse.decodeBody(GetRepositoryResponse.self, using: self.decoder) else {
118-
throw Errors.invalidResponse(metadataURL, "Empty body")
119-
}
120-
let tags = try results[tagsURL]?.success?.decodeBody([Tag].self, using: self.decoder) ?? []
121-
let contributors = try results[contributorsURL]?.success?.decodeBody([Contributor].self, using: self.decoder)
122-
let readme = try results[readmeURL]?.success?.decodeBody(Readme.self, using: self.decoder)
115+
// process results
123116

124-
callback(.success(.init(
125-
summary: metadata.description,
126-
keywords: metadata.topics,
127-
// filters out non-semantic versioned tags
128-
versions: tags.compactMap { TSCUtility.Version(string: $0.name) },
129-
watchersCount: metadata.watchersCount,
130-
readmeURL: readme?.downloadURL,
131-
authors: contributors?.map { .init(username: $0.login, url: $0.url, service: .init(name: "GitHub")) },
132-
processedAt: Date()
133-
)))
117+
do {
118+
// check for main request error state
119+
switch results[metadataURL] {
120+
case .none:
121+
throw Errors.invalidResponse(metadataURL, "Response missing")
122+
case .some(.failure(let error)):
123+
throw error
124+
case .some(.success(let metadataResponse)):
125+
guard let metadata = try metadataResponse.decodeBody(GetRepositoryResponse.self, using: self.decoder) else {
126+
throw Errors.invalidResponse(metadataURL, "Empty body")
134127
}
135-
} catch {
136-
return callback(.failure(error))
128+
let tags = try results[tagsURL]?.success?.decodeBody([Tag].self, using: self.decoder) ?? []
129+
let contributors = try results[contributorsURL]?.success?.decodeBody([Contributor].self, using: self.decoder)
130+
let readme = try results[readmeURL]?.success?.decodeBody(Readme.self, using: self.decoder)
131+
132+
callback(.success(.init(
133+
summary: metadata.description,
134+
keywords: metadata.topics,
135+
// filters out non-semantic versioned tags
136+
versions: tags.compactMap { TSCUtility.Version(string: $0.name) },
137+
watchersCount: metadata.watchersCount,
138+
readmeURL: readme?.downloadURL,
139+
authors: contributors?.map { .init(username: $0.login, url: $0.url, service: .init(name: "GitHub")) },
140+
processedAt: Date()
141+
)))
137142
}
143+
} catch {
144+
return callback(.failure(error))
138145
}
139146
}
140147

0 commit comments

Comments
 (0)