Skip to content

fix concurrency issue with GithubMetadata provider #3086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 2, 2020
Merged

fix concurrency issue with GithubMetadata provider #3086

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 86 additions & 79 deletions Sources/PackageCollections/Providers/GitHubPackageMetadataProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,12 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
private let httpClient: HTTPClient
private let diagnosticsEngine: DiagnosticsEngine?
private let decoder: JSONDecoder
private let queue: DispatchQueue

init(configuration: Configuration = .init(), httpClient: HTTPClient? = nil, diagnosticsEngine: DiagnosticsEngine? = nil) {
self.configuration = configuration
self.httpClient = httpClient ?? Self.makeDefaultHTTPClient(diagnosticsEngine: diagnosticsEngine)
self.diagnosticsEngine = diagnosticsEngine
self.decoder = JSONDecoder.makeWithDefaults()
self.queue = DispatchQueue(label: "org.swift.swiftpm.GitHubPackageMetadataProvider", attributes: .concurrent)
}

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

self.queue.async {
let sync = DispatchGroup()
var results = [URL: Result<HTTPClientResponse, Error>]()
let resultsLock = Lock()

// get the main data
sync.enter()
var metadataHeaders = self.makeRequestHeaders(metadataURL)
metadataHeaders.add(name: "Accept", value: "application/vnd.github.mercy-preview+json")
let metadataOptions = self.makeRequestOptions(validResponseCodes: [200, 401, 403, 404])
httpClient.get(metadataURL, headers: metadataHeaders, options: metadataOptions) { result in
defer { sync.leave() }
resultsLock.withLock {
results[metadataURL] = result
}
if case .success(let response) = result {
let apiLimit = response.headers.get("X-RateLimit-Limit").first.flatMap(Int.init) ?? -1
let apiRemaining = response.headers.get("X-RateLimit-Remaining").first.flatMap(Int.init) ?? -1

switch (response.statusCode, metadataHeaders.contains("Authorization"), apiRemaining) {
case (_, _, 0):
self.diagnosticsEngine?.emit(warning: "Exceeded API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
return callback(.failure(Errors.apiLimitsExceeded(metadataURL, apiLimit)))
case (401, true, _):
return callback(.failure(Errors.invalidAuthToken(metadataURL)))
case (401, false, _):
return callback(.failure(Errors.permissionDenied(metadataURL)))
case (403, _, _):
return callback(.failure(Errors.permissionDenied(metadataURL)))
case (404, _, _):
return callback(.failure(NotFoundError("\(baseURL)")))
case (200, _, _):
if apiRemaining < self.configuration.apiLimitWarningThreshold {
self.diagnosticsEngine?.emit(warning: "Approaching API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
}
// if successful, fan out multiple API calls
[tagsURL, contributorsURL, readmeURL].forEach { url in
sync.enter()
var headers = self.makeRequestHeaders(url)
headers.add(name: "Accept", value: "application/vnd.github.v3+json")
let options = self.makeRequestOptions(validResponseCodes: [200])
httpClient.get(url, headers: headers, options: options) { result in
defer { sync.leave() }
resultsLock.withLock {
results[url] = result
}
let sync = DispatchGroup()
var results = [URL: Result<HTTPClientResponse, Error>]()
let resultsLock = Lock()

// get the main data
sync.enter()
var metadataHeaders = self.makeRequestHeaders(metadataURL)
metadataHeaders.add(name: "Accept", value: "application/vnd.github.mercy-preview+json")
let metadataOptions = self.makeRequestOptions(validResponseCodes: [200, 401, 403, 404])
httpClient.get(metadataURL, headers: metadataHeaders, options: metadataOptions) { result in
defer { sync.leave() }
resultsLock.withLock {
results[metadataURL] = result
}
if case .success(let response) = result {
let apiLimit = response.headers.get("X-RateLimit-Limit").first.flatMap(Int.init) ?? -1
let apiRemaining = response.headers.get("X-RateLimit-Remaining").first.flatMap(Int.init) ?? -1
switch (response.statusCode, metadataHeaders.contains("Authorization"), apiRemaining) {
case (_, _, 0):
self.diagnosticsEngine?.emit(warning: "Exceeded API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
resultsLock.withLock {
results[metadataURL] = .failure(Errors.apiLimitsExceeded(metadataURL, apiLimit))
}
case (401, true, _):
resultsLock.withLock {
results[metadataURL] = .failure(Errors.invalidAuthToken(metadataURL))
}
case (401, false, _):
resultsLock.withLock {
results[metadataURL] = .failure(Errors.permissionDenied(metadataURL))
}
case (403, _, _):
resultsLock.withLock {
results[metadataURL] = .failure(Errors.permissionDenied(metadataURL))
}
case (404, _, _):
resultsLock.withLock {
results[metadataURL] = .failure(NotFoundError("\(baseURL)"))
}
case (200, _, _):
if apiRemaining < self.configuration.apiLimitWarningThreshold {
self.diagnosticsEngine?.emit(warning: "Approaching API limits on \(metadataURL.host ?? metadataURL.absoluteString) (\(apiRemaining)/\(apiLimit)), consider configuring an API token for this service.")
}
// if successful, fan out multiple API calls
[tagsURL, contributorsURL, readmeURL].forEach { url in
sync.enter()
var headers = self.makeRequestHeaders(url)
headers.add(name: "Accept", value: "application/vnd.github.v3+json")
let options = self.makeRequestOptions(validResponseCodes: [200])
httpClient.get(url, headers: headers, options: options) { result in
defer { sync.leave() }
resultsLock.withLock {
results[url] = result
}
}
default:
return callback(.failure(Errors.invalidResponse(metadataURL, "Invalid status code: \(response.statusCode)")))
}
default:
resultsLock.withLock {
results[metadataURL] = .failure(Errors.invalidResponse(metadataURL, "Invalid status code: \(response.statusCode)"))
}
}
}
sync.wait()

// process results
}
sync.wait()

do {
// check for main request error state
switch results[metadataURL] {
case .none:
throw Errors.invalidResponse(metadataURL, "Response missing")
case .some(.failure(let error)):
throw error
case .some(.success(let metadataResponse)):
guard let metadata = try metadataResponse.decodeBody(GetRepositoryResponse.self, using: self.decoder) else {
throw Errors.invalidResponse(metadataURL, "Empty body")
}
let tags = try results[tagsURL]?.success?.decodeBody([Tag].self, using: self.decoder) ?? []
let contributors = try results[contributorsURL]?.success?.decodeBody([Contributor].self, using: self.decoder)
let readme = try results[readmeURL]?.success?.decodeBody(Readme.self, using: self.decoder)
// process results

callback(.success(.init(
summary: metadata.description,
keywords: metadata.topics,
// filters out non-semantic versioned tags
versions: tags.compactMap { TSCUtility.Version(string: $0.name) },
watchersCount: metadata.watchersCount,
readmeURL: readme?.downloadURL,
authors: contributors?.map { .init(username: $0.login, url: $0.url, service: .init(name: "GitHub")) },
processedAt: Date()
)))
do {
// check for main request error state
switch results[metadataURL] {
case .none:
throw Errors.invalidResponse(metadataURL, "Response missing")
case .some(.failure(let error)):
throw error
case .some(.success(let metadataResponse)):
guard let metadata = try metadataResponse.decodeBody(GetRepositoryResponse.self, using: self.decoder) else {
throw Errors.invalidResponse(metadataURL, "Empty body")
}
} catch {
return callback(.failure(error))
let tags = try results[tagsURL]?.success?.decodeBody([Tag].self, using: self.decoder) ?? []
let contributors = try results[contributorsURL]?.success?.decodeBody([Contributor].self, using: self.decoder)
let readme = try results[readmeURL]?.success?.decodeBody(Readme.self, using: self.decoder)

callback(.success(.init(
summary: metadata.description,
keywords: metadata.topics,
// filters out non-semantic versioned tags
versions: tags.compactMap { TSCUtility.Version(string: $0.name) },
watchersCount: metadata.watchersCount,
readmeURL: readme?.downloadURL,
authors: contributors?.map { .init(username: $0.login, url: $0.url, service: .init(name: "GitHub")) },
processedAt: Date()
)))
}
} catch {
return callback(.failure(error))
}
}

Expand Down