Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions app/lib/search/search_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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, {
Expand All @@ -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(),
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);
}
}

Expand All @@ -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.
Expand Down
45 changes: 39 additions & 6 deletions pkg/_pub_shared/lib/utils/http.dart
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,41 @@ 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 specified, the inner loop will not create a new client object on retries.
http.Client? client,
Map<String, String>? 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps document that the retry function can check for this.

} finally {
client.close();
if (closeClient) {
effectiveClient.close();
}
}
},
maxAttempts: maxAttempts,
retryIf: _retryIf,
retryIf: (e) => _retryIf(e) || (retryIf != null && retryIf(e)),
);
}

Expand All @@ -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';
}