From 7a330d296e6b349ab0a7e1613ecb8ee705f15f5f Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Fri, 2 May 2025 17:16:50 +0200 Subject: [PATCH 1/5] Use new HTTP GET retry logic in SearchClient. --- app/lib/search/search_client.dart | 27 ++++++++++------- pkg/_pub_shared/lib/utils/http.dart | 45 +++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/app/lib/search/search_client.dart b/app/lib/search/search_client.dart index 56a5b5c0c6..f199a3a549 100644 --- a/app/lib/search/search_client.dart +++ b/app/lib/search/search_client.dart @@ -8,7 +8,6 @@ import 'dart:convert'; import 'package:_pub_shared/utils/http.dart'; import 'package:clock/clock.dart'; import 'package:gcloud/service_scope.dart' as ss; -import 'package:http/http.dart' as http; import '../../../service/rate_limit/rate_limit.dart'; import '../shared/configuration.dart'; @@ -33,16 +32,13 @@ SearchClient get searchClient => ss.lookup(#_searchClient) as SearchClient; /// indexed data. class SearchClient { /// The HTTP client used for making calls to our search service. - final http.Client _httpClient; + final _httpClient = httpRetryClient(); /// Before this timestamp we may use the fallback search service URL, which /// is the unversioned service URL, potentially getting responses from an /// older instance. final _fallbackSearchThreshold = clock.now().add(Duration(minutes: 30)); - SearchClient([http.Client? client]) - : _httpClient = client ?? httpRetryClient(retries: 3); - /// Calls the search service (or uses cache) to serve the [query]. Future search( ServiceSearchQuery query, { @@ -69,16 +65,25 @@ class SearchClient { skipCache = true; } - // returns null on timeout (after 5 seconds) - Future doCallHttpServiceEndpoint({String? prefix}) async { + // Returns the status code and the body of the last response, or null on timeout. + Future<({int statusCode, String? body})?> doCallHttpServiceEndpoint( + {String? prefix}) async { final httpHostPort = prefix ?? activeConfiguration.searchServicePrefix; final serviceUrl = '$httpHostPort/search$serviceUrlParams'; try { - return await _httpClient - .get(Uri.parse(serviceUrl), headers: cloudTraceHeaders()) - .timeout(Duration(seconds: 5)); + return await httpGetWithRetry( + Uri.parse(serviceUrl), + client: _httpClient, + headers: cloudTraceHeaders(), + timeout: Duration(seconds: 5), + retryIf: (e) => (e is UnexpectedStatusException && + e.statusCode == searchIndexNotReadyCode), + responseFn: (rs) => (statusCode: rs.statusCode, body: rs.body), + ); } on TimeoutException { return null; + } on UnexpectedStatusException catch (e) { + return (statusCode: e.statusCode, body: null); } } @@ -103,7 +108,7 @@ class SearchClient { } if (response.statusCode == 200) { return PackageSearchResult.fromJson( - json.decode(response.body) as Map, + json.decode(response.body!) as Map, ); } // Search request before the service initialization completed. diff --git a/pkg/_pub_shared/lib/utils/http.dart b/pkg/_pub_shared/lib/utils/http.dart index 5876eb468b..d3c9279309 100644 --- a/pkg/_pub_shared/lib/utils/http.dart +++ b/pkg/_pub_shared/lib/utils/http.dart @@ -47,23 +47,41 @@ Future httpGetWithRetry( Uri uri, { required FutureOr Function(http.Response response) responseFn, int maxAttempts = 3, + + /// The HTTP client to use. + /// + /// Note: when the client is specified, the inner loop will not create a new client object on retries. + http.Client? client, + Map? headers, + + /// Per-request time amount that will be applied on the overall HTTP request. + Duration? timeout, + + /// Additional retry conditions (on top of the default ones). + bool Function(Exception e)? retryIf, }) async { return await retry( () async { - final client = http.Client(); + final closeClient = client == null; + final effectiveClient = client ?? http.Client(); try { - final rs = await client.get(uri); + var f = effectiveClient.get(uri, headers: headers); + if (timeout != null) { + f = f.timeout(timeout); + } + final rs = await f; if (rs.statusCode == 200) { return responseFn(rs); } - throw http.ClientException( - 'Unexpected status code for $uri: ${rs.statusCode}.'); + throw UnexpectedStatusException(rs.statusCode, uri); } finally { - client.close(); + if (closeClient) { + effectiveClient.close(); + } } }, maxAttempts: maxAttempts, - retryIf: _retryIf, + retryIf: (e) => _retryIf(e) || (retryIf != null && retryIf(e)), ); } @@ -77,5 +95,20 @@ bool _retryIf(Exception e) { if (e is http.ClientException) { return true; // HTTP issues are worth retrying } + if (e is UnexpectedStatusException) { + return _transientStatusCodes.contains(e.statusCode); + } return false; } + +/// Thrown when status code is not 200. +class UnexpectedStatusException implements Exception { + final int statusCode; + final String message; + + UnexpectedStatusException(this.statusCode, Uri uri) + : message = 'Unexpected status code for $uri: $statusCode.'; + + @override + String toString() => 'UnexpectedStatusException: $message'; +} From f9ced18e5371ca53052c2cafbf52439eaed1fef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Istv=C3=A1n=20So=C3=B3s?= Date: Mon, 5 May 2025 10:42:36 +0200 Subject: [PATCH 2/5] Update pkg/_pub_shared/lib/utils/http.dart Co-authored-by: Sigurd Meldgaard --- pkg/_pub_shared/lib/utils/http.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/_pub_shared/lib/utils/http.dart b/pkg/_pub_shared/lib/utils/http.dart index d3c9279309..cac0ac586f 100644 --- a/pkg/_pub_shared/lib/utils/http.dart +++ b/pkg/_pub_shared/lib/utils/http.dart @@ -50,7 +50,7 @@ Future httpGetWithRetry( /// The HTTP client to use. /// - /// Note: when the client is specified, the inner loop will not create a new client object on retries. + /// Note: when the client is not specified, the inner loop will create a new [Client] object on each retry attempt. http.Client? client, Map? headers, From 4c72662102931e5bc61be2162358940c4cc6078d Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Mon, 5 May 2025 11:06:47 +0200 Subject: [PATCH 3/5] rename to perRequestTimeout --- app/lib/search/search_client.dart | 2 +- pkg/_pub_shared/lib/utils/http.dart | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/lib/search/search_client.dart b/app/lib/search/search_client.dart index f199a3a549..a87fa37bbd 100644 --- a/app/lib/search/search_client.dart +++ b/app/lib/search/search_client.dart @@ -75,7 +75,7 @@ class SearchClient { Uri.parse(serviceUrl), client: _httpClient, headers: cloudTraceHeaders(), - timeout: Duration(seconds: 5), + perRequestTimeout: Duration(seconds: 5), retryIf: (e) => (e is UnexpectedStatusException && e.statusCode == searchIndexNotReadyCode), responseFn: (rs) => (statusCode: rs.statusCode, body: rs.body), diff --git a/pkg/_pub_shared/lib/utils/http.dart b/pkg/_pub_shared/lib/utils/http.dart index cac0ac586f..c0359ab87f 100644 --- a/pkg/_pub_shared/lib/utils/http.dart +++ b/pkg/_pub_shared/lib/utils/http.dart @@ -55,7 +55,7 @@ Future httpGetWithRetry( Map? headers, /// Per-request time amount that will be applied on the overall HTTP request. - Duration? timeout, + Duration? perRequestTimeout, /// Additional retry conditions (on top of the default ones). bool Function(Exception e)? retryIf, @@ -66,8 +66,8 @@ Future httpGetWithRetry( final effectiveClient = client ?? http.Client(); try { var f = effectiveClient.get(uri, headers: headers); - if (timeout != null) { - f = f.timeout(timeout); + if (perRequestTimeout != null) { + f = f.timeout(perRequestTimeout); } final rs = await f; if (rs.statusCode == 200) { From 6635727786da1852e89dbb25f6f763326524d7ef Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Mon, 5 May 2025 11:07:06 +0200 Subject: [PATCH 4/5] fix doc reference --- pkg/_pub_shared/lib/utils/http.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/_pub_shared/lib/utils/http.dart b/pkg/_pub_shared/lib/utils/http.dart index c0359ab87f..63ec054732 100644 --- a/pkg/_pub_shared/lib/utils/http.dart +++ b/pkg/_pub_shared/lib/utils/http.dart @@ -50,7 +50,7 @@ Future httpGetWithRetry( /// The HTTP client to use. /// - /// Note: when the client is not specified, the inner loop will create a new [Client] object on each retry attempt. + /// Note: when the client is not specified, the inner loop will create a new [http.Client] object on each retry attempt. http.Client? client, Map? headers, From dd331eb30c960df80a5d3ee578541ae66d08b9e2 Mon Sep 17 00:00:00 2001 From: Istvan Soos Date: Mon, 5 May 2025 11:13:32 +0200 Subject: [PATCH 5/5] additional note --- pkg/_pub_shared/lib/utils/http.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/_pub_shared/lib/utils/http.dart b/pkg/_pub_shared/lib/utils/http.dart index 63ec054732..81ecf0efa6 100644 --- a/pkg/_pub_shared/lib/utils/http.dart +++ b/pkg/_pub_shared/lib/utils/http.dart @@ -58,6 +58,7 @@ Future httpGetWithRetry( Duration? perRequestTimeout, /// Additional retry conditions (on top of the default ones). + /// Note: check for [UnexpectedStatusException] to allow non-200 response status codes. bool Function(Exception e)? retryIf, }) async { return await retry(