From bb49d16050e2504d550e042a6db151f8716be694 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 5 Oct 2022 15:07:51 -0700 Subject: [PATCH 01/31] Break out networking related OS error codes --- lib/src/http.dart | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index 6473a8e19..3a853f648 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -189,14 +189,10 @@ class _ThrowingClient extends http.BaseClient { // with a retry. Failing to retry intermittent issues is likely to cause // customers to wrap pub in a retry loop which will not improve the // end-user experience. - if (error.osError!.errorCode == 8 || - error.osError!.errorCode == -2 || - error.osError!.errorCode == -5 || - error.osError!.errorCode == 11001 || - error.osError!.errorCode == 11004) { + if (error.osError!.isDnsError) { fail('Could not resolve URL "${request.url.origin}".', error, stackTrace); - } else if (error.osError!.errorCode == -12276) { + } else if (error.osError!.isSslError) { fail( 'Unable to validate SSL certificate for ' '"${request.url.origin}".', @@ -398,3 +394,21 @@ class _ThrottleClient extends http.BaseClient { @override void close() => _inner.close(); } + +extension on OSError { + get isDnsError { + // See https://github.com/dart-lang/pub/pull/2254#pullrequestreview-314895700 + const indeterminateOSCodes = [8, -2, -5]; + const windowsCodes = [11001, 11004]; + + return indeterminateOSCodes.contains(errorCode) || + windowsCodes.contains(errorCode); + } + + get isSslError { + // TODO: does dart/pub use nspr anymore? + const nsprCodes = [-12276]; + + return nsprCodes.contains(errorCode); + } +} From f4dd5a2e57e3781936c3e6f3e397c9d20cbfbfeb Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 5 Oct 2022 15:55:52 -0700 Subject: [PATCH 02/31] Add mapException feature to retry util --- lib/src/utils.dart | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 0f1bf5172..52162d668 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -659,7 +659,8 @@ bool fixedTimeBytesEquals(List? a, List? b) { } /// Call [fn] retrying so long as [retryIf] return `true` for the exception -/// thrown, up-to [maxAttempts] times. +/// thrown, up-to [maxAttempts] times. Optionally, pass [mapException] function +/// to update or replace [Exception] before [retryIf] is invoked. /// /// Defaults to 8 attempts, sleeping as following after 1st, 2nd, 3rd, ..., /// 7th attempt: @@ -692,7 +693,8 @@ Future retry( double randomizationFactor = 0.25, Duration maxDelay = const Duration(seconds: 30), int maxAttempts = 8, - FutureOr Function(Exception)? retryIf, + Exception Function(Exception, StackTrace)? mapException, + FutureOr Function(Exception, StackTrace)? retryIf, FutureOr Function(Exception, int retryCount)? onRetry, }) async { var attempt = 0; @@ -701,12 +703,16 @@ Future retry( attempt++; // first invocation is the first attempt try { return await fn(); - } on Exception catch (e) { - if (attempt >= maxAttempts || (retryIf != null && !(await retryIf(e)))) { + } on Exception catch (e, stackTrace) { + late final exception = mapException?.call(e, stackTrace) ?? e; + + if (attempt >= maxAttempts || + (retryIf != null && !(await retryIf(exception, stackTrace)))) { rethrow; } + if (onRetry != null) { - await onRetry(e, attempt); + await onRetry(exception, attempt); } } From e86748a12a5c14d2ab43556ba43482118d44bf40 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 6 Oct 2022 13:48:21 -0700 Subject: [PATCH 03/31] Add WIP sendWithRetries method --- lib/src/http.dart | 260 ++++++++++++++++++++++++++++------ lib/src/utils.dart | 2 +- test/get/hosted/get_test.dart | 8 +- 3 files changed, 221 insertions(+), 49 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index 3a853f648..014de196d 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -6,14 +6,15 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; -import 'dart:math' as math; +// import 'dart:math' as math; import 'package:http/http.dart' as http; -import 'package:http/retry.dart'; +// import 'package:http/retry.dart'; import 'package:pool/pool.dart'; -import 'package:stack_trace/stack_trace.dart'; +// import 'package:stack_trace/stack_trace.dart'; import 'command.dart'; +import 'exceptions.dart'; import 'io.dart'; import 'log.dart' as log; import 'oauth2.dart' as oauth2; @@ -233,46 +234,47 @@ class _ThrowingClient extends http.BaseClient { } /// The HTTP client to use for all HTTP requests. -final httpClient = _ThrottleClient( - 16, - _ThrowingClient(RetryClient(_pubClient, - retries: math.max( - 1, // Having less than 1 retry is **always** wrong. - int.tryParse(Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? 7, - ), - when: (response) => - const [500, 502, 503, 504].contains(response.statusCode), - whenError: (error, stackTrace) { - if (error is! IOException) return false; - - var chain = Chain.forTrace(stackTrace); - log.io('HTTP error:\n$error\n\n${chain.terse}'); - return true; - }, - delay: (retryCount) { - if (retryCount < 3) { - // Retry quickly a couple times in case of a short transient error. - // - // Add a random delay to avoid retrying a bunch of parallel requests - // all at the same time. - return Duration(milliseconds: 500) * math.pow(1.5, retryCount) + - Duration(milliseconds: random.nextInt(500)); - } else { - // If the error persists, wait a long time. This works around issues - // where an AppEngine instance will go down and need to be rebooted, - // which takes about a minute. - return Duration(seconds: 30); - } - }, - onRetry: (request, response, retryCount) { - log.io('Retry #${retryCount + 1} for ' - '${request.method} ${request.url}...'); - if (retryCount != 3) return; - if (!_retriedHosts.add(request.url.host)) return; - log.message( - 'It looks like ${request.url.host} is having some trouble.\n' - 'Pub will wait for a while before trying to connect again.'); - }))); +final httpClient = _pubClient; +// final httpClient = _ThrottleClient( +// 16, +// _ThrowingClient(RetryClient(_pubClient, +// retries: math.max( +// 1, // Having less than 1 retry is **always** wrong. +// int.tryParse(Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? 7, +// ), +// when: (response) => +// const [500, 502, 503, 504].contains(response.statusCode), +// whenError: (error, stackTrace) { +// if (error is! IOException) return false; + +// var chain = Chain.forTrace(stackTrace); +// log.io('HTTP error:\n$error\n\n${chain.terse}'); +// return true; +// }, +// delay: (retryCount) { +// if (retryCount < 3) { +// // Retry quickly a couple times in case of a short transient error. +// // +// // Add a random delay to avoid retrying a bunch of parallel requests +// // all at the same time. +// return Duration(milliseconds: 500) * math.pow(1.5, retryCount) + +// Duration(milliseconds: random.nextInt(500)); +// } else { +// // If the error persists, wait a long time. This works around issues +// // where an AppEngine instance will go down and need to be rebooted, +// // which takes about a minute. +// return Duration(seconds: 30); +// } +// }, +// onRetry: (request, response, retryCount) { +// log.io('Retry #${retryCount + 1} for ' +// '${request.method} ${request.url}...'); +// if (retryCount != 3) return; +// if (!_retriedHosts.add(request.url.host)) return; +// log.message( +// 'It looks like ${request.url.host} is having some trouble.\n' +// 'Pub will wait for a while before trying to connect again.'); +// }))); /// The underlying HTTP client wrapped by [httpClient]. http.Client get innerHttpClient => _pubClient._inner; @@ -339,11 +341,27 @@ Map parseJsonResponse(http.Response response) { Never invalidServerResponse(http.Response response) => fail(log.red('Invalid server response:\n${response.body}')); +/// Exception thrown when an HTTP operation fails. +class PubConnectException extends WrappedException { + final bool couldRetry; + + PubConnectException( + String message, { + required this.couldRetry, + Object? innerError, + StackTrace? innerTrace, + }) : super(message, innerError, innerTrace); + + @override + String toString() => 'Connection error: $message. Intermittent: $couldRetry'; +} + /// Exception thrown when an HTTP operation fails. class PubHttpException implements Exception { final http.Response response; + final bool couldRetry; - const PubHttpException(this.response); + const PubHttpException(this.response, {this.couldRetry = false}); @override String toString() => 'HTTP error ${response.statusCode}: ' @@ -412,3 +430,157 @@ extension on OSError { return nsprCodes.contains(errorCode); } } + +extension on http.StreamedResponse { + /// Creates a copy of this response with [newStream] as the stream. + http.StreamedResponse replacingStream(Stream> newStream) { + return http.StreamedResponse(newStream, statusCode, + contentLength: contentLength, + request: request, + headers: headers, + isRedirect: isRedirect, + persistentConnection: persistentConnection, + reasonPhrase: reasonPhrase); + } +} + +/// Program-wide limiter for concurrent network requests. +final _httpPool = Pool(16); + +extension HttpRetrying on http.Client { + Future sendWithRetries( + {required FutureOr Function() composeRequest, + required Future Function(http.StreamedResponse response) onResponse, + int maxAttempts = 8, + FutureOr Function(Exception, StackTrace)? retryIf, + FutureOr Function(Exception e, int retryCount, http.Request request, + http.StreamedResponse response)? + onRetry}) async { + late http.Request request; + late http.StreamedResponse response; + + return await retry(() async { + final resource = await _httpPool.request(); + request = await composeRequest(); + + http.StreamedResponse directResponse; + try { + directResponse = await send(request); + directResponse.throwIfError(); + } catch (_) { + resource.release(); + rethrow; + } + + // [PoolResource] has no knowledge of streams, so we must manually release + // the resource once the stream is canceled or done. We pipe the response + // stream through a [StreamController], which enables us to set up lifecycle + // hooks. + var didStreamActivate = false; + final responseController = StreamController>( + sync: true, + onListen: () => didStreamActivate = true, + ); + + // TODO: does this need to be in a try block? this internally calls + // [addStream], which theoretically(?) could throw if the stream has an + // error. + unawaited(directResponse.stream.pipe(responseController)); + unawaited(responseController.done.then((_) => resource.release())); + response = directResponse.replacingStream(responseController.stream); + + try { + return await onResponse(response); + } catch (_) { + rethrow; + } finally { + if (!didStreamActivate) { + // Release resource if the stream was never subscribed to. + unawaited(responseController.stream.listen(null).cancel()); + } + } + }, mapException: (e, stackTrace) { + if (e is SocketException) { + final osError = e.osError; + if (osError == null) return PubConnectException('', couldRetry: true); + + // Handle error codes known to be related to DNS or SSL issues. While it + // is tempting to handle these error codes before retrying, saving time + // for the end-user, it is known that DNS lookups can fail intermittently + // in some cloud environments. Furthermore, since these error codes are + // platform-specific (undocumented) and essentially cargo-culted along + // skipping retries may lead to intermittent issues that could be fixed + // with a retry. Failing to retry intermittent issues is likely to cause + // customers to wrap pub in a retry loop which will not improve the + // end-user experience. + + // TODO: should these return PubConnectException('msg', couldRetry: false)? + if (osError.isDnsError) { + fail('Could not resolve URL "${request.url.origin}".'); + } else if (osError.isSslError) { + fail( + 'Unable to validate SSL certificate for "${request.url.origin}".'); + } + } + + return e; + }, retryIf: (e, stackTrace) async { + // TODO: think about IOException. RetryClient used to catch IOException + // from _PubHttpClient. + + if (e is PubConnectException && e.couldRetry) return true; + if (e is PubHttpException && e.couldRetry) return true; + + return (e is PubHttpException && e.couldRetry) || + e is HttpException || + e is TimeoutException || + e is FormatException || + (retryIf != null && await retryIf(e, stackTrace)); + }, onRetry: (exception, retryCount) async { + if (onRetry != null) { + await onRetry(exception, retryCount, request, response); + } else { + log.io('Retry #${retryCount + 1} for ' + '${request.method} ${request.url}...'); + } + + if (retryCount != 3) return; + if (!_retriedHosts.add(request.url.host)) return; + log.message('It looks like ${request.url.host} is having some trouble.\n' + 'Pub will wait for a while before trying to connect again.'); + }, maxAttempts: maxAttempts); + } +} + +extension on http.BaseResponse { + void throwIfError() { + // Retry if the response indicates a server error. + if ([500, 502, 503, 504].contains(statusCode)) { + throw PubHttpException(this as http.Response, couldRetry: true); + } + + // 401 responses should be handled by the OAuth2 client. It's very + // unlikely that they'll be returned by non-OAuth2 requests. We also want + // to pass along 400 responses from the token endpoint. + var tokenRequest = request!.url == oauth2.tokenEndpoint; + if (statusCode < 400 || + statusCode == 401 || + (statusCode == 400 && tokenRequest)) { + return; + } + + if (statusCode == 406 && + request!.headers['Accept'] == pubApiHeaders['Accept']) { + fail('Pub ${sdk.version} is incompatible with the current version of ' + '${request!.url.host}.\n' + 'Upgrade pub to the latest version and try again.'); + } + + if (statusCode == 500 && + (request!.url.host == 'pub.dartlang.org' || + request!.url.host == 'storage.googleapis.com')) { + fail('HTTP error 500: Internal Server Error at ${request!.url}.\n' + 'This is likely a transient error. Please try again later.'); + } + } +} diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 52162d668..de1abb5d9 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -712,7 +712,7 @@ Future retry( } if (onRetry != null) { - await onRetry(exception, attempt); + await onRetry(exception, attempt - 1); } } diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index 81d484558..3f5362f0c 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -143,7 +143,7 @@ void main() { r'''Package archive for foo 1.2.3 downloaded from "(.+)" has ''' r'''"x-goog-hash: crc32c=(\d+)", which doesn't match the checksum ''' r'''of the archive downloaded\.'''), - silent: contains('Retry #2 because of checksum error'), + silent: contains('Retry #1 because of checksum error'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -179,7 +179,7 @@ void main() { error: contains( 'Package archive "foo-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #2 because of checksum error'), + silent: contains('Retry #1 because of checksum error'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -199,7 +199,7 @@ void main() { error: contains( 'Package archive "bar-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #2 because of checksum error'), + silent: contains('Retry #1 because of checksum error'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -219,7 +219,7 @@ void main() { error: contains( 'Package archive "baz-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #2 because of checksum error'), + silent: contains('Retry #1 because of checksum error'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, From b985c3773f2ea3d657f32deb979c874a8a82ed3d Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 6 Oct 2022 13:57:55 -0700 Subject: [PATCH 04/31] Fix potential runtime null response in onRetry option --- lib/src/http.dart | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index 014de196d..a9cd6559d 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -454,10 +454,10 @@ extension HttpRetrying on http.Client { int maxAttempts = 8, FutureOr Function(Exception, StackTrace)? retryIf, FutureOr Function(Exception e, int retryCount, http.Request request, - http.StreamedResponse response)? + http.StreamedResponse? response)? onRetry}) async { late http.Request request; - late http.StreamedResponse response; + http.StreamedResponse? managedResponse; return await retry(() async { final resource = await _httpPool.request(); @@ -487,10 +487,11 @@ extension HttpRetrying on http.Client { // error. unawaited(directResponse.stream.pipe(responseController)); unawaited(responseController.done.then((_) => resource.release())); - response = directResponse.replacingStream(responseController.stream); + managedResponse = + directResponse.replacingStream(responseController.stream); try { - return await onResponse(response); + return await onResponse(managedResponse!); } catch (_) { rethrow; } finally { @@ -538,7 +539,7 @@ extension HttpRetrying on http.Client { (retryIf != null && await retryIf(e, stackTrace)); }, onRetry: (exception, retryCount) async { if (onRetry != null) { - await onRetry(exception, retryCount, request, response); + await onRetry(exception, retryCount, request, managedResponse); } else { log.io('Retry #${retryCount + 1} for ' '${request.method} ${request.url}...'); From f7ae46ad82b2af6b637d722bc48df23dbb15989e Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 6 Oct 2022 15:00:19 -0700 Subject: [PATCH 05/31] Remove code to get CI working --- lib/src/http.dart | 155 ---------------------------------------------- 1 file changed, 155 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index a9cd6559d..6ccf14d71 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -163,118 +163,8 @@ final _pubClient = _PubHttpClient(); /// we're waiting for them to come back up. final _retriedHosts = {}; -/// Intercepts all requests and throws exceptions if the response was not -/// considered successful. -class _ThrowingClient extends http.BaseClient { - final http.Client _inner; - - _ThrowingClient(this._inner); - - @override - Future send(http.BaseRequest request) async { - late http.StreamedResponse streamedResponse; - try { - streamedResponse = await _inner.send(request); - } on SocketException catch (error, stackTraceOrNull) { - // Work around issue 23008. - var stackTrace = stackTraceOrNull; - - if (error.osError == null) rethrow; - - // Handle error codes known to be related to DNS or SSL issues. While it - // is tempting to handle these error codes before retrying, saving time - // for the end-user, it is known that DNS lookups can fail intermittently - // in some cloud environments. Furthermore, since these error codes are - // platform-specific (undocumented) and essentially cargo-culted along - // skipping retries may lead to intermittent issues that could be fixed - // with a retry. Failing to retry intermittent issues is likely to cause - // customers to wrap pub in a retry loop which will not improve the - // end-user experience. - if (error.osError!.isDnsError) { - fail('Could not resolve URL "${request.url.origin}".', error, - stackTrace); - } else if (error.osError!.isSslError) { - fail( - 'Unable to validate SSL certificate for ' - '"${request.url.origin}".', - error, - stackTrace); - } else { - rethrow; - } - } - - var status = streamedResponse.statusCode; - // 401 responses should be handled by the OAuth2 client. It's very - // unlikely that they'll be returned by non-OAuth2 requests. We also want - // to pass along 400 responses from the token endpoint. - var tokenRequest = streamedResponse.request!.url == oauth2.tokenEndpoint; - if (status < 400 || status == 401 || (status == 400 && tokenRequest)) { - return streamedResponse; - } - - if (status == 406 && request.headers['Accept'] == pubApiHeaders['Accept']) { - fail('Pub ${sdk.version} is incompatible with the current version of ' - '${request.url.host}.\n' - 'Upgrade pub to the latest version and try again.'); - } - - if (status == 500 && - (request.url.host == 'pub.dev' || - request.url.host == 'storage.googleapis.com')) { - fail('HTTP error 500: Internal Server Error at ${request.url}.\n' - 'This is likely a transient error. Please try again later.'); - } - - throw PubHttpException(await http.Response.fromStream(streamedResponse)); - } - - @override - void close() => _inner.close(); -} - /// The HTTP client to use for all HTTP requests. final httpClient = _pubClient; -// final httpClient = _ThrottleClient( -// 16, -// _ThrowingClient(RetryClient(_pubClient, -// retries: math.max( -// 1, // Having less than 1 retry is **always** wrong. -// int.tryParse(Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? 7, -// ), -// when: (response) => -// const [500, 502, 503, 504].contains(response.statusCode), -// whenError: (error, stackTrace) { -// if (error is! IOException) return false; - -// var chain = Chain.forTrace(stackTrace); -// log.io('HTTP error:\n$error\n\n${chain.terse}'); -// return true; -// }, -// delay: (retryCount) { -// if (retryCount < 3) { -// // Retry quickly a couple times in case of a short transient error. -// // -// // Add a random delay to avoid retrying a bunch of parallel requests -// // all at the same time. -// return Duration(milliseconds: 500) * math.pow(1.5, retryCount) + -// Duration(milliseconds: random.nextInt(500)); -// } else { -// // If the error persists, wait a long time. This works around issues -// // where an AppEngine instance will go down and need to be rebooted, -// // which takes about a minute. -// return Duration(seconds: 30); -// } -// }, -// onRetry: (request, response, retryCount) { -// log.io('Retry #${retryCount + 1} for ' -// '${request.method} ${request.url}...'); -// if (retryCount != 3) return; -// if (!_retriedHosts.add(request.url.host)) return; -// log.message( -// 'It looks like ${request.url.host} is having some trouble.\n' -// 'Pub will wait for a while before trying to connect again.'); -// }))); /// The underlying HTTP client wrapped by [httpClient]. http.Client get innerHttpClient => _pubClient._inner; @@ -368,51 +258,6 @@ class PubHttpException implements Exception { '${response.reasonPhrase}'; } -/// A middleware client that throttles the number of concurrent requests. -/// -/// As long as the number of requests is within the limit, this works just like -/// a normal client. If a request is made beyond the limit, the underlying HTTP -/// request won't be sent until other requests have completed. -class _ThrottleClient extends http.BaseClient { - final Pool _pool; - final http.Client _inner; - - /// Creates a new client that allows no more than [maxActiveRequests] - /// concurrent requests. - /// - /// If [inner] is passed, it's used as the inner client for sending HTTP - /// requests. It defaults to `new http.Client()`. - _ThrottleClient(int maxActiveRequests, this._inner) - : _pool = Pool(maxActiveRequests); - - @override - Future send(http.BaseRequest request) async { - var resource = await _pool.request(); - - http.StreamedResponse response; - try { - response = await _inner.send(request); - } catch (_) { - resource.release(); - rethrow; - } - - final responseController = StreamController>(sync: true); - unawaited(response.stream.pipe(responseController)); - unawaited(responseController.done.then((_) => resource.release())); - return http.StreamedResponse(responseController.stream, response.statusCode, - contentLength: response.contentLength, - request: response.request, - headers: response.headers, - isRedirect: response.isRedirect, - persistentConnection: response.persistentConnection, - reasonPhrase: response.reasonPhrase); - } - - @override - void close() => _inner.close(); -} - extension on OSError { get isDnsError { // See https://github.com/dart-lang/pub/pull/2254#pullrequestreview-314895700 From d05678dc6e01a26adf2a242298cf12e3bc1e4a6d Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 7 Oct 2022 16:50:29 -0700 Subject: [PATCH 06/31] Apply suggestions from code review Co-authored-by: Jonas Finnemann Jensen --- lib/src/http.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index 6ccf14d71..2b479a876 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -311,7 +311,7 @@ extension HttpRetrying on http.Client { http.StreamedResponse directResponse; try { directResponse = await send(request); - directResponse.throwIfError(); + directResponse.throwIfNotOK(); } catch (_) { resource.release(); rethrow; @@ -399,7 +399,7 @@ extension HttpRetrying on http.Client { } extension on http.BaseResponse { - void throwIfError() { + void throwIfNotOK() { // Retry if the response indicates a server error. if ([500, 502, 503, 504].contains(statusCode)) { throw PubHttpException(this as http.Response, couldRetry: true); @@ -416,9 +416,9 @@ extension on http.BaseResponse { } if (statusCode == 406 && - request!.headers['Accept'] == pubApiHeaders['Accept']) { + request?.headers['Accept'] == pubApiHeaders['Accept']) { fail('Pub ${sdk.version} is incompatible with the current version of ' - '${request!.url.host}.\n' + '${request?.url.host}.\n' 'Upgrade pub to the latest version and try again.'); } From 4aed21695e8560ac637c6c79802bef6f70322710 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 7 Oct 2022 17:35:28 -0700 Subject: [PATCH 07/31] Update Pub HTTP exceptions --- lib/src/authentication/client.dart | 2 +- lib/src/command/lish.dart | 6 ++--- lib/src/http.dart | 41 ++++++++++++++---------------- lib/src/source/hosted.dart | 2 +- 4 files changed, 24 insertions(+), 27 deletions(-) diff --git a/lib/src/authentication/client.dart b/lib/src/authentication/client.dart index a6001ecf9..f1f70af45 100644 --- a/lib/src/authentication/client.dart +++ b/lib/src/authentication/client.dart @@ -55,7 +55,7 @@ class _AuthenticatedClient extends http.BaseClient { _throwAuthException(response); } return response; - } on PubHttpException catch (e) { + } on PubHttpResponseException catch (e) { if (e.response.statusCode == 403) { _throwAuthException(e.response); } diff --git a/lib/src/command/lish.dart b/lib/src/command/lish.dart index 4b182b269..df0c75930 100644 --- a/lib/src/command/lish.dart +++ b/lib/src/command/lish.dart @@ -118,7 +118,7 @@ class LishCommand extends PubCommand { await http.Response.fromStream(await client.send(request)); var location = postResponse.headers['location']; - if (location == null) throw PubHttpException(postResponse); + if (location == null) throw PubHttpResponseException(postResponse); handleJsonSuccess( await client.get(Uri.parse(location), headers: pubApiHeaders)); }); @@ -138,7 +138,7 @@ class LishCommand extends PubCommand { msg += '\n${error.serverMessage!}\n'; } dataError(msg + log.red('Authentication failed!')); - } on PubHttpException catch (error) { + } on PubHttpResponseException catch (error) { var url = error.response.request!.url; if (url == cloudStorageUrl) { // TODO(nweiz): the response may have XML-formatted information about @@ -189,7 +189,7 @@ class LishCommand extends PubCommand { return _publishUsingClient(packageBytes, client); }); } - } on PubHttpException catch (error) { + } on PubHttpResponseException catch (error) { var url = error.response.request!.url; if (Uri.parse(url.origin) == Uri.parse(host.origin)) { handleJsonError(error.response); diff --git a/lib/src/http.dart b/lib/src/http.dart index 2b479a876..12c0a21f0 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -14,7 +14,6 @@ import 'package:pool/pool.dart'; // import 'package:stack_trace/stack_trace.dart'; import 'command.dart'; -import 'exceptions.dart'; import 'io.dart'; import 'log.dart' as log; import 'oauth2.dart' as oauth2; @@ -232,30 +231,29 @@ Never invalidServerResponse(http.Response response) => fail(log.red('Invalid server response:\n${response.body}')); /// Exception thrown when an HTTP operation fails. -class PubConnectException extends WrappedException { +class PubHttpException implements Exception { + final String message; final bool couldRetry; - PubConnectException( - String message, { - required this.couldRetry, - Object? innerError, - StackTrace? innerTrace, - }) : super(message, innerError, innerTrace); - - @override - String toString() => 'Connection error: $message. Intermittent: $couldRetry'; + PubHttpException(this.message, {this.couldRetry = false}); } -/// Exception thrown when an HTTP operation fails. -class PubHttpException implements Exception { +/// Exception thrown when an HTTP response is not OK. +class PubHttpResponseException extends PubHttpException { final http.Response response; - final bool couldRetry; - const PubHttpException(this.response, {this.couldRetry = false}); + PubHttpResponseException(this.response, + {String message = '', bool couldRetry = false}) + : super(message, couldRetry: couldRetry); @override - String toString() => 'HTTP error ${response.statusCode}: ' - '${response.reasonPhrase}'; + String toString() { + var temp = 'HTTP error ${response.statusCode}: ${response.reasonPhrase}'; + if (message != '') { + temp += ': $message'; + } + return temp; + } } extension on OSError { @@ -348,7 +346,9 @@ extension HttpRetrying on http.Client { }, mapException: (e, stackTrace) { if (e is SocketException) { final osError = e.osError; - if (osError == null) return PubConnectException('', couldRetry: true); + if (osError == null) { + return PubHttpException('Socket operation failure', couldRetry: true); + } // Handle error codes known to be related to DNS or SSL issues. While it // is tempting to handle these error codes before retrying, saving time @@ -374,9 +374,6 @@ extension HttpRetrying on http.Client { // TODO: think about IOException. RetryClient used to catch IOException // from _PubHttpClient. - if (e is PubConnectException && e.couldRetry) return true; - if (e is PubHttpException && e.couldRetry) return true; - return (e is PubHttpException && e.couldRetry) || e is HttpException || e is TimeoutException || @@ -402,7 +399,7 @@ extension on http.BaseResponse { void throwIfNotOK() { // Retry if the response indicates a server error. if ([500, 502, 503, 504].contains(statusCode)) { - throw PubHttpException(this as http.Response, couldRetry: true); + throw PubHttpResponseException(this as http.Response, couldRetry: true); } // 401 responses should be handled by the OAuth2 client. It's very diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 4e8a939fc..d0becf5ea 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1110,7 +1110,7 @@ See $contentHashesDocumentationUrl. String package, String hostedUrl, ) { - if (error is PubHttpException) { + if (error is PubHttpResponseException) { if (error.response.statusCode == 404) { throw PackageNotFoundException( 'could not find package $package at $hostedUrl', From d60e6bc4139d6404f1c23f2b3ce9cded454ba1fa Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 7 Oct 2022 17:37:49 -0700 Subject: [PATCH 08/31] Remove stack trace from retry methods --- lib/src/http.dart | 8 ++++---- lib/src/utils.dart | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index 12c0a21f0..b82539d37 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -295,7 +295,7 @@ extension HttpRetrying on http.Client { {required FutureOr Function() composeRequest, required Future Function(http.StreamedResponse response) onResponse, int maxAttempts = 8, - FutureOr Function(Exception, StackTrace)? retryIf, + FutureOr Function(Exception)? retryIf, FutureOr Function(Exception e, int retryCount, http.Request request, http.StreamedResponse? response)? onRetry}) async { @@ -343,7 +343,7 @@ extension HttpRetrying on http.Client { unawaited(responseController.stream.listen(null).cancel()); } } - }, mapException: (e, stackTrace) { + }, mapException: (e) { if (e is SocketException) { final osError = e.osError; if (osError == null) { @@ -370,7 +370,7 @@ extension HttpRetrying on http.Client { } return e; - }, retryIf: (e, stackTrace) async { + }, retryIf: (e) async { // TODO: think about IOException. RetryClient used to catch IOException // from _PubHttpClient. @@ -378,7 +378,7 @@ extension HttpRetrying on http.Client { e is HttpException || e is TimeoutException || e is FormatException || - (retryIf != null && await retryIf(e, stackTrace)); + (retryIf != null && await retryIf(e)); }, onRetry: (exception, retryCount) async { if (onRetry != null) { await onRetry(exception, retryCount, request, managedResponse); diff --git a/lib/src/utils.dart b/lib/src/utils.dart index de1abb5d9..db2bc8b5e 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -693,8 +693,8 @@ Future retry( double randomizationFactor = 0.25, Duration maxDelay = const Duration(seconds: 30), int maxAttempts = 8, - Exception Function(Exception, StackTrace)? mapException, - FutureOr Function(Exception, StackTrace)? retryIf, + Exception Function(Exception)? mapException, + FutureOr Function(Exception)? retryIf, FutureOr Function(Exception, int retryCount)? onRetry, }) async { var attempt = 0; @@ -703,11 +703,11 @@ Future retry( attempt++; // first invocation is the first attempt try { return await fn(); - } on Exception catch (e, stackTrace) { - late final exception = mapException?.call(e, stackTrace) ?? e; + } on Exception catch (e) { + late final exception = mapException?.call(e) ?? e; if (attempt >= maxAttempts || - (retryIf != null && !(await retryIf(exception, stackTrace)))) { + (retryIf != null && !(await retryIf(exception)))) { rethrow; } From f107cce43b7498b129731fe3c46e591a2e9b1d32 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 7 Oct 2022 17:39:30 -0700 Subject: [PATCH 09/31] Remove hard-coded domain check for HTTP 500 --- lib/src/http.dart | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index b82539d37..c5dee56ef 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -418,12 +418,5 @@ extension on http.BaseResponse { '${request?.url.host}.\n' 'Upgrade pub to the latest version and try again.'); } - - if (statusCode == 500 && - (request!.url.host == 'pub.dartlang.org' || - request!.url.host == 'storage.googleapis.com')) { - fail('HTTP error 500: Internal Server Error at ${request!.url}.\n' - 'This is likely a transient error. Please try again later.'); - } } } From f7d1ee57fd9e8d1f9caf90c97e402053ce63c4f0 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Thu, 27 Oct 2022 14:54:45 -0700 Subject: [PATCH 10/31] Simplify method --- lib/src/exceptions.dart | 9 +-- lib/src/http.dart | 148 ++++++++++++------------------------- lib/src/source/hosted.dart | 9 ++- 3 files changed, 57 insertions(+), 109 deletions(-) diff --git a/lib/src/exceptions.dart b/lib/src/exceptions.dart index 39e9c51e4..d7f8850f0 100644 --- a/lib/src/exceptions.dart +++ b/lib/src/exceptions.dart @@ -12,6 +12,7 @@ import 'package:stack_trace/stack_trace.dart'; import 'package:yaml/yaml.dart'; import 'dart.dart'; +import 'http.dart'; /// An exception class for exceptions that are intended to be seen by the user. /// @@ -106,12 +107,8 @@ class PackageNotFoundException extends WrappedException { } /// A class for exceptions where a package's checksum could not be validated. -class PackageIntegrityException extends WrappedException { - PackageIntegrityException( - String message, { - Object? innerError, - StackTrace? innerTrace, - }) : super(message, innerError, innerTrace); +class PackageIntegrityException extends PubHttpException { + PackageIntegrityException(super.message, {super.couldRetry}); } /// Returns whether [error] is a user-facing error object. diff --git a/lib/src/http.dart b/lib/src/http.dart index c5dee56ef..7db396ddc 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -290,112 +290,62 @@ extension on http.StreamedResponse { /// Program-wide limiter for concurrent network requests. final _httpPool = Pool(16); -extension HttpRetrying on http.Client { - Future sendWithRetries( - {required FutureOr Function() composeRequest, - required Future Function(http.StreamedResponse response) onResponse, - int maxAttempts = 8, - FutureOr Function(Exception)? retryIf, - FutureOr Function(Exception e, int retryCount, http.Request request, - http.StreamedResponse? response)? - onRetry}) async { - late http.Request request; - http.StreamedResponse? managedResponse; - - return await retry(() async { - final resource = await _httpPool.request(); - request = await composeRequest(); - - http.StreamedResponse directResponse; - try { - directResponse = await send(request); - directResponse.throwIfNotOK(); - } catch (_) { - resource.release(); - rethrow; +Future retryForHttp(String operation, FutureOr Function() fn, + {int maxAttempts = 8, + FutureOr Function(Exception)? retryIf, + FutureOr Function(Exception e, int retryCount)? onRetry}) async { + return await retry( + () async => await _httpPool.withResource(() async => await fn()), + mapException: (e) { + if (e is SocketException) { + final osError = e.osError; + if (osError == null) { + return PubHttpException('Socket operation failure', couldRetry: true); } - // [PoolResource] has no knowledge of streams, so we must manually release - // the resource once the stream is canceled or done. We pipe the response - // stream through a [StreamController], which enables us to set up lifecycle - // hooks. - var didStreamActivate = false; - final responseController = StreamController>( - sync: true, - onListen: () => didStreamActivate = true, - ); - - // TODO: does this need to be in a try block? this internally calls - // [addStream], which theoretically(?) could throw if the stream has an - // error. - unawaited(directResponse.stream.pipe(responseController)); - unawaited(responseController.done.then((_) => resource.release())); - managedResponse = - directResponse.replacingStream(responseController.stream); - - try { - return await onResponse(managedResponse!); - } catch (_) { - rethrow; - } finally { - if (!didStreamActivate) { - // Release resource if the stream was never subscribed to. - unawaited(responseController.stream.listen(null).cancel()); - } - } - }, mapException: (e) { - if (e is SocketException) { - final osError = e.osError; - if (osError == null) { - return PubHttpException('Socket operation failure', couldRetry: true); - } - - // Handle error codes known to be related to DNS or SSL issues. While it - // is tempting to handle these error codes before retrying, saving time - // for the end-user, it is known that DNS lookups can fail intermittently - // in some cloud environments. Furthermore, since these error codes are - // platform-specific (undocumented) and essentially cargo-culted along - // skipping retries may lead to intermittent issues that could be fixed - // with a retry. Failing to retry intermittent issues is likely to cause - // customers to wrap pub in a retry loop which will not improve the - // end-user experience. - - // TODO: should these return PubConnectException('msg', couldRetry: false)? - if (osError.isDnsError) { - fail('Could not resolve URL "${request.url.origin}".'); - } else if (osError.isSslError) { - fail( - 'Unable to validate SSL certificate for "${request.url.origin}".'); - } + // Handle error codes known to be related to DNS or SSL issues. While it + // is tempting to handle these error codes before retrying, saving time + // for the end-user, it is known that DNS lookups can fail intermittently + // in some cloud environments. Furthermore, since these error codes are + // platform-specific (undocumented) and essentially cargo-culted along + // skipping retries may lead to intermittent issues that could be fixed + // with a retry. Failing to retry intermittent issues is likely to cause + // customers to wrap pub in a retry loop which will not improve the + // end-user experience. + + // TODO: should these return PubConnectException('msg', couldRetry: false)? + if (osError.isDnsError) { + fail('Could not resolve URL when $operation.'); + } else if (osError.isSslError) { + fail('Unable to validate SSL certificate when $operation.'); } + } - return e; - }, retryIf: (e) async { - // TODO: think about IOException. RetryClient used to catch IOException - // from _PubHttpClient. - - return (e is PubHttpException && e.couldRetry) || - e is HttpException || - e is TimeoutException || - e is FormatException || - (retryIf != null && await retryIf(e)); - }, onRetry: (exception, retryCount) async { - if (onRetry != null) { - await onRetry(exception, retryCount, request, managedResponse); - } else { - log.io('Retry #${retryCount + 1} for ' - '${request.method} ${request.url}...'); - } + return e; + }, retryIf: (e) async { + // TODO: think about IOException. RetryClient used to catch IOException + // from _PubHttpClient. + + return (e is PubHttpException && e.couldRetry) || + e is HttpException || + e is TimeoutException || + e is FormatException || + (retryIf != null && await retryIf(e)); + }, onRetry: (exception, retryCount) async { + if (onRetry != null) { + await onRetry( + exception, + retryCount, + ); + } else { + log.io('Retry #${retryCount + 1} for $operation'); + } - if (retryCount != 3) return; - if (!_retriedHosts.add(request.url.host)) return; - log.message('It looks like ${request.url.host} is having some trouble.\n' - 'Pub will wait for a while before trying to connect again.'); - }, maxAttempts: maxAttempts); - } + if (retryCount != 3) return; + }, maxAttempts: maxAttempts); } -extension on http.BaseResponse { +extension Throwing on http.BaseResponse { void throwIfNotOK() { // Retry if the response indicates a server error. if ([500, 502, 503, 504].contains(statusCode)) { diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index d0becf5ea..f3a386d2f 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1049,9 +1049,10 @@ See $contentHashesDocumentationUrl. // The client from `withAuthenticatedClient` will retry HTTP requests. // This wrapper is one layer up and will retry checksum validation errors. - await retry( - // Attempt to download archive and validate its checksum. + await retryForHttp( + 'downloading "$archiveUrl"', () async { + // Attempt to download archive and validate its checksum. final request = http.Request('GET', archiveUrl); final response = await withAuthenticatedClient(cache, Uri.parse(description.url), (client) => client.send(request)); @@ -1436,11 +1437,11 @@ int? _parseCrc32c(Map headers, String fileName) { return ByteData.view(bytes.buffer).getUint32(0); } on FormatException catch (e, s) { + log.exception(e, s); throw PackageIntegrityException( 'Package archive "$fileName" has a malformed CRC32C checksum in ' 'its response headers', - innerError: e, - innerTrace: s); + couldRetry: true); } } } From 238e97cceb91405bb77951fd9294e250e8a66c0d Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 28 Oct 2022 13:44:51 -0700 Subject: [PATCH 11/31] Align retry behavior with requirements --- lib/src/exceptions.dart | 2 +- lib/src/http.dart | 142 +++++++++---------------------------- lib/src/source/hosted.dart | 67 +++++++---------- lib/src/utils.dart | 11 +-- 4 files changed, 62 insertions(+), 160 deletions(-) diff --git a/lib/src/exceptions.dart b/lib/src/exceptions.dart index d7f8850f0..fc897e2d0 100644 --- a/lib/src/exceptions.dart +++ b/lib/src/exceptions.dart @@ -108,7 +108,7 @@ class PackageNotFoundException extends WrappedException { /// A class for exceptions where a package's checksum could not be validated. class PackageIntegrityException extends PubHttpException { - PackageIntegrityException(super.message, {super.couldRetry}); + PackageIntegrityException(super.message, {super.isIntermittent}); } /// Returns whether [error] is a user-facing error object. diff --git a/lib/src/http.dart b/lib/src/http.dart index 7db396ddc..140e5496e 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -6,17 +6,14 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; -// import 'dart:math' as math; +import 'dart:math' as math; import 'package:http/http.dart' as http; -// import 'package:http/retry.dart'; import 'package:pool/pool.dart'; -// import 'package:stack_trace/stack_trace.dart'; import 'command.dart'; import 'io.dart'; import 'log.dart' as log; -import 'oauth2.dart' as oauth2; import 'package.dart'; import 'sdk.dart'; import 'source/hosted.dart'; @@ -158,10 +155,6 @@ class _PubHttpClient extends http.BaseClient { /// The [_PubHttpClient] wrapped by [httpClient]. final _pubClient = _PubHttpClient(); -/// A set of all hostnames for which we've printed a message indicating that -/// we're waiting for them to come back up. -final _retriedHosts = {}; - /// The HTTP client to use for all HTTP requests. final httpClient = _pubClient; @@ -233,9 +226,9 @@ Never invalidServerResponse(http.Response response) => /// Exception thrown when an HTTP operation fails. class PubHttpException implements Exception { final String message; - final bool couldRetry; + final bool isIntermittent; - PubHttpException(this.message, {this.couldRetry = false}); + PubHttpException(this.message, {this.isIntermittent = false}); } /// Exception thrown when an HTTP response is not OK. @@ -243,8 +236,8 @@ class PubHttpResponseException extends PubHttpException { final http.Response response; PubHttpResponseException(this.response, - {String message = '', bool couldRetry = false}) - : super(message, couldRetry: couldRetry); + {String message = '', bool isIntermittent = false}) + : super(message, isIntermittent: isIntermittent); @override String toString() { @@ -256,117 +249,46 @@ class PubHttpResponseException extends PubHttpException { } } -extension on OSError { - get isDnsError { - // See https://github.com/dart-lang/pub/pull/2254#pullrequestreview-314895700 - const indeterminateOSCodes = [8, -2, -5]; - const windowsCodes = [11001, 11004]; - - return indeterminateOSCodes.contains(errorCode) || - windowsCodes.contains(errorCode); - } - - get isSslError { - // TODO: does dart/pub use nspr anymore? - const nsprCodes = [-12276]; - - return nsprCodes.contains(errorCode); - } -} - -extension on http.StreamedResponse { - /// Creates a copy of this response with [newStream] as the stream. - http.StreamedResponse replacingStream(Stream> newStream) { - return http.StreamedResponse(newStream, statusCode, - contentLength: contentLength, - request: request, - headers: headers, - isRedirect: isRedirect, - persistentConnection: persistentConnection, - reasonPhrase: reasonPhrase); - } -} - /// Program-wide limiter for concurrent network requests. final _httpPool = Pool(16); -Future retryForHttp(String operation, FutureOr Function() fn, - {int maxAttempts = 8, - FutureOr Function(Exception)? retryIf, - FutureOr Function(Exception e, int retryCount)? onRetry}) async { +Future retryForHttp(String operation, FutureOr Function() fn) async { return await retry( () async => await _httpPool.withResource(() async => await fn()), - mapException: (e) { - if (e is SocketException) { - final osError = e.osError; - if (osError == null) { - return PubHttpException('Socket operation failure', couldRetry: true); - } - - // Handle error codes known to be related to DNS or SSL issues. While it - // is tempting to handle these error codes before retrying, saving time - // for the end-user, it is known that DNS lookups can fail intermittently - // in some cloud environments. Furthermore, since these error codes are - // platform-specific (undocumented) and essentially cargo-culted along - // skipping retries may lead to intermittent issues that could be fixed - // with a retry. Failing to retry intermittent issues is likely to cause - // customers to wrap pub in a retry loop which will not improve the - // end-user experience. - - // TODO: should these return PubConnectException('msg', couldRetry: false)? - if (osError.isDnsError) { - fail('Could not resolve URL when $operation.'); - } else if (osError.isSslError) { - fail('Unable to validate SSL certificate when $operation.'); - } - } - - return e; - }, retryIf: (e) async { - // TODO: think about IOException. RetryClient used to catch IOException - // from _PubHttpClient. - - return (e is PubHttpException && e.couldRetry) || - e is HttpException || - e is TimeoutException || - e is FormatException || - (retryIf != null && await retryIf(e)); - }, onRetry: (exception, retryCount) async { - if (onRetry != null) { - await onRetry( - exception, - retryCount, - ); - } else { - log.io('Retry #${retryCount + 1} for $operation'); - } - - if (retryCount != 3) return; - }, maxAttempts: maxAttempts); + retryIf: (e) async => + (e is PubHttpException && e.isIntermittent) || + e is TimeoutException || + e is HttpException || + e is TlsException || + e is SocketException || + e is WebSocketException, + onRetry: (exception, retryCount) async => + log.io('Retry #${retryCount + 1} for $operation'), + maxAttempts: math.max( + 1, // Having less than 1 attempt doesn't make sense. + int.tryParse(Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? 7, + )); } extension Throwing on http.BaseResponse { - void throwIfNotOK() { - // Retry if the response indicates a server error. - if ([500, 502, 503, 504].contains(statusCode)) { - throw PubHttpResponseException(this as http.Response, couldRetry: true); - } - - // 401 responses should be handled by the OAuth2 client. It's very - // unlikely that they'll be returned by non-OAuth2 requests. We also want - // to pass along 400 responses from the token endpoint. - var tokenRequest = request!.url == oauth2.tokenEndpoint; - if (statusCode < 400 || - statusCode == 401 || - (statusCode == 400 && tokenRequest)) { + void throwIfNotOk() { + if (statusCode >= 200 && statusCode <= 299) { return; - } - - if (statusCode == 406 && + } else if (statusCode == HttpStatus.notAcceptable && request?.headers['Accept'] == pubApiHeaders['Accept']) { fail('Pub ${sdk.version} is incompatible with the current version of ' '${request?.url.host}.\n' 'Upgrade pub to the latest version and try again.'); + } else if (statusCode >= 500 || + statusCode == HttpStatus.requestTimeout || + statusCode == HttpStatus.tooManyRequests) { + // Throw if the response indicates a server error or an intermittent + // client error, but mark it as intermittent so it can be retried. + throw PubHttpResponseException(this as http.Response, + isIntermittent: true); + } else { + // Throw for all other status codes. + throw PubHttpResponseException(this as http.Response); } } } diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index f3a386d2f..d2d7eed72 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -5,7 +5,6 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io' as io; -import 'dart:math' as math; import 'dart:typed_data'; import 'package:collection/collection.dart' @@ -1047,44 +1046,30 @@ See $contentHashesDocumentationUrl. // download. final expectedSha256 = versionInfo.archiveSha256; - // The client from `withAuthenticatedClient` will retry HTTP requests. - // This wrapper is one layer up and will retry checksum validation errors. - await retryForHttp( - 'downloading "$archiveUrl"', - () async { - // Attempt to download archive and validate its checksum. - final request = http.Request('GET', archiveUrl); - final response = await withAuthenticatedClient(cache, - Uri.parse(description.url), (client) => client.send(request)); - final expectedCrc32Checksum = - _parseCrc32c(response.headers, fileName); - - Stream> stream = response.stream; - if (expectedCrc32Checksum != null) { - stream = _validateStreamCrc32Checksum( - response.stream, expectedCrc32Checksum, id, archiveUrl); - } - stream = validateSha256( - stream, (expectedSha256 == null) ? null : Digest(expectedSha256)); - // We download the archive to disk instead of streaming it directly - // into the tar unpacking. This simplifies stream handling. - // Package:tar cancels the stream when it reaches end-of-archive, and - // cancelling a http stream makes it not reusable. - // There are ways around this, and we might revisit this later. - await createFileFromStream(stream, archivePath); - }, - // Retry if the checksum response header was malformed or the actual - // checksum did not match the expected checksum. - retryIf: (e) => e is PackageIntegrityException, - onRetry: (e, retryCount) => log - .io('Retry #${retryCount + 1} because of checksum error with GET ' - '$archiveUrl...'), - maxAttempts: math.max( - 1, // Having less than 1 attempt doesn't make sense. - int.tryParse(io.Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? - 7, - ), - ); + // This will retry HTTP errors as well as sha256/crc32c errors because + // [PackageIntegrityException] subclasses [PubHttpException]. + await retryForHttp('downloading "$archiveUrl"', () async { + final request = http.Request('GET', archiveUrl); + final response = await withAuthenticatedClient(cache, + Uri.parse(description.url), (client) => client.send(request)); + response.throwIfNotOk(); + + Stream> stream = response.stream; + final expectedCrc32c = _parseCrc32c(response.headers, fileName); + if (expectedCrc32c != null) { + stream = + _validateCrc32c(response.stream, expectedCrc32c, id, archiveUrl); + } + stream = validateSha256( + stream, (expectedSha256 == null) ? null : Digest(expectedSha256)); + + // We download the archive to disk instead of streaming it directly + // into the tar unpacking. This simplifies stream handling. + // Package:tar cancels the stream when it reaches end-of-archive, and + // cancelling a http stream makes it not reusable. + // There are ways around this, and we might revisit this later. + await createFileFromStream(stream, archivePath); + }); var tempDir = cache.createTempDir(); await extractTarGz(readBinaryFileAsStream(archivePath), tempDir); @@ -1379,7 +1364,7 @@ const checksumHeaderName = 'x-goog-hash'; /// the one present in the checksum response header. /// /// Throws [PackageIntegrityException] if there is a checksum mismatch. -Stream> _validateStreamCrc32Checksum(Stream> stream, +Stream> _validateCrc32c(Stream> stream, int expectedChecksum, PackageId id, Uri archiveUrl) async* { final crc32c = Crc32c(); @@ -1441,7 +1426,7 @@ int? _parseCrc32c(Map headers, String fileName) { throw PackageIntegrityException( 'Package archive "$fileName" has a malformed CRC32C checksum in ' 'its response headers', - couldRetry: true); + isIntermittent: true); } } } diff --git a/lib/src/utils.dart b/lib/src/utils.dart index db2bc8b5e..db89c4d5a 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -659,8 +659,7 @@ bool fixedTimeBytesEquals(List? a, List? b) { } /// Call [fn] retrying so long as [retryIf] return `true` for the exception -/// thrown, up-to [maxAttempts] times. Optionally, pass [mapException] function -/// to update or replace [Exception] before [retryIf] is invoked. +/// thrown, up-to [maxAttempts] times. /// /// Defaults to 8 attempts, sleeping as following after 1st, 2nd, 3rd, ..., /// 7th attempt: @@ -693,7 +692,6 @@ Future retry( double randomizationFactor = 0.25, Duration maxDelay = const Duration(seconds: 30), int maxAttempts = 8, - Exception Function(Exception)? mapException, FutureOr Function(Exception)? retryIf, FutureOr Function(Exception, int retryCount)? onRetry, }) async { @@ -704,15 +702,12 @@ Future retry( try { return await fn(); } on Exception catch (e) { - late final exception = mapException?.call(e) ?? e; - - if (attempt >= maxAttempts || - (retryIf != null && !(await retryIf(exception)))) { + if (attempt >= maxAttempts || (retryIf != null && !(await retryIf(e)))) { rethrow; } if (onRetry != null) { - await onRetry(exception, attempt - 1); + await onRetry(e, attempt - 1); } } From 4d245ad45d0b61943a07c42c965a1963f67f30eb Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 28 Oct 2022 13:55:25 -0700 Subject: [PATCH 12/31] Move withAuthenticatedClient a layer up and clarify comment --- lib/src/source/hosted.dart | 49 ++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index d2d7eed72..af84d021f 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1046,29 +1046,32 @@ See $contentHashesDocumentationUrl. // download. final expectedSha256 = versionInfo.archiveSha256; - // This will retry HTTP errors as well as sha256/crc32c errors because - // [PackageIntegrityException] subclasses [PubHttpException]. - await retryForHttp('downloading "$archiveUrl"', () async { - final request = http.Request('GET', archiveUrl); - final response = await withAuthenticatedClient(cache, - Uri.parse(description.url), (client) => client.send(request)); - response.throwIfNotOk(); - - Stream> stream = response.stream; - final expectedCrc32c = _parseCrc32c(response.headers, fileName); - if (expectedCrc32c != null) { - stream = - _validateCrc32c(response.stream, expectedCrc32c, id, archiveUrl); - } - stream = validateSha256( - stream, (expectedSha256 == null) ? null : Digest(expectedSha256)); - - // We download the archive to disk instead of streaming it directly - // into the tar unpacking. This simplifies stream handling. - // Package:tar cancels the stream when it reaches end-of-archive, and - // cancelling a http stream makes it not reusable. - // There are ways around this, and we might revisit this later. - await createFileFromStream(stream, archivePath); + await withAuthenticatedClient(cache, Uri.parse(description.url), + (client) async { + // In addition to HTTP errors, this will retry crc32c/sha256 errors as + // well because [PackageIntegrityException] subclasses + // [PubHttpException]. + await retryForHttp('downloading "$archiveUrl"', () async { + final request = http.Request('GET', archiveUrl); + final response = await client.send(request); + response.throwIfNotOk(); + + Stream> stream = response.stream; + final expectedCrc32c = _parseCrc32c(response.headers, fileName); + if (expectedCrc32c != null) { + stream = _validateCrc32c( + response.stream, expectedCrc32c, id, archiveUrl); + } + stream = validateSha256( + stream, (expectedSha256 == null) ? null : Digest(expectedSha256)); + + // We download the archive to disk instead of streaming it directly + // into the tar unpacking. This simplifies stream handling. + // Package:tar cancels the stream when it reaches end-of-archive, and + // cancelling a http stream makes it not reusable. + // There are ways around this, and we might revisit this later. + await createFileFromStream(stream, archivePath); + }); }); var tempDir = cache.createTempDir(); From 1f18d5ce092c38cbe8f2924c650a3ba43d20f4ab Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 31 Oct 2022 13:41:45 -0700 Subject: [PATCH 13/31] Clarify usage of global HTTP client --- lib/src/authentication/client.dart | 2 +- lib/src/command.dart | 2 +- lib/src/command/login.dart | 2 +- lib/src/http.dart | 7 ++++--- lib/src/oauth2.dart | 4 ++-- tool/extract_all_pub_dev.dart | 6 +++--- 6 files changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/src/authentication/client.dart b/lib/src/authentication/client.dart index f1f70af45..477dc3997 100644 --- a/lib/src/authentication/client.dart +++ b/lib/src/authentication/client.dart @@ -127,7 +127,7 @@ Future withAuthenticatedClient( Future Function(http.Client) fn, ) async { final credential = systemCache.tokenStore.findCredential(hostedUrl); - final client = _AuthenticatedClient(httpClient, credential); + final client = _AuthenticatedClient(globalHttpClient, credential); try { return await fn(client); diff --git a/lib/src/command.dart b/lib/src/command.dart index 31f0b17aa..fa5d03cb4 100644 --- a/lib/src/command.dart +++ b/lib/src/command.dart @@ -238,7 +238,7 @@ and attaching the relevant parts of that log file. log.message('Logs written to $transcriptPath.'); } } - httpClient.close(); + globalHttpClient.close(); } } diff --git a/lib/src/command/login.dart b/lib/src/command/login.dart index 8f428ab14..632b56e8b 100644 --- a/lib/src/command/login.dart +++ b/lib/src/command/login.dart @@ -46,7 +46,7 @@ class LoginCommand extends PubCommand { Future<_UserInfo?> _retrieveUserInfo() async { return await oauth2.withClient(cache, (client) async { - final discovery = await httpClient.get(Uri.https( + final discovery = await globalHttpClient.get(Uri.https( 'accounts.google.com', '/.well-known/openid-configuration')); final userInfoEndpoint = json.decode(discovery.body)['userinfo_endpoint']; final userInfoRequest = await client.get(Uri.parse(userInfoEndpoint)); diff --git a/lib/src/http.dart b/lib/src/http.dart index 140e5496e..4462b43f7 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -152,13 +152,14 @@ class _PubHttpClient extends http.BaseClient { void close() => _inner.close(); } -/// The [_PubHttpClient] wrapped by [httpClient]. +/// The [_PubHttpClient] wrapped by [globalHttpClient]. final _pubClient = _PubHttpClient(); /// The HTTP client to use for all HTTP requests. -final httpClient = _pubClient; +final globalHttpClient = _pubClient; -/// The underlying HTTP client wrapped by [httpClient]. +/// The underlying HTTP client wrapped by [globalHttpClient]. +/// This enables the ability to use a mock client in tests. http.Client get innerHttpClient => _pubClient._inner; set innerHttpClient(http.Client client) => _pubClient._inner = client; diff --git a/lib/src/oauth2.dart b/lib/src/oauth2.dart index 2159847d9..b32157da7 100644 --- a/lib/src/oauth2.dart +++ b/lib/src/oauth2.dart @@ -142,7 +142,7 @@ Future _getClient(SystemCache cache) async { secret: _secret, // Google's OAuth2 API doesn't support basic auth. basicAuth: false, - httpClient: httpClient); + httpClient: globalHttpClient); _saveCredentials(cache, client.credentials); return client; } @@ -221,7 +221,7 @@ Future _authorize() async { secret: _secret, // Google's OAuth2 API doesn't support basic auth. basicAuth: false, - httpClient: httpClient); + httpClient: globalHttpClient); // Spin up a one-shot HTTP server to receive the authorization code from the // Google OAuth2 server via redirect. This server will close itself as soon as diff --git a/tool/extract_all_pub_dev.dart b/tool/extract_all_pub_dev.dart index 474c89794..28e372532 100644 --- a/tool/extract_all_pub_dev.dart +++ b/tool/extract_all_pub_dev.dart @@ -20,13 +20,13 @@ const statusFilename = 'extract_all_pub_status.json'; Future> allPackageNames() async { var nextUrl = Uri.https('pub.dev', 'api/packages?compact=1'); - final result = json.decode(await httpClient.read(nextUrl)); + final result = json.decode(await globalHttpClient.read(nextUrl)); return List.from(result['packages']); } Future> versionArchiveUrls(String packageName) async { final url = Uri.https('pub.dev', 'api/packages/$packageName'); - final result = json.decode(await httpClient.read(url)); + final result = json.decode(await globalHttpClient.read(url)); return List.from(result['versions'].map((v) => v['archive_url'])); } @@ -81,7 +81,7 @@ Future main() async { log.message('downloading $archiveUrl'); http.StreamedResponse response; try { - response = await httpClient + response = await globalHttpClient .send(http.Request('GET', Uri.parse(archiveUrl))); await extractTarGz(response.stream, tempDir); log.message('Extracted $archiveUrl'); From c11c00865d989ac3f46fbe3c781b587047b707c6 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 2 Nov 2022 11:38:23 -0700 Subject: [PATCH 14/31] Take request metadata out of _PubHttpClient --- lib/src/command/lish.dart | 10 +++--- lib/src/http.dart | 39 +-------------------- lib/src/source/hosted.dart | 65 ++++++++++++++++++++++++++++++++--- tool/extract_all_pub_dev.dart | 14 +++++--- 4 files changed, 77 insertions(+), 51 deletions(-) diff --git a/lib/src/command/lish.dart b/lib/src/command/lish.dart index df0c75930..57af146eb 100644 --- a/lib/src/command/lish.dart +++ b/lib/src/command/lish.dart @@ -18,7 +18,7 @@ import '../io.dart'; import '../log.dart' as log; import '../oauth2.dart' as oauth2; import '../solver/type.dart'; -import '../source/hosted.dart' show validateAndNormalizeHostedUrl; +import '../source/hosted.dart' show HostedSource, validateAndNormalizeHostedUrl; import '../utils.dart'; import '../validator.dart'; @@ -94,7 +94,8 @@ class LishCommand extends PubCommand { try { await log.progress('Uploading', () async { var newUri = host.resolve('api/packages/versions/new'); - var response = await client.get(newUri, headers: pubApiHeaders); + var response = await client.get(newUri, + headers: HostedSource.httpRequestHeadersFor(newUri)); var parameters = parseJsonResponse(response); var url = _expectField(parameters, 'url', response); @@ -119,8 +120,9 @@ class LishCommand extends PubCommand { var location = postResponse.headers['location']; if (location == null) throw PubHttpResponseException(postResponse); - handleJsonSuccess( - await client.get(Uri.parse(location), headers: pubApiHeaders)); + final locationUri = Uri.parse(location); + handleJsonSuccess(await client.get(locationUri, + headers: HostedSource.httpRequestHeadersFor(locationUri))); }); } on AuthenticationException catch (error) { var msg = ''; diff --git a/lib/src/http.dart b/lib/src/http.dart index 4462b43f7..4f75d9d03 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -11,12 +11,9 @@ import 'dart:math' as math; import 'package:http/http.dart' as http; import 'package:pool/pool.dart'; -import 'command.dart'; -import 'io.dart'; import 'log.dart' as log; import 'package.dart'; import 'sdk.dart'; -import 'source/hosted.dart'; import 'utils.dart'; /// Headers and field names that should be censored in the log output. @@ -30,7 +27,7 @@ const _censoredFields = ['refresh_token', 'authorization']; const pubApiHeaders = {'Accept': 'application/vnd.pub.v2+json'}; /// A unique ID to identify this particular invocation of pub. -final _sessionId = createUuid(); +final sessionId = createUuid(); /// An HTTP client that transforms 40* errors and socket exceptions into more /// user-friendly error messages. @@ -43,22 +40,6 @@ class _PubHttpClient extends http.BaseClient { @override Future send(http.BaseRequest request) async { - if (_shouldAddMetadata(request)) { - request.headers['X-Pub-OS'] = Platform.operatingSystem; - request.headers['X-Pub-Command'] = PubCommand.command; - request.headers['X-Pub-Session-ID'] = _sessionId; - - var environment = Platform.environment['PUB_ENVIRONMENT']; - if (environment != null) { - request.headers['X-Pub-Environment'] = environment; - } - - var type = Zone.current[#_dependencyType]; - if (type != null && type != DependencyType.none) { - request.headers['X-Pub-Reason'] = type.toString(); - } - } - _requestStopwatches[request] = Stopwatch()..start(); request.headers[HttpHeaders.userAgentHeader] = 'Dart pub ${sdk.version}'; _logRequest(request); @@ -70,24 +51,6 @@ class _PubHttpClient extends http.BaseClient { return streamedResponse; } - /// Whether extra metadata headers should be added to [request]. - bool _shouldAddMetadata(http.BaseRequest request) { - if (runningFromTest && Platform.environment.containsKey('PUB_HOSTED_URL')) { - if (request.url.origin != Platform.environment['PUB_HOSTED_URL']) { - return false; - } - } else { - if (!HostedSource.isPubDevUrl(request.url.toString())) return false; - } - - if (Platform.environment.containsKey('CI') && - Platform.environment['CI'] != 'false') { - return false; - } - - return true; - } - /// Logs the fact that [request] was sent, and information about it. void _logRequest(http.BaseRequest request) { var requestLog = StringBuffer(); diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index af84d021f..c202c44b9 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -5,6 +5,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io' as io; +import 'dart:io'; import 'dart:typed_data'; import 'package:collection/collection.dart' @@ -17,6 +18,7 @@ import 'package:pub_semver/pub_semver.dart'; import 'package:stack_trace/stack_trace.dart'; import '../authentication/client.dart'; +import '../command.dart'; import '../crc32c.dart'; import '../exceptions.dart'; import '../http.dart'; @@ -166,6 +168,57 @@ class HostedSource extends CachedSource { } }(); + /// Whether extra metadata headers should be sent for HTTP requests to a given + /// [url]. + static bool _shouldSendRequestMetadata(Uri url) { + if (runningFromTest && Platform.environment.containsKey('PUB_HOSTED_URL')) { + if (url.origin != Platform.environment['PUB_HOSTED_URL']) { + return false; + } + } else { + if (!HostedSource.isPubDevUrl(url.toString())) return false; + } + + if (Platform.environment.containsKey('CI') && + Platform.environment['CI'] != 'false') { + return false; + } + + return true; + } + + /// Adds request metadata headers to a [Map] of HTTP headers. The headers + /// contain information about the Pub tool's environment and the currently + /// running command. + static void attachRequestMetadata(Map headers) { + headers['X-Pub-OS'] = Platform.operatingSystem; + headers['X-Pub-Command'] = PubCommand.command; + headers['X-Pub-Session-ID'] = sessionId; + + var environment = Platform.environment['PUB_ENVIRONMENT']; + if (environment != null) { + headers['X-Pub-Environment'] = environment; + } + + var type = Zone.current[#_dependencyType]; + if (type != null && type != DependencyType.none) { + headers['X-Pub-Reason'] = type.toString(); + } + } + + /// Creates a [Map] of HTTP headers that include the Pub API version "Accept" + /// header and, if determined necessary from the environment and the passed + /// [url], metadata headers. + static Map httpRequestHeadersFor(Uri url) { + final headers = {...pubApiHeaders}; + + if (_shouldSendRequestMetadata(url)) { + attachRequestMetadata(headers); + } + + return headers; + } + /// Returns a reference to a hosted package named [name]. /// /// If [url] is passed, it's the URL of the pub server from which the package @@ -373,12 +426,12 @@ class HostedSource extends CachedSource { final List<_VersionInfo> result; try { // TODO(sigurdm): Implement cancellation of requests. This probably - // requires resolution of: https://github.com/dart-lang/sdk/issues/22265. + // requires resolution of: https://github.com/dart-lang/http/issues/424. bodyText = await withAuthenticatedClient( - cache, - Uri.parse(hostedUrl), - (client) => client.read(url, headers: pubApiHeaders), - ); + cache, + Uri.parse(hostedUrl), + (client) => client.read(url, + headers: HostedSource.httpRequestHeadersFor(url))); final decoded = jsonDecode(bodyText); if (decoded is! Map) { throw FormatException('version listing must be a mapping'); @@ -1053,6 +1106,8 @@ See $contentHashesDocumentationUrl. // [PubHttpException]. await retryForHttp('downloading "$archiveUrl"', () async { final request = http.Request('GET', archiveUrl); + request.headers + .addAll(HostedSource.httpRequestHeadersFor(archiveUrl)); final response = await client.send(request); response.throwIfNotOk(); diff --git a/tool/extract_all_pub_dev.dart b/tool/extract_all_pub_dev.dart index 28e372532..aad3797cb 100644 --- a/tool/extract_all_pub_dev.dart +++ b/tool/extract_all_pub_dev.dart @@ -15,18 +15,21 @@ import 'package:pool/pool.dart'; import 'package:pub/src/http.dart'; import 'package:pub/src/io.dart'; import 'package:pub/src/log.dart' as log; +import 'package:pub/src/source/hosted.dart'; const statusFilename = 'extract_all_pub_status.json'; Future> allPackageNames() async { var nextUrl = Uri.https('pub.dev', 'api/packages?compact=1'); - final result = json.decode(await globalHttpClient.read(nextUrl)); + final result = json.decode(await globalHttpClient.read(nextUrl, + headers: HostedSource.httpRequestHeadersFor(nextUrl))); return List.from(result['packages']); } Future> versionArchiveUrls(String packageName) async { final url = Uri.https('pub.dev', 'api/packages/$packageName'); - final result = json.decode(await globalHttpClient.read(url)); + final result = json.decode(await globalHttpClient.read(url, + headers: HostedSource.httpRequestHeadersFor(url))); return List.from(result['versions'].map((v) => v['archive_url'])); } @@ -81,8 +84,11 @@ Future main() async { log.message('downloading $archiveUrl'); http.StreamedResponse response; try { - response = await globalHttpClient - .send(http.Request('GET', Uri.parse(archiveUrl))); + final archiveUri = Uri.parse(archiveUrl); + final request = http.Request('GET', archiveUri); + request.headers + .addAll(HostedSource.httpRequestHeadersFor(archiveUri)); + response = await globalHttpClient.send(request); await extractTarGz(response.stream, tempDir); log.message('Extracted $archiveUrl'); } catch (e) { From 60edd81c55ab2cbb7e81718b1deefe94c3af854d Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 2 Nov 2022 11:48:13 -0700 Subject: [PATCH 15/31] Use RetryClient for OAuth2 calls --- lib/src/oauth2.dart | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/src/oauth2.dart b/lib/src/oauth2.dart index b32157da7..fedcf7835 100644 --- a/lib/src/oauth2.dart +++ b/lib/src/oauth2.dart @@ -6,6 +6,7 @@ import 'dart:async'; import 'dart:io'; import 'package:collection/collection.dart' show IterableExtension; +import 'package:http/retry.dart'; import 'package:oauth2/oauth2.dart'; import 'package:path/path.dart' as path; import 'package:shelf/shelf.dart' as shelf; @@ -17,6 +18,11 @@ import 'log.dart' as log; import 'system_cache.dart'; import 'utils.dart'; +/// The global HTTP client with basic retries. Used instead of retryForHttp for +/// OAuth calls because the OAuth2 package requires a client to be passed. While +/// the retry logic is more basic, this is fine for the publishing process. +final _retryHttpClient = RetryClient(globalHttpClient); + /// The pub client's OAuth2 identifier. const _identifier = '818368855108-8grd2eg9tj9f38os6f1urbcvsq399u8n.apps.' 'googleusercontent.com'; @@ -142,7 +148,7 @@ Future _getClient(SystemCache cache) async { secret: _secret, // Google's OAuth2 API doesn't support basic auth. basicAuth: false, - httpClient: globalHttpClient); + httpClient: _retryHttpClient); _saveCredentials(cache, client.credentials); return client; } @@ -221,7 +227,7 @@ Future _authorize() async { secret: _secret, // Google's OAuth2 API doesn't support basic auth. basicAuth: false, - httpClient: globalHttpClient); + httpClient: _retryHttpClient); // Spin up a one-shot HTTP server to receive the authorization code from the // Google OAuth2 server via redirect. This server will close itself as soon as From 6d8e7be8bf9aa4a1d2c1a58fa2389aa367dfd11d Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 2 Nov 2022 14:17:46 -0700 Subject: [PATCH 16/31] Retrigger checks From 72e47c994727aa03ed7eec374a8083201e5a2edc Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 2 Nov 2022 15:13:10 -0700 Subject: [PATCH 17/31] Wrap versions HTTP call with retryForHttp --- lib/src/source/hosted.dart | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index c202c44b9..61f369156 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -292,8 +292,6 @@ class HostedSource extends CachedSource { ); } - HostedDescription _asDescription(desc) => desc as HostedDescription; - /// Parses the description for a package. /// /// If the package parses correctly, this returns a (name, url) pair. If not, @@ -417,6 +415,7 @@ class HostedSource extends CachedSource { if (description is! HostedDescription) { throw ArgumentError('Wrong source'); } + final packageName = description.packageName; final hostedUrl = description.url; final url = _listVersionsUrl(ref); log.io('Get versions from $url.'); @@ -427,11 +426,14 @@ class HostedSource extends CachedSource { try { // TODO(sigurdm): Implement cancellation of requests. This probably // requires resolution of: https://github.com/dart-lang/http/issues/424. - bodyText = await withAuthenticatedClient( - cache, - Uri.parse(hostedUrl), - (client) => client.read(url, - headers: HostedSource.httpRequestHeadersFor(url))); + bodyText = await withAuthenticatedClient(cache, Uri.parse(hostedUrl), + (client) async { + return await retryForHttp( + 'fetching versions for "$packageName" from "$url"', () async { + return await client.read(url, + headers: HostedSource.httpRequestHeadersFor(url)); + }); + }); final decoded = jsonDecode(bodyText); if (decoded is! Map) { throw FormatException('version listing must be a mapping'); @@ -439,7 +441,6 @@ class HostedSource extends CachedSource { body = decoded; result = _versionInfoFromPackageListing(body, ref, url, cache); } on Exception catch (error, stackTrace) { - final packageName = _asDescription(ref.description).packageName; _throwFriendlyError(error, stackTrace, packageName, hostedUrl); } From b57b58fc796b6ed9ac55dd93670f40bee9e998a7 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 2 Nov 2022 15:44:34 -0700 Subject: [PATCH 18/31] Add more comments --- lib/src/http.dart | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index 4f75d9d03..b7653bff2 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -195,7 +195,7 @@ class PubHttpException implements Exception { PubHttpException(this.message, {this.isIntermittent = false}); } -/// Exception thrown when an HTTP response is not OK. +/// Exception thrown when an HTTP response is not Ok. class PubHttpResponseException extends PubHttpException { final http.Response response; @@ -216,6 +216,14 @@ class PubHttpResponseException extends PubHttpException { /// Program-wide limiter for concurrent network requests. final _httpPool = Pool(16); +/// Runs the provided function [fn] and returns the response. +/// +/// If there is an HTTP-related exception, an intermittent HTTP error response, +/// or an async timeout, [fn] is run repeatedly until there is a successful +/// response or at most seven total attempts have been made. If all attempts +/// fail, the final exception is re-thrown. +/// +/// Each attempt is run within a [Pool] configured with 16 maximum resources. Future retryForHttp(String operation, FutureOr Function() fn) async { return await retry( () async => await _httpPool.withResource(() async => await fn()), @@ -235,6 +243,13 @@ Future retryForHttp(String operation, FutureOr Function() fn) async { } extension Throwing on http.BaseResponse { + /// Throws [PubHttpResponseException], calls [fail], or does nothing depending + /// on the status code. + /// + /// If the code is in the 200 range, nothing is done. If the code is 408, 429, + /// or in the 500 range, [PubHttpResponseException] is thrown with + /// "isIntermittent" set to `true`. Otherwise, [PubHttpResponseException] is + /// thrown with "isIntermittent" set to `false`. void throwIfNotOk() { if (statusCode >= 200 && statusCode <= 299) { return; From 3f23c43064a64cba4a38b175dc630bee9eb3bedb Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Wed, 2 Nov 2022 16:30:09 -0700 Subject: [PATCH 19/31] Use "throwIfNotOk" in versions HTTP call --- lib/src/source/hosted.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 61f369156..b79d42490 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -430,8 +430,10 @@ class HostedSource extends CachedSource { (client) async { return await retryForHttp( 'fetching versions for "$packageName" from "$url"', () async { - return await client.read(url, + final response = await client.get(url, headers: HostedSource.httpRequestHeadersFor(url)); + response.throwIfNotOk(); + return response.body; }); }); final decoded = jsonDecode(bodyText); From e288e145803fb0f94af805719c3a8f7673808657 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 4 Nov 2022 15:54:54 -0700 Subject: [PATCH 20/31] Change PackageIntegrityException to always subclass as an intermittent PubHttpException --- lib/src/exceptions.dart | 3 ++- lib/src/source/hosted.dart | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/src/exceptions.dart b/lib/src/exceptions.dart index fc897e2d0..90c9d1aa9 100644 --- a/lib/src/exceptions.dart +++ b/lib/src/exceptions.dart @@ -108,7 +108,8 @@ class PackageNotFoundException extends WrappedException { /// A class for exceptions where a package's checksum could not be validated. class PackageIntegrityException extends PubHttpException { - PackageIntegrityException(super.message, {super.isIntermittent}); + PackageIntegrityException(String message) + : super(message, isIntermittent: true); } /// Returns whether [error] is a user-facing error object. diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index b79d42490..e63c2c2aa 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -1486,8 +1486,7 @@ int? _parseCrc32c(Map headers, String fileName) { log.exception(e, s); throw PackageIntegrityException( 'Package archive "$fileName" has a malformed CRC32C checksum in ' - 'its response headers', - isIntermittent: true); + 'its response headers'); } } } From d962237611fc314fae5735ce7965cd0bd844f779 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Fri, 4 Nov 2022 16:17:01 -0700 Subject: [PATCH 21/31] Configure OAuth2 HTTP retries --- lib/src/http.dart | 16 ++++++++++++---- lib/src/oauth2.dart | 4 +++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index b7653bff2..95e446244 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -213,6 +213,17 @@ class PubHttpResponseException extends PubHttpException { } } +/// Whether [e] is one of a few HTTP-related exceptions that subclass +/// [IOException]. Can be used if your try-catch block contains various +/// operations in addition to HTTP calls and so a [IOException] instance check +/// would be too coarse. +bool isHttpIOException(Object e) { + return e is HttpException || + e is TlsException || + e is SocketException || + e is WebSocketException; +} + /// Program-wide limiter for concurrent network requests. final _httpPool = Pool(16); @@ -230,10 +241,7 @@ Future retryForHttp(String operation, FutureOr Function() fn) async { retryIf: (e) async => (e is PubHttpException && e.isIntermittent) || e is TimeoutException || - e is HttpException || - e is TlsException || - e is SocketException || - e is WebSocketException, + isHttpIOException(e), onRetry: (exception, retryCount) async => log.io('Retry #${retryCount + 1} for $operation'), maxAttempts: math.max( diff --git a/lib/src/oauth2.dart b/lib/src/oauth2.dart index fedcf7835..f2e048605 100644 --- a/lib/src/oauth2.dart +++ b/lib/src/oauth2.dart @@ -21,7 +21,9 @@ import 'utils.dart'; /// The global HTTP client with basic retries. Used instead of retryForHttp for /// OAuth calls because the OAuth2 package requires a client to be passed. While /// the retry logic is more basic, this is fine for the publishing process. -final _retryHttpClient = RetryClient(globalHttpClient); +final _retryHttpClient = RetryClient(globalHttpClient, + when: (response) => response.statusCode >= 500, + whenError: (e, _) => isHttpIOException(e)); /// The pub client's OAuth2 identifier. const _identifier = '818368855108-8grd2eg9tj9f38os6f1urbcvsq399u8n.apps.' From 26b12d1ecf0cfae64e08fe0236ebd7553bc14595 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Sun, 6 Nov 2022 14:48:31 -0800 Subject: [PATCH 22/31] Make metadata headers method an extension on http.Request and use .throwIfNotOk() in more places --- lib/src/command/lish.dart | 17 +++++++++----- lib/src/http.dart | 44 ++++++++++++++++++++++++++++++++++- lib/src/source/hosted.dart | 43 ++++------------------------------ tool/extract_all_pub_dev.dart | 19 +++++++++------ 4 files changed, 71 insertions(+), 52 deletions(-) diff --git a/lib/src/command/lish.dart b/lib/src/command/lish.dart index 57af146eb..0c6d8437c 100644 --- a/lib/src/command/lish.dart +++ b/lib/src/command/lish.dart @@ -94,8 +94,10 @@ class LishCommand extends PubCommand { try { await log.progress('Uploading', () async { var newUri = host.resolve('api/packages/versions/new'); - var response = await client.get(newUri, - headers: HostedSource.httpRequestHeadersFor(newUri)); + var publishRequest = http.Request('GET', newUri); + publishRequest.attachMetadataHeaders(); + var response = await client.sendSync(publishRequest); + response.throwIfNotOk(); var parameters = parseJsonResponse(response); var url = _expectField(parameters, 'url', response); @@ -115,14 +117,17 @@ class LishCommand extends PubCommand { request.followRedirects = false; request.files.add(http.MultipartFile.fromBytes('file', packageBytes, filename: 'package.tar.gz')); - var postResponse = - await http.Response.fromStream(await client.send(request)); + var postResponse = await client.sendSync(request); + postResponse.throwIfNotOk(); var location = postResponse.headers['location']; if (location == null) throw PubHttpResponseException(postResponse); final locationUri = Uri.parse(location); - handleJsonSuccess(await client.get(locationUri, - headers: HostedSource.httpRequestHeadersFor(locationUri))); + var finalizeRequest = http.Request('GET', locationUri); + finalizeRequest.attachMetadataHeaders(); + var finalizeResponse = await client.sendSync(finalizeRequest); + finalizeResponse.throwIfNotOk(); + handleJsonSuccess(finalizeResponse); }); } on AuthenticationException catch (error) { var msg = ''; diff --git a/lib/src/http.dart b/lib/src/http.dart index 95e446244..a4c15ab31 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -10,10 +10,12 @@ import 'dart:math' as math; import 'package:http/http.dart' as http; import 'package:pool/pool.dart'; +import 'command.dart'; import 'log.dart' as log; import 'package.dart'; import 'sdk.dart'; +import 'source/hosted.dart'; import 'utils.dart'; /// Headers and field names that should be censored in the log output. @@ -27,7 +29,7 @@ const _censoredFields = ['refresh_token', 'authorization']; const pubApiHeaders = {'Accept': 'application/vnd.pub.v2+json'}; /// A unique ID to identify this particular invocation of pub. -final sessionId = createUuid(); +final _sessionId = createUuid(); /// An HTTP client that transforms 40* errors and socket exceptions into more /// user-friendly error messages. @@ -138,6 +140,35 @@ Future withDependencyType( return runZoned(callback, zoneValues: {#_dependencyType: type}); } +extension AttachHeaders on http.Request { + /// Adds request metadata headers if the request URL indicates the destination + /// is a Hosted Pub Repository. Additional information about the Pub tool's + /// environment and the currently running command is sent depending on the + /// environment and the URL. + void attachMetadataHeaders() { + // Always include the Pub API version "Accept" header. + headers.addAll(pubApiHeaders); + + if (!HostedSource.shouldSendAdditionalMetadataFor(url)) { + return; + } + + headers['X-Pub-OS'] = Platform.operatingSystem; + headers['X-Pub-Command'] = PubCommand.command; + headers['X-Pub-Session-ID'] = _sessionId; + + var environment = Platform.environment['PUB_ENVIRONMENT']; + if (environment != null) { + headers['X-Pub-Environment'] = environment; + } + + var type = Zone.current[#_dependencyType]; + if (type != null && type != DependencyType.none) { + headers['X-Pub-Reason'] = type.toString(); + } + } +} + /// Handles a successful JSON-formatted response from pub.dev. /// /// These responses are expected to be of the form `{"success": {"message": @@ -279,3 +310,14 @@ extension Throwing on http.BaseResponse { } } } + +extension SyncSending on http.Client { + /// Sends an HTTP request and synchronously returns the response. The regular + /// send method on [http.Client], which returns a [http.StreamedResponse], is + /// the only method that accepts a request object. This method can be used + /// when you need to send a request object but want a regular response object. + Future sendSync(http.BaseRequest request) async { + final streamedResponse = await send(request); + return await http.Response.fromStream(streamedResponse); + } +} diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index e63c2c2aa..469a2a36f 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -18,7 +18,6 @@ import 'package:pub_semver/pub_semver.dart'; import 'package:stack_trace/stack_trace.dart'; import '../authentication/client.dart'; -import '../command.dart'; import '../crc32c.dart'; import '../exceptions.dart'; import '../http.dart'; @@ -170,7 +169,7 @@ class HostedSource extends CachedSource { /// Whether extra metadata headers should be sent for HTTP requests to a given /// [url]. - static bool _shouldSendRequestMetadata(Uri url) { + static bool shouldSendAdditionalMetadataFor(Uri url) { if (runningFromTest && Platform.environment.containsKey('PUB_HOSTED_URL')) { if (url.origin != Platform.environment['PUB_HOSTED_URL']) { return false; @@ -187,38 +186,6 @@ class HostedSource extends CachedSource { return true; } - /// Adds request metadata headers to a [Map] of HTTP headers. The headers - /// contain information about the Pub tool's environment and the currently - /// running command. - static void attachRequestMetadata(Map headers) { - headers['X-Pub-OS'] = Platform.operatingSystem; - headers['X-Pub-Command'] = PubCommand.command; - headers['X-Pub-Session-ID'] = sessionId; - - var environment = Platform.environment['PUB_ENVIRONMENT']; - if (environment != null) { - headers['X-Pub-Environment'] = environment; - } - - var type = Zone.current[#_dependencyType]; - if (type != null && type != DependencyType.none) { - headers['X-Pub-Reason'] = type.toString(); - } - } - - /// Creates a [Map] of HTTP headers that include the Pub API version "Accept" - /// header and, if determined necessary from the environment and the passed - /// [url], metadata headers. - static Map httpRequestHeadersFor(Uri url) { - final headers = {...pubApiHeaders}; - - if (_shouldSendRequestMetadata(url)) { - attachRequestMetadata(headers); - } - - return headers; - } - /// Returns a reference to a hosted package named [name]. /// /// If [url] is passed, it's the URL of the pub server from which the package @@ -430,8 +397,9 @@ class HostedSource extends CachedSource { (client) async { return await retryForHttp( 'fetching versions for "$packageName" from "$url"', () async { - final response = await client.get(url, - headers: HostedSource.httpRequestHeadersFor(url)); + final request = http.Request('GET', url); + request.attachMetadataHeaders(); + final response = await client.sendSync(request); response.throwIfNotOk(); return response.body; }); @@ -1109,8 +1077,7 @@ See $contentHashesDocumentationUrl. // [PubHttpException]. await retryForHttp('downloading "$archiveUrl"', () async { final request = http.Request('GET', archiveUrl); - request.headers - .addAll(HostedSource.httpRequestHeadersFor(archiveUrl)); + request.attachMetadataHeaders(); final response = await client.send(request); response.throwIfNotOk(); diff --git a/tool/extract_all_pub_dev.dart b/tool/extract_all_pub_dev.dart index aad3797cb..e2cd2acf5 100644 --- a/tool/extract_all_pub_dev.dart +++ b/tool/extract_all_pub_dev.dart @@ -15,21 +15,26 @@ import 'package:pool/pool.dart'; import 'package:pub/src/http.dart'; import 'package:pub/src/io.dart'; import 'package:pub/src/log.dart' as log; -import 'package:pub/src/source/hosted.dart'; const statusFilename = 'extract_all_pub_status.json'; Future> allPackageNames() async { var nextUrl = Uri.https('pub.dev', 'api/packages?compact=1'); - final result = json.decode(await globalHttpClient.read(nextUrl, - headers: HostedSource.httpRequestHeadersFor(nextUrl))); + final request = http.Request('GET', nextUrl); + request.attachMetadataHeaders(); + final response = await globalHttpClient.sendSync(request); + response.throwIfNotOk(); + final result = json.decode(response.body); return List.from(result['packages']); } Future> versionArchiveUrls(String packageName) async { final url = Uri.https('pub.dev', 'api/packages/$packageName'); - final result = json.decode(await globalHttpClient.read(url, - headers: HostedSource.httpRequestHeadersFor(url))); + final request = http.Request('GET', url); + request.attachMetadataHeaders(); + final response = await globalHttpClient.sendSync(request); + response.throwIfNotOk(); + final result = json.decode(response.body); return List.from(result['versions'].map((v) => v['archive_url'])); } @@ -86,9 +91,9 @@ Future main() async { try { final archiveUri = Uri.parse(archiveUrl); final request = http.Request('GET', archiveUri); - request.headers - .addAll(HostedSource.httpRequestHeadersFor(archiveUri)); + request.attachMetadataHeaders(); response = await globalHttpClient.send(request); + response.throwIfNotOk(); await extractTarGz(response.stream, tempDir); log.message('Extracted $archiveUrl'); } catch (e) { From ccc3e30c418f73c0d9d55a7fa91e575d8e5e83f7 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 7 Nov 2022 14:10:00 -0800 Subject: [PATCH 23/31] Add retries to OIDC discovery document request --- lib/src/command/login.dart | 6 ++---- lib/src/http.dart | 2 +- lib/src/oauth2.dart | 23 +++++++++++++++++++++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/lib/src/command/login.dart b/lib/src/command/login.dart index 632b56e8b..074384546 100644 --- a/lib/src/command/login.dart +++ b/lib/src/command/login.dart @@ -7,7 +7,6 @@ import 'dart:convert'; import '../command.dart'; import '../command_runner.dart'; -import '../http.dart'; import '../log.dart' as log; import '../oauth2.dart' as oauth2; @@ -46,9 +45,8 @@ class LoginCommand extends PubCommand { Future<_UserInfo?> _retrieveUserInfo() async { return await oauth2.withClient(cache, (client) async { - final discovery = await globalHttpClient.get(Uri.https( - 'accounts.google.com', '/.well-known/openid-configuration')); - final userInfoEndpoint = json.decode(discovery.body)['userinfo_endpoint']; + final discovery = await oauth2.fetchOidcDiscoveryDocument(); + final userInfoEndpoint = discovery['userinfo_endpoint']; final userInfoRequest = await client.get(Uri.parse(userInfoEndpoint)); if (userInfoRequest.statusCode != 200) return null; try { diff --git a/lib/src/http.dart b/lib/src/http.dart index a4c15ab31..f6dd0b640 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -10,8 +10,8 @@ import 'dart:math' as math; import 'package:http/http.dart' as http; import 'package:pool/pool.dart'; -import 'command.dart'; +import 'command.dart'; import 'log.dart' as log; import 'package.dart'; import 'sdk.dart'; diff --git a/lib/src/oauth2.dart b/lib/src/oauth2.dart index f2e048605..23eeab1a2 100644 --- a/lib/src/oauth2.dart +++ b/lib/src/oauth2.dart @@ -6,6 +6,7 @@ import 'dart:async'; import 'dart:io'; import 'package:collection/collection.dart' show IterableExtension; +import 'package:http/http.dart' as http; import 'package:http/retry.dart'; import 'package:oauth2/oauth2.dart'; import 'package:path/path.dart' as path; @@ -34,6 +35,12 @@ const _identifier = '818368855108-8grd2eg9tj9f38os6f1urbcvsq399u8n.apps.' /// This isn't actually meant to be kept a secret. const _secret = 'SWeqj8seoJW0w7_CpEPFLX0K'; +/// The URL from which the pub client will retrieve Google's OIDC endpoint URIs. +/// +/// [Google OpenID Connect documentation]: https://developers.google.com/identity/openid-connect/openid-connect#discovery +final _oidcDiscoveryDocumentEndpoint = + Uri.https('accounts.google.com', '/.well-known/openid-configuration'); + /// The URL to which the user will be directed to authorize the pub client to /// get an OAuth2 access token. /// @@ -266,3 +273,19 @@ Future _authorize() async { log.message('Successfully authorized.\n'); return client; } + +/// Fetches Google's OpenID Connect Discovery document and parses the JSON +/// response body into a [Map]. +/// +/// See +/// https://developers.google.com/identity/openid-connect/openid-connect#discovery +Future fetchOidcDiscoveryDocument() async { + final discoveryResponse = await retryForHttp( + 'fetching Google\'s OpenID Connect Discovery document', () async { + final request = http.Request('GET', _oidcDiscoveryDocumentEndpoint); + final response = await globalHttpClient.sendSync(request); + response.throwIfNotOk(); + return response; + }); + return parseJsonResponse(discoveryResponse); +} From a686043cc85c794cc2f791489574ec2bc9976218 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 7 Nov 2022 15:53:21 -0800 Subject: [PATCH 24/31] Add some retries to upload command --- lib/src/command/lish.dart | 40 ++++++++++++++++++----------------- lib/src/http.dart | 33 ++++++++++++++++++++++++----- lib/src/oauth2.dart | 4 +--- lib/src/source/hosted.dart | 6 ++---- tool/extract_all_pub_dev.dart | 9 +++----- 5 files changed, 55 insertions(+), 37 deletions(-) diff --git a/lib/src/command/lish.dart b/lib/src/command/lish.dart index 0c6d8437c..efec14580 100644 --- a/lib/src/command/lish.dart +++ b/lib/src/command/lish.dart @@ -93,40 +93,42 @@ class LishCommand extends PubCommand { try { await log.progress('Uploading', () async { - var newUri = host.resolve('api/packages/versions/new'); - var publishRequest = http.Request('GET', newUri); - publishRequest.attachMetadataHeaders(); - var response = await client.sendSync(publishRequest); - response.throwIfNotOk(); - var parameters = parseJsonResponse(response); - - var url = _expectField(parameters, 'url', response); - if (url is! String) invalidServerResponse(response); + final parametersResponse = + await retryForHttp('initiating upload', () async { + final request = + http.Request('GET', host.resolve('api/packages/versions/new')); + request.attachMetadataHeaders(); + return await client.fetch(request); + }); + final parameters = parseJsonResponse(parametersResponse); + + var url = _expectField(parameters, 'url', parametersResponse); + if (url is! String) invalidServerResponse(parametersResponse); cloudStorageUrl = Uri.parse(url); // TODO(nweiz): Cloud Storage can provide an XML-formatted error. We // should report that error and exit. var request = http.MultipartRequest('POST', cloudStorageUrl!); - var fields = _expectField(parameters, 'fields', response); - if (fields is! Map) invalidServerResponse(response); + var fields = _expectField(parameters, 'fields', parametersResponse); + if (fields is! Map) invalidServerResponse(parametersResponse); fields.forEach((key, value) { - if (value is! String) invalidServerResponse(response); + if (value is! String) invalidServerResponse(parametersResponse); request.fields[key] = value; }); request.followRedirects = false; request.files.add(http.MultipartFile.fromBytes('file', packageBytes, filename: 'package.tar.gz')); - var postResponse = await client.sendSync(request); - postResponse.throwIfNotOk(); + var postResponse = await client.fetch(request); var location = postResponse.headers['location']; if (location == null) throw PubHttpResponseException(postResponse); - final locationUri = Uri.parse(location); - var finalizeRequest = http.Request('GET', locationUri); - finalizeRequest.attachMetadataHeaders(); - var finalizeResponse = await client.sendSync(finalizeRequest); - finalizeResponse.throwIfNotOk(); + final finalizeResponse = + await retryForHttp('finalizing upload', () async { + final request = http.Request('GET', Uri.parse(location)); + request.attachMetadataHeaders(); + return await client.fetch(request); + }); handleJsonSuccess(finalizeResponse); }); } on AuthenticationException catch (error) { diff --git a/lib/src/http.dart b/lib/src/http.dart index f6dd0b640..d6f653752 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -311,13 +311,36 @@ extension Throwing on http.BaseResponse { } } -extension SyncSending on http.Client { - /// Sends an HTTP request and synchronously returns the response. The regular - /// send method on [http.Client], which returns a [http.StreamedResponse], is - /// the only method that accepts a request object. This method can be used +extension RequestSending on http.Client { + /// Sends an HTTP request, validates the response headers, and if validation + /// is successful, reads the whole response body and returns it. + /// + /// The send method on [http.Client], which returns a [http.StreamedResponse], + /// is the only method that accepts a request object. This method can be used /// when you need to send a request object but want a regular response object. - Future sendSync(http.BaseRequest request) async { + /// + /// If false is passed for [throwIfNotOk], the response will not be validated. + /// See [http.BaseResponse.throwIfNotOk] extension for validation details. + Future fetch(http.BaseRequest request, + {bool throwIfNotOk = true}) async { final streamedResponse = await send(request); + if (throwIfNotOk) { + streamedResponse.throwIfNotOk(); + } return await http.Response.fromStream(streamedResponse); } + + /// Sends an HTTP request, validates the response headers, and if validation + /// is successful, returns a [http.StreamedResponse]. + /// + /// If false is passed for [throwIfNotOk], the response will not be validated. + /// See [http.BaseResponse.throwIfNotOk] extension for validation details. + Future fetchAsStream(http.BaseRequest request, + {bool throwIfNotOk = true}) async { + final streamedResponse = await send(request); + if (throwIfNotOk) { + streamedResponse.throwIfNotOk(); + } + return streamedResponse; + } } diff --git a/lib/src/oauth2.dart b/lib/src/oauth2.dart index 23eeab1a2..7f9b6ae6c 100644 --- a/lib/src/oauth2.dart +++ b/lib/src/oauth2.dart @@ -283,9 +283,7 @@ Future fetchOidcDiscoveryDocument() async { final discoveryResponse = await retryForHttp( 'fetching Google\'s OpenID Connect Discovery document', () async { final request = http.Request('GET', _oidcDiscoveryDocumentEndpoint); - final response = await globalHttpClient.sendSync(request); - response.throwIfNotOk(); - return response; + return await globalHttpClient.fetch(request); }); return parseJsonResponse(discoveryResponse); } diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index 469a2a36f..acab61e2e 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -399,8 +399,7 @@ class HostedSource extends CachedSource { 'fetching versions for "$packageName" from "$url"', () async { final request = http.Request('GET', url); request.attachMetadataHeaders(); - final response = await client.sendSync(request); - response.throwIfNotOk(); + final response = await client.fetch(request); return response.body; }); }); @@ -1078,8 +1077,7 @@ See $contentHashesDocumentationUrl. await retryForHttp('downloading "$archiveUrl"', () async { final request = http.Request('GET', archiveUrl); request.attachMetadataHeaders(); - final response = await client.send(request); - response.throwIfNotOk(); + final response = await client.fetchAsStream(request); Stream> stream = response.stream; final expectedCrc32c = _parseCrc32c(response.headers, fileName); diff --git a/tool/extract_all_pub_dev.dart b/tool/extract_all_pub_dev.dart index e2cd2acf5..ca90cd094 100644 --- a/tool/extract_all_pub_dev.dart +++ b/tool/extract_all_pub_dev.dart @@ -22,8 +22,7 @@ Future> allPackageNames() async { var nextUrl = Uri.https('pub.dev', 'api/packages?compact=1'); final request = http.Request('GET', nextUrl); request.attachMetadataHeaders(); - final response = await globalHttpClient.sendSync(request); - response.throwIfNotOk(); + final response = await globalHttpClient.fetch(request); final result = json.decode(response.body); return List.from(result['packages']); } @@ -32,8 +31,7 @@ Future> versionArchiveUrls(String packageName) async { final url = Uri.https('pub.dev', 'api/packages/$packageName'); final request = http.Request('GET', url); request.attachMetadataHeaders(); - final response = await globalHttpClient.sendSync(request); - response.throwIfNotOk(); + final response = await globalHttpClient.fetch(request); final result = json.decode(response.body); return List.from(result['versions'].map((v) => v['archive_url'])); } @@ -92,8 +90,7 @@ Future main() async { final archiveUri = Uri.parse(archiveUrl); final request = http.Request('GET', archiveUri); request.attachMetadataHeaders(); - response = await globalHttpClient.send(request); - response.throwIfNotOk(); + response = await globalHttpClient.fetchAsStream(request); await extractTarGz(response.stream, tempDir); log.message('Extracted $archiveUrl'); } catch (e) { From 19f5f01b90cc05acefa724daa3dd5d66df950f0d Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 7 Nov 2022 16:11:39 -0800 Subject: [PATCH 25/31] Add retries to upload package step --- lib/src/command/lish.dart | 42 ++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/lib/src/command/lish.dart b/lib/src/command/lish.dart index efec14580..cb0f6e33c 100644 --- a/lib/src/command/lish.dart +++ b/lib/src/command/lish.dart @@ -18,7 +18,7 @@ import '../io.dart'; import '../log.dart' as log; import '../oauth2.dart' as oauth2; import '../solver/type.dart'; -import '../source/hosted.dart' show HostedSource, validateAndNormalizeHostedUrl; +import '../source/hosted.dart' show validateAndNormalizeHostedUrl; import '../utils.dart'; import '../validator.dart'; @@ -93,6 +93,7 @@ class LishCommand extends PubCommand { try { await log.progress('Uploading', () async { + /// 1. Initiate upload final parametersResponse = await retryForHttp('initiating upload', () async { final request = @@ -102,29 +103,34 @@ class LishCommand extends PubCommand { }); final parameters = parseJsonResponse(parametersResponse); + /// 2. Upload package var url = _expectField(parameters, 'url', parametersResponse); if (url is! String) invalidServerResponse(parametersResponse); cloudStorageUrl = Uri.parse(url); - // TODO(nweiz): Cloud Storage can provide an XML-formatted error. We - // should report that error and exit. - var request = http.MultipartRequest('POST', cloudStorageUrl!); - - var fields = _expectField(parameters, 'fields', parametersResponse); - if (fields is! Map) invalidServerResponse(parametersResponse); - fields.forEach((key, value) { - if (value is! String) invalidServerResponse(parametersResponse); - request.fields[key] = value; + final uploadResponse = + await retryForHttp('uploading package', () async { + // TODO(nweiz): Cloud Storage can provide an XML-formatted error. We + // should report that error and exit. + var request = http.MultipartRequest('POST', cloudStorageUrl!); + + var fields = _expectField(parameters, 'fields', parametersResponse); + if (fields is! Map) invalidServerResponse(parametersResponse); + fields.forEach((key, value) { + if (value is! String) invalidServerResponse(parametersResponse); + request.fields[key] = value; + }); + + request.followRedirects = false; + request.files.add(http.MultipartFile.fromBytes('file', packageBytes, + filename: 'package.tar.gz')); + return await client.fetch(request); }); - request.followRedirects = false; - request.files.add(http.MultipartFile.fromBytes('file', packageBytes, - filename: 'package.tar.gz')); - var postResponse = await client.fetch(request); - - var location = postResponse.headers['location']; - if (location == null) throw PubHttpResponseException(postResponse); + /// 3. Finalize publish + var location = uploadResponse.headers['location']; + if (location == null) throw PubHttpResponseException(uploadResponse); final finalizeResponse = - await retryForHttp('finalizing upload', () async { + await retryForHttp('finalizing publish', () async { final request = http.Request('GET', Uri.parse(location)); request.attachMetadataHeaders(); return await client.fetch(request); From fde54cc36ac94b4095962ad385ff04bf98a32929 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Mon, 7 Nov 2022 16:13:17 -0800 Subject: [PATCH 26/31] Fix indentation --- lib/src/oauth2.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/src/oauth2.dart b/lib/src/oauth2.dart index 7f9b6ae6c..41eb5f5d4 100644 --- a/lib/src/oauth2.dart +++ b/lib/src/oauth2.dart @@ -277,8 +277,7 @@ Future _authorize() async { /// Fetches Google's OpenID Connect Discovery document and parses the JSON /// response body into a [Map]. /// -/// See -/// https://developers.google.com/identity/openid-connect/openid-connect#discovery +/// See https://developers.google.com/identity/openid-connect/openid-connect#discovery Future fetchOidcDiscoveryDocument() async { final discoveryResponse = await retryForHttp( 'fetching Google\'s OpenID Connect Discovery document', () async { From 7afe52cd1aa68c4546dcfe595a53d8890ed3b024 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 8 Nov 2022 15:40:38 -0800 Subject: [PATCH 27/31] Make PubHttpResponseException use BaseResponse --- lib/src/http.dart | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index d6f653752..00a3a45fb 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -189,7 +189,12 @@ void handleJsonSuccess(http.Response response) { /// These responses are expected to be of the form `{"error": {"message": "some /// message"}}`. If the format is correct, the message will be raised as an /// error; otherwise an [invalidServerResponse] error will be raised. -void handleJsonError(http.Response response) { +void handleJsonError(http.BaseResponse response) { + if (response is! http.Response) { + // Not likely to be a common code path, but necessary. + // See https://github.com/dart-lang/pub/pull/3590#discussion_r1012978108 + fail(log.red('Invalid server response')); + } var errorMap = parseJsonResponse(response); if (errorMap['error'] is! Map || !errorMap['error'].containsKey('message') || @@ -228,7 +233,7 @@ class PubHttpException implements Exception { /// Exception thrown when an HTTP response is not Ok. class PubHttpResponseException extends PubHttpException { - final http.Response response; + final http.BaseResponse response; PubHttpResponseException(this.response, {String message = '', bool isIntermittent = false}) @@ -302,18 +307,17 @@ extension Throwing on http.BaseResponse { statusCode == HttpStatus.tooManyRequests) { // Throw if the response indicates a server error or an intermittent // client error, but mark it as intermittent so it can be retried. - throw PubHttpResponseException(this as http.Response, - isIntermittent: true); + throw PubHttpResponseException(this, isIntermittent: true); } else { // Throw for all other status codes. - throw PubHttpResponseException(this as http.Response); + throw PubHttpResponseException(this); } } } extension RequestSending on http.Client { - /// Sends an HTTP request, validates the response headers, and if validation - /// is successful, reads the whole response body and returns it. + /// Sends an HTTP request, reads the whole response body, validates the + /// response headers, and if validation is successful, and returns it. /// /// The send method on [http.Client], which returns a [http.StreamedResponse], /// is the only method that accepts a request object. This method can be used @@ -324,10 +328,11 @@ extension RequestSending on http.Client { Future fetch(http.BaseRequest request, {bool throwIfNotOk = true}) async { final streamedResponse = await send(request); + final response = await http.Response.fromStream(streamedResponse); if (throwIfNotOk) { - streamedResponse.throwIfNotOk(); + response.throwIfNotOk(); } - return await http.Response.fromStream(streamedResponse); + return response; } /// Sends an HTTP request, validates the response headers, and if validation From a38f45a9132d41774f820b8e639688df5c2020db Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 8 Nov 2022 16:39:15 -0800 Subject: [PATCH 28/31] Fix get_test --- lib/src/http.dart | 8 +++++++- test/get/hosted/get_test.dart | 15 ++++++++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/src/http.dart b/lib/src/http.dart index 00a3a45fb..1dd0f95e5 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -229,6 +229,11 @@ class PubHttpException implements Exception { final bool isIntermittent; PubHttpException(this.message, {this.isIntermittent = false}); + + @override + String toString() { + return 'PubHttpException: $message'; + } } /// Exception thrown when an HTTP response is not Ok. @@ -241,7 +246,8 @@ class PubHttpResponseException extends PubHttpException { @override String toString() { - var temp = 'HTTP error ${response.statusCode}: ${response.reasonPhrase}'; + var temp = 'PubHttpResponseException: HTTP error ${response.statusCode} ' + '${response.reasonPhrase}'; if (message != '') { temp += ': $message'; } diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index 3f5362f0c..f75388823 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -139,11 +139,12 @@ void main() { }).create(); await pubGet( + exitCode: exit_codes.UNAVAILABLE, error: RegExp( r'''Package archive for foo 1.2.3 downloaded from "(.+)" has ''' r'''"x-goog-hash: crc32c=(\d+)", which doesn't match the checksum ''' r'''of the archive downloaded\.'''), - silent: contains('Retry #1 because of checksum error'), + silent: contains('Retry #1 for downloading'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -175,11 +176,11 @@ void main() { }).create(); await pubGet( - exitCode: exit_codes.DATA, + exitCode: exit_codes.UNAVAILABLE, error: contains( 'Package archive "foo-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #1 because of checksum error'), + silent: contains('Retry #1 for downloading'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -195,11 +196,11 @@ void main() { }).create(); await pubGet( - exitCode: exit_codes.DATA, + exitCode: exit_codes.UNAVAILABLE, error: contains( 'Package archive "bar-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #1 because of checksum error'), + silent: contains('Retry #1 for downloading'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -215,11 +216,11 @@ void main() { }).create(); await pubGet( - exitCode: exit_codes.DATA, + exitCode: exit_codes.UNAVAILABLE, error: contains( 'Package archive "baz-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #1 because of checksum error'), + silent: contains('Retry #1 for downloading'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, From afe16eeb3919be0c8d7117dfe92403f7c74a1fc5 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 8 Nov 2022 17:36:30 -0800 Subject: [PATCH 29/31] Fix more tests --- lib/src/authentication/client.dart | 19 +++++++----------- lib/src/http.dart | 20 +++++++++++++++---- .../fail_gracefully_on_url_resolve_test.dart | 3 ++- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/lib/src/authentication/client.dart b/lib/src/authentication/client.dart index 477dc3997..787f394c6 100644 --- a/lib/src/authentication/client.dart +++ b/lib/src/authentication/client.dart @@ -48,19 +48,14 @@ class _AuthenticatedClient extends http.BaseClient { await _credential!.getAuthorizationHeaderValue(); } - try { - final response = await _inner.send(request); - if (response.statusCode == 401) { - _detectInvalidCredentials = true; - _throwAuthException(response); - } - return response; - } on PubHttpResponseException catch (e) { - if (e.response.statusCode == 403) { - _throwAuthException(e.response); - } - rethrow; + final response = await _inner.send(request); + if (response.statusCode == 401) { + _detectInvalidCredentials = true; + } + if (response.statusCode == 401 || response.statusCode == 403) { + _throwAuthException(response); } + return response; } /// Throws [AuthenticationException] that includes response status code and diff --git a/lib/src/http.dart b/lib/src/http.dart index 1dd0f95e5..5636c88f6 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -293,16 +293,28 @@ Future retryForHttp(String operation, FutureOr Function() fn) async { } extension Throwing on http.BaseResponse { + /// See https://api.flutter.dev/flutter/dart-io/HttpClientRequest/followRedirects.html + static const _redirectStatusCodes = [ + HttpStatus.movedPermanently, + HttpStatus.movedTemporarily, + HttpStatus.seeOther, + HttpStatus.temporaryRedirect, + HttpStatus.permanentRedirect + ]; + /// Throws [PubHttpResponseException], calls [fail], or does nothing depending /// on the status code. /// - /// If the code is in the 200 range, nothing is done. If the code is 408, 429, - /// or in the 500 range, [PubHttpResponseException] is thrown with - /// "isIntermittent" set to `true`. Otherwise, [PubHttpResponseException] is - /// thrown with "isIntermittent" set to `false`. + /// If the code is in the 200 range or if its a 300 range redirect code, + /// nothing is done. If the code is 408, 429, or in the 500 range, + /// [PubHttpResponseException] is thrown with "isIntermittent" set to `true`. + /// Otherwise, [PubHttpResponseException] is thrown with "isIntermittent" set + /// to `false`. void throwIfNotOk() { if (statusCode >= 200 && statusCode <= 299) { return; + } else if (_redirectStatusCodes.contains(statusCode)) { + return; } else if (statusCode == HttpStatus.notAcceptable && request?.headers['Accept'] == pubApiHeaders['Accept']) { fail('Pub ${sdk.version} is incompatible with the current version of ' diff --git a/test/hosted/fail_gracefully_on_url_resolve_test.dart b/test/hosted/fail_gracefully_on_url_resolve_test.dart index 7a91e7abb..0dff9518d 100644 --- a/test/hosted/fail_gracefully_on_url_resolve_test.dart +++ b/test/hosted/fail_gracefully_on_url_resolve_test.dart @@ -20,7 +20,8 @@ void main() { ]).create(); await pubCommand(command, - error: 'Could not resolve URL "https://invalid-url.foo".', + error: 'Got socket error trying to find package foo at ' + 'https://invalid-url.foo.', exitCode: exit_codes.UNAVAILABLE, environment: { 'PUB_MAX_HTTP_RETRIES': '2', From 9ef523fe308592af230d341bc03011ee5153490b Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 8 Nov 2022 18:24:00 -0800 Subject: [PATCH 30/31] Fix SHA-256 tests --- lib/src/command.dart | 4 +++- lib/src/http.dart | 4 ++-- lib/src/utils.dart | 4 ++-- test/content_hash_test.dart | 3 ++- test/get/hosted/get_test.dart | 16 ++++++++-------- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/lib/src/command.dart b/lib/src/command.dart index fa5d03cb4..90f920d4a 100644 --- a/lib/src/command.dart +++ b/lib/src/command.dart @@ -253,7 +253,9 @@ and attaching the relevant parts of that log file. exception = exception.innerError!; } - if (exception is HttpException || + if (exception is PackageIntegrityException) { + return exit_codes.TEMP_FAIL; + } else if (exception is HttpException || exception is http.ClientException || exception is SocketException || exception is TlsException || diff --git a/lib/src/http.dart b/lib/src/http.dart index 5636c88f6..32c53bc2f 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -284,8 +284,8 @@ Future retryForHttp(String operation, FutureOr Function() fn) async { (e is PubHttpException && e.isIntermittent) || e is TimeoutException || isHttpIOException(e), - onRetry: (exception, retryCount) async => - log.io('Retry #${retryCount + 1} for $operation'), + onRetry: (exception, attemptNumber) async => + log.io('Attempt #$attemptNumber for $operation'), maxAttempts: math.max( 1, // Having less than 1 attempt doesn't make sense. int.tryParse(Platform.environment['PUB_MAX_HTTP_RETRIES'] ?? '') ?? 7, diff --git a/lib/src/utils.dart b/lib/src/utils.dart index db89c4d5a..0719bbb1d 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -693,7 +693,7 @@ Future retry( Duration maxDelay = const Duration(seconds: 30), int maxAttempts = 8, FutureOr Function(Exception)? retryIf, - FutureOr Function(Exception, int retryCount)? onRetry, + FutureOr Function(Exception, int attemptNumber)? onRetry, }) async { var attempt = 0; // ignore: literal_only_boolean_expressions @@ -707,7 +707,7 @@ Future retry( } if (onRetry != null) { - await onRetry(e, attempt - 1); + await onRetry(e, attempt + 1); } } diff --git a/test/content_hash_test.dart b/test/content_hash_test.dart index 6a0725bf7..2485918c0 100644 --- a/test/content_hash_test.dart +++ b/test/content_hash_test.dart @@ -54,7 +54,8 @@ Future main() async { 'e7a7a0f6d9873e4c40cf68cc3cc9ca5b6c8cef6a2220241bdada4b9cb0083279'); await appDir({'foo': 'any'}).create(); await pubGet( - silent: contains('Retry #2'), + exitCode: exit_codes.TEMP_FAIL, + silent: contains('Attempt #2'), error: contains('Downloaded archive for foo-1.0.0 had wrong content-hash.'), environment: { diff --git a/test/get/hosted/get_test.dart b/test/get/hosted/get_test.dart index f75388823..837fb32dc 100644 --- a/test/get/hosted/get_test.dart +++ b/test/get/hosted/get_test.dart @@ -139,12 +139,12 @@ void main() { }).create(); await pubGet( - exitCode: exit_codes.UNAVAILABLE, + exitCode: exit_codes.TEMP_FAIL, error: RegExp( r'''Package archive for foo 1.2.3 downloaded from "(.+)" has ''' r'''"x-goog-hash: crc32c=(\d+)", which doesn't match the checksum ''' r'''of the archive downloaded\.'''), - silent: contains('Retry #1 for downloading'), + silent: contains('Attempt #2'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -176,11 +176,11 @@ void main() { }).create(); await pubGet( - exitCode: exit_codes.UNAVAILABLE, + exitCode: exit_codes.TEMP_FAIL, error: contains( 'Package archive "foo-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #1 for downloading'), + silent: contains('Attempt #2'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -196,11 +196,11 @@ void main() { }).create(); await pubGet( - exitCode: exit_codes.UNAVAILABLE, + exitCode: exit_codes.TEMP_FAIL, error: contains( 'Package archive "bar-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #1 for downloading'), + silent: contains('Attempt #2'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, @@ -216,11 +216,11 @@ void main() { }).create(); await pubGet( - exitCode: exit_codes.UNAVAILABLE, + exitCode: exit_codes.TEMP_FAIL, error: contains( 'Package archive "baz-1.2.3.tar.gz" has a malformed CRC32C ' 'checksum in its response headers'), - silent: contains('Retry #1 for downloading'), + silent: contains('Attempt #2'), environment: { 'PUB_MAX_HTTP_RETRIES': '2', }, From d29d8eef8da7beb1c0cab4f91a3a686f3a57fd82 Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Tue, 8 Nov 2022 18:51:21 -0800 Subject: [PATCH 31/31] Only send pub.dev API Accept header when necessary and fix tests --- lib/src/command/lish.dart | 2 ++ lib/src/http.dart | 13 +++++++------ lib/src/source/hosted.dart | 1 + test/add/hosted/non_default_pub_server_test.dart | 3 ++- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/src/command/lish.dart b/lib/src/command/lish.dart index cb0f6e33c..7090412ec 100644 --- a/lib/src/command/lish.dart +++ b/lib/src/command/lish.dart @@ -98,6 +98,7 @@ class LishCommand extends PubCommand { await retryForHttp('initiating upload', () async { final request = http.Request('GET', host.resolve('api/packages/versions/new')); + request.attachPubApiHeaders(); request.attachMetadataHeaders(); return await client.fetch(request); }); @@ -132,6 +133,7 @@ class LishCommand extends PubCommand { final finalizeResponse = await retryForHttp('finalizing publish', () async { final request = http.Request('GET', Uri.parse(location)); + request.attachPubApiHeaders(); request.attachMetadataHeaders(); return await client.fetch(request); }); diff --git a/lib/src/http.dart b/lib/src/http.dart index 32c53bc2f..ccd514610 100644 --- a/lib/src/http.dart +++ b/lib/src/http.dart @@ -141,14 +141,15 @@ Future withDependencyType( } extension AttachHeaders on http.Request { - /// Adds request metadata headers if the request URL indicates the destination - /// is a Hosted Pub Repository. Additional information about the Pub tool's - /// environment and the currently running command is sent depending on the - /// environment and the URL. - void attachMetadataHeaders() { - // Always include the Pub API version "Accept" header. + /// Adds headers required for pub.dev API requests. + void attachPubApiHeaders() { headers.addAll(pubApiHeaders); + } + /// Adds request metadata headers about the Pub tool's environment and the + /// currently running command if the request URL indicates the destination is + /// a Hosted Pub Repository. + void attachMetadataHeaders() { if (!HostedSource.shouldSendAdditionalMetadataFor(url)) { return; } diff --git a/lib/src/source/hosted.dart b/lib/src/source/hosted.dart index acab61e2e..769c9040a 100644 --- a/lib/src/source/hosted.dart +++ b/lib/src/source/hosted.dart @@ -398,6 +398,7 @@ class HostedSource extends CachedSource { return await retryForHttp( 'fetching versions for "$packageName" from "$url"', () async { final request = http.Request('GET', url); + request.attachPubApiHeaders(); request.attachMetadataHeaders(); final response = await client.fetch(request); return response.body; diff --git a/test/add/hosted/non_default_pub_server_test.dart b/test/add/hosted/non_default_pub_server_test.dart index 787bf32e8..f93f292cf 100644 --- a/test/add/hosted/non_default_pub_server_test.dart +++ b/test/add/hosted/non_default_pub_server_test.dart @@ -91,7 +91,8 @@ void main() { await pubAdd( args: ['foo', '--hosted-url', 'https://invalid-url.foo'], - error: contains('Could not resolve URL "https://invalid-url.foo".'), + error: contains('Got socket error trying to find package foo at ' + 'https://invalid-url.foo.'), exitCode: exit_codes.DATA, environment: { // Limit the retries - the url will never go valid.