Use new HTTP GET retry logic in SearchClient. (#8732)

Author: isoos

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<PackageSearchResult> search(
     ServiceSearchQuery query, {
@@ -69,16 +65,25 @@ class SearchClient {
       skipCache = true;
     }
 
-    // returns null on timeout (after 5 seconds)
-    Future<http.Response?> 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(),
+          perRequestTimeout: 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<String, dynamic>,
+          json.decode(response.body!) as Map<String, dynamic>,
         );
       }
       // Search request before the service initialization completed.

pkg/_pub_shared/lib/utils/http.dart

@@ -47,23 +47,42 @@ Future<K> httpGetWithRetry<K>(
   Uri uri, {
   required FutureOr<K> Function(http.Response response) responseFn,
   int maxAttempts = 3,
+
+  /// The HTTP client to use.
+  ///
+  /// 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<String, String>? headers,
+
+  /// Per-request time amount that will be applied on the overall HTTP request.
+  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(
     () 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 (perRequestTimeout != null) {
+          f = f.timeout(perRequestTimeout);
+        }
+        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 +96,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';
+}