Skip to content

Commit b0afc91

Browse files
committed
store: Ignore boring NetworkExceptions
Usually when a client goes back from sleep or lose network connection, there will be a bunch of errors when polling event queue. This known error will always be a `NetworkException`, so we make a separate case for it. Previously, we may report these errors, and it ended up being spammy. This filters out the more interesting one to report instead. Signed-off-by: Zixuan James Li <[email protected]>
1 parent ac2a6ad commit b0afc91

File tree

2 files changed

+43
-3
lines changed

2 files changed

+43
-3
lines changed

lib/model/store.dart

+22-1
Original file line numberDiff line numberDiff line change
@@ -817,7 +817,7 @@ class UpdateMachine {
817817
debugLog('… Event queue replaced.');
818818
return;
819819

820-
case Server5xxException() || NetworkException():
820+
case Server5xxException():
821821
assert(debugLog('Transient error polling event queue for $store: $e\n'
822822
'Backing off, then will retry…'));
823823
maybeReportTransientError(
@@ -828,6 +828,27 @@ class UpdateMachine {
828828
assert(debugLog('… Backoff wait complete, retrying poll.'));
829829
continue;
830830

831+
case NetworkException():
832+
assert(debugLog('Transient error polling event queue for $store: $e\n'
833+
'Backing off, then will retry…'));
834+
if (e.cause is! SocketException) {
835+
// Heuristic check to only report interesting errors to the user.
836+
// This currently ignores [SocketException], which typically
837+
// occurs when the client goes back from sleep.
838+
// TODO: Investigate if there are other cases of [SocketException]
839+
// that might turn out to be interesting.
840+
//
841+
// See also:
842+
// * [NetworkException.cause], which is the underlying exception.
843+
maybeReportTransientError(
844+
localizations.errorConnectingToServerShort,
845+
details: localizations.errorConnectingToServerDetails(
846+
serverUrl, e.toString()));
847+
}
848+
await (backoffMachine ??= BackoffMachine()).wait();
849+
assert(debugLog('… Backoff wait complete, retrying poll.'));
850+
continue;
851+
831852
default:
832853
assert(debugLog('Error polling event queue for $store: $e\n'
833854
'Backing off and retrying even though may be hopeless…'));

test/model/store_test.dart

+21-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import 'dart:async';
2+
import 'dart:io';
23

34
import 'package:checks/checks.dart';
45
import 'package:flutter/foundation.dart';
@@ -476,10 +477,14 @@ void main() {
476477
/// Check if [UpdateMachine.poll] retries as expected when there are
477478
/// errors.
478479
///
479-
/// This also verifies that the first user-facing error message appears
480-
/// after the `numFailedRequests`'th failed request.
480+
/// By default, also verify that the first user-facing error message
481+
/// appears after the `numFailedRequests`'th failed request.
482+
///
483+
/// If `expectNotifyError` is false, instead verify that there is no
484+
/// user-facing error message regardless of the retries.
481485
void checkRetry(void Function() prepareError, {
482486
required int numFailedRequests,
487+
bool expectNotifyError = true,
483488
}) {
484489
assert(numFailedRequests > 0);
485490
reportErrorToUserBriefly = logAndReportErrorToUserBriefly;
@@ -510,12 +515,19 @@ void main() {
510515
async.flushTimers();
511516
}
512517

518+
if (!expectNotifyError) {
519+
// No matter how many retries there have been, no request should
520+
// result in an error message.
521+
check(takeLastReportedError()).isNull();
522+
continue;
523+
}
513524
if (i < numFailedRequests - 1) {
514525
// The error message should not appear until the `updateMachine`
515526
// has retried the given number of times.
516527
check(takeLastReportedError()).isNull();
517528
continue;
518529
}
530+
assert(expectNotifyError);
519531
assert(i == numFailedRequests - 1);
520532
check(takeLastReportedError()).isNotNull().contains(expectedErrorMessage);
521533
}
@@ -548,6 +560,13 @@ void main() {
548560
numFailedRequests: UpdateMachine.transientFailureCountNotifyThreshold + 1);
549561
});
550562

563+
test('NetworkException to be ignored', () {
564+
checkRetry(() => connection.prepare(
565+
exception: const SocketException("failed")),
566+
numFailedRequests: UpdateMachine.transientFailureCountNotifyThreshold + 1,
567+
expectNotifyError: false);
568+
});
569+
551570
test('ZulipApiException', () {
552571
checkRetry(() => connection.prepare(httpStatus: 400, json: {
553572
'result': 'error', 'code': 'BAD_REQUEST', 'msg': 'Bad request'}),

0 commit comments

Comments
 (0)