Skip to content

fix: retry HTTP requests on transient network errors and 5xx #2779

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
lacostej wants to merge 2 commits into from
Closed

fix: retry HTTP requests on transient network errors and 5xx #2779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7cda7bd
fix: add 15-second timeout to all HTTP requests
lacostej Mar 16, 2026
1bdbd85
fix: retry HTTP requests on transient network errors and 5xx
lacostej Mar 16, 2026
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
10 changes: 7 additions & 3 deletions lib/src/network/http.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,14 @@ final defaultClientProvider = Provider<DefaultClient>((Ref ref) {
/// Only one instance of this client is created and kept alive for the whole app.
final lichessClientProvider = Provider<LichessClient>((Ref ref) {
final client = LichessClient(
// Retry just once, after 500ms, on 429 Too Many Requests.
// Retry once after 500ms on rate-limiting, server errors, or transient
// network failures (socket errors, timeouts).
RetryClient(
ref.read(httpClientFactoryProvider)(),
retries: 1,
delay: _defaultDelay,
when: (response) => response.statusCode == 429,
when: (response) => response.statusCode == 429 || response.statusCode >= 500,
Copy link
Copy Markdown
Contributor

@veloce veloce Mar 17, 2026

Choose a reason for hiding this comment

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

Not so sure about the utility of retrying on all 5xx server errors, even though it should not cause any harm. Also not sure about retrying on SocketException, as most of the time it means the network is off.

Also FYI there is a global retry mechanism in the riverpod layer:

mobile/lib/main.dart

Line 45 in e65f046

if (error is ServerException && error.statusCode != 503) return null;

Of course the client can be used outside a provider, but the most common case is inside a provider. Ideally we should try to avoid both retry mechanisms to conflict with each other and unnecessary requests.

Not sure what is the best approach here. We could handle the network retries in the client only which means we need to improve the logic to filter out network errors in the riverpod retry callback. It could be useful to throw a custom HttpTimeoutException in that case.

whenError: (error, _) => error is SocketException || error is TimeoutException,
),
ref,
);
Expand Down Expand Up @@ -332,6 +334,8 @@ class _RegisterCallbackClient extends BaseClient {
class LichessClient implements Client {
LichessClient(this._inner, this._ref);

static const defaultRequestTimeout = Duration(seconds: 15);

final Ref _ref;
final Client _inner;

Expand All @@ -353,7 +357,7 @@ class LichessClient implements Client {
_logger.info('${request.method} ${request.url} ${request.headers['User-Agent']}');

try {
final response = await _inner.send(request);
final response = await _inner.send(request).timeout(defaultRequestTimeout);

_logIfError(response);

Expand Down
100 changes: 100 additions & 0 deletions test/network/http_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,106 @@ void main() {
);
});

test('retries once on SocketException then succeeds', () async {
int callCount = 0;
final retryClient = MockClient((request) async {
callCount++;
if (callCount == 1) {
throw const SocketException('no internet');
}
return http.Response('{"ok": true}', 200);
});
final container = await makeContainer(
overrides: {
httpClientFactoryProvider: httpClientFactoryProvider.overrideWith((ref) {
return FakeHttpClientFactory(() => retryClient);
}),
},
);
final client = container.read(lichessClientProvider);
fakeAsync((async) {
late http.Response response;
client.get(Uri(path: '/test')).then((r) => response = r);
async.flushTimers();
expect(response.statusCode, 200);
expect(callCount, 2);
});
});

test('retries once on 500 then succeeds', () async {
int callCount = 0;
final retryClient = MockClient((request) async {
callCount++;
if (callCount == 1) {
return http.Response('error', 500);
}
return http.Response('{"ok": true}', 200);
});
final container = await makeContainer(
overrides: {
httpClientFactoryProvider: httpClientFactoryProvider.overrideWith((ref) {
return FakeHttpClientFactory(() => retryClient);
}),
},
);
final client = container.read(lichessClientProvider);
fakeAsync((async) {
late http.Response response;
client.get(Uri(path: '/test')).then((r) => response = r);
async.flushTimers();
expect(response.statusCode, 200);
expect(callCount, 2);
});
});

test('does not retry on 4xx errors', () async {
int callCount = 0;
final retryClient = MockClient((request) async {
callCount++;
return http.Response('not found', 404);
});
final container = await makeContainer(
overrides: {
httpClientFactoryProvider: httpClientFactoryProvider.overrideWith((ref) {
return FakeHttpClientFactory(() => retryClient);
}),
},
);
final client = container.read(lichessClientProvider);
fakeAsync((async) {
late http.Response response;
client.get(Uri(path: '/test')).then((r) => response = r);
async.flushTimers();
expect(response.statusCode, 404);
expect(callCount, 1);
});
});

test('propagates SocketException after retry exhausted', () async {
int callCount = 0;
final retryClient = MockClient((request) {
callCount++;
throw const SocketException('no internet');
});
final container = await makeContainer(
overrides: {
httpClientFactoryProvider: httpClientFactoryProvider.overrideWith((ref) {
return FakeHttpClientFactory(() => retryClient);
}),
},
);
final client = container.read(lichessClientProvider);
fakeAsync((async) {
Object? caughtError;
client.get(Uri(path: '/test')).then((_) {}).catchError((Object e) {
caughtError = e;
});
async.flushTimers();
expect(caughtError, isA<SocketException>());
expect(callCount, 2);
});
});

test('failed JSON parsing will throw ClientException', () async {
final container = await makeContainer(
overrides: {
Expand Down
8 changes: 8 additions & 0 deletions test/test_provider_scope.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,14 @@ Future<Widget> makeOfflineTestProviderScope(
httpClientFactoryProvider: httpClientFactoryProvider.overrideWith((ref) {
return FakeHttpClientFactory(() => offlineClient);
}),
// Override lichessClientProvider to bypass RetryClient, which would
// otherwise retry on SocketException and create pending timers in
// FakeAsync-based tests.
lichessClientProvider: lichessClientProvider.overrideWith((ref) {
final client = LichessClient(offlineClient, ref);
ref.onDispose(() => client.close());
return client;
}),
...?overrides,
},
authUser: authUser,
Expand Down