Skip to content

Commit 33feddc

Browse files
committed
store: Report polling errors with details
We delay reporting transient errors until we have retried a certain number of times, because they might recover on their own. Signed-off-by: Zixuan James Li <[email protected]>
1 parent 03dfdf1 commit 33feddc

File tree

3 files changed

+113
-2
lines changed

3 files changed

+113
-2
lines changed

assets/l10n/app_en.arb

+12
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,18 @@
160160
"message": {"type": "String", "example": "Invalid format"}
161161
}
162162
},
163+
"errorConnectingToServerShort": "Error connecting to Zulip. Retrying…",
164+
"@errorConnectingToServerShort": {
165+
"description": "Short error message for a generic unknown error connecting to the server."
166+
},
167+
"errorConnectingToServerDetails": "Error connecting to Zulip at {serverUrl}. Will retry:\n\n{error}",
168+
"@errorConnectingToServerDetails": {
169+
"description": "Dialog error message for a generic unknown error connecting to the server with details.",
170+
"placeholders": {
171+
"serverUrl": {"type": "String", "example": "http://example.com/"},
172+
"error": {"type": "String", "example": "Invalid format"}
173+
}
174+
},
163175
"errorSharingFailed": "Sharing failed",
164176
"@errorSharingFailed": {
165177
"description": "Error message when sharing a message failed."

lib/model/store.dart

+33-2
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import '../notifications/receive.dart';
2222
import 'autocomplete.dart';
2323
import 'database.dart';
2424
import 'emoji.dart';
25+
import 'localizations.dart';
2526
import 'message.dart';
2627
import 'message_list.dart';
2728
import 'recent_dm_conversations.dart';
@@ -912,10 +913,28 @@ class UpdateMachine {
912913
}());
913914
}
914915

916+
/// This controls when we start to report transient errors to the user when
917+
/// polling.
918+
///
919+
/// At the 6th failure, the expected time elapsed since the first failure
920+
/// will be 1.55 seocnds.
921+
static const transientFailureCountNotifyThreshold = 5;
922+
915923
void poll() async {
916924
assert(!_disposed);
917925

918926
BackoffMachine? backoffMachine;
927+
int accumulatedTransientFailureCount = 0;
928+
929+
/// This only reports transient errors after reaching
930+
/// a pre-defined threshold of retries.
931+
void maybeReportTransientError(String? message, {String? details}) {
932+
accumulatedTransientFailureCount++;
933+
if (accumulatedTransientFailureCount > transientFailureCountNotifyThreshold) {
934+
reportErrorToUserBriefly(message, details: details);
935+
}
936+
}
937+
919938
while (true) {
920939
if (_debugLoopSignal != null) {
921940
await _debugLoopSignal!.future;
@@ -935,6 +954,8 @@ class UpdateMachine {
935954
if (_disposed) return;
936955

937956
store.isLoading = true;
957+
final localizations = GlobalLocalizations.zulipLocalizations;
958+
final serverUrl = store.connection.realmUrl.origin;
938959
switch (e) {
939960
case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'):
940961
assert(debugLog('Lost event queue for $store. Replacing…'));
@@ -946,15 +967,22 @@ class UpdateMachine {
946967
case Server5xxException() || NetworkException():
947968
assert(debugLog('Transient error polling event queue for $store: $e\n'
948969
'Backing off, then will retry…'));
949-
// TODO tell user if transient polling errors persist
970+
maybeReportTransientError(
971+
localizations.errorConnectingToServerShort,
972+
details: localizations.errorConnectingToServerDetails(
973+
serverUrl, e.toString()));
950974
await (backoffMachine ??= BackoffMachine()).wait();
951975
assert(debugLog('… Backoff wait complete, retrying poll.'));
952976
continue;
953977

954978
default:
955979
assert(debugLog('Error polling event queue for $store: $e\n'
956980
'Backing off and retrying even though may be hopeless…'));
957-
// TODO tell user on non-transient error in polling
981+
// TODO(#186): Handle unrecoverable failures
982+
reportErrorToUserBriefly(
983+
localizations.errorConnectingToServerShort,
984+
details: localizations.errorConnectingToServerDetails(
985+
serverUrl, e.toString()));
958986
await (backoffMachine ??= BackoffMachine()).wait();
959987
assert(debugLog('… Backoff wait complete, retrying poll.'));
960988
continue;
@@ -978,6 +1006,9 @@ class UpdateMachine {
9781006
// and failures, the successes themselves should space out the requests.
9791007
backoffMachine = null;
9801008
store.isLoading = false;
1009+
// Dismiss existing errors, if any.
1010+
reportErrorToUserBriefly(null);
1011+
accumulatedTransientFailureCount = 0;
9811012

9821013
final events = result.events;
9831014
for (final event in events) {

test/model/store_test.dart

+68
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import 'dart:async';
22

33
import 'package:checks/checks.dart';
4+
import 'package:fake_async/fake_async.dart';
45
import 'package:flutter/foundation.dart';
56
import 'package:http/http.dart' as http;
67
import 'package:test/scaffolding.dart';
@@ -12,6 +13,7 @@ import 'package:zulip/api/route/messages.dart';
1213
import 'package:zulip/api/route/realm.dart';
1314
import 'package:zulip/model/message_list.dart';
1415
import 'package:zulip/model/narrow.dart';
16+
import 'package:zulip/log.dart';
1517
import 'package:zulip/model/store.dart';
1618
import 'package:zulip/notifications/receive.dart';
1719

@@ -652,6 +654,72 @@ void main() {
652654
test('retries on MalformedServerResponseException', () {
653655
checkRetry(() => connection.prepare(httpStatus: 200, body: 'nonsense'));
654656
});
657+
658+
group('reportErrorToUserBriefly', () {
659+
String? lastReportedError;
660+
String? takeLastReportedError() {
661+
final result = lastReportedError;
662+
lastReportedError = null;
663+
return result;
664+
}
665+
666+
/// This is an alternative to [ZulipApp]'s implementation of
667+
/// [reportErrorToUserBriefly] for testing.
668+
Future<void> logAndReportErrorToUserBriefly(String? message, {
669+
String? details,
670+
}) async {
671+
if (message == null) return;
672+
lastReportedError = '$message\n$details';
673+
}
674+
675+
Future<void> prepare() async {
676+
reportErrorToUserBriefly = logAndReportErrorToUserBriefly;
677+
addTearDown(() => reportErrorToUserBriefly = defaultReportErrorToUserBriefly);
678+
679+
await prepareStore(lastEventId: 1);
680+
updateMachine.debugPauseLoop();
681+
updateMachine.poll();
682+
}
683+
684+
late void Function() prepareError;
685+
686+
void advanceLoopAndFail(FakeAsync async) {
687+
prepareError();
688+
updateMachine.debugAdvanceLoop();
689+
async.elapse(Duration.zero);
690+
checkLastRequest(lastEventId: 1);
691+
check(store).isLoading.isTrue();
692+
}
693+
694+
test('report non-transient errors', () => awaitFakeAsync((async) async {
695+
prepareError = () => connection.prepare(httpStatus: 400, json: {
696+
'result': 'error', 'code': 'BAD_REQUEST', 'msg': 'Bad request'});
697+
await prepare();
698+
699+
advanceLoopAndFail(async);
700+
check(takeLastReportedError()).isNotNull().startsWith(
701+
"Error connecting to Zulip. Retrying…\n"
702+
"Error connecting to Zulip at");
703+
}));
704+
705+
test('report transient errors', () => awaitFakeAsync((async) async {
706+
prepareError = () => connection.prepare(httpStatus: 500, body: 'splat');
707+
await prepare();
708+
709+
// There should be no user visible error messages during these retries.
710+
for (int i = 0; i < UpdateMachine.transientFailureCountNotifyThreshold; i++) {
711+
advanceLoopAndFail(async);
712+
check(takeLastReportedError()).isNull();
713+
// This would skip the pending polling backoff.
714+
async.flushTimers();
715+
}
716+
717+
advanceLoopAndFail(async);
718+
check(takeLastReportedError()).isNotNull().startsWith(
719+
"Error connecting to Zulip. Retrying…\n"
720+
"Error connecting to Zulip at");
721+
}));
722+
});
655723
});
656724

657725
group('UpdateMachine.registerNotificationToken', () {

0 commit comments

Comments
 (0)