store: Report polling errors with details

We extend the checkRetry helper to support simulating multiple
consecutive errors.

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]>

Author: PIG208

assets/l10n/app_en.arb

@@ -160,6 +160,18 @@
       "message": {"type": "String", "example": "Invalid format"}
     }
   },
+  "errorConnectingToServerShort": "Error connecting to Zulip. Retrying…",
+  "@errorConnectingToServerShort": {
+    "description": "Short error message for a generic unknown error connecting to the server."
+  },
+  "errorConnectingToServerDetails": "Error connecting to Zulip at {serverUrl}. Will retry:\n\n{error}",
+  "@errorConnectingToServerDetails": {
+    "description": "Dialog error message for a generic unknown error connecting to the server with details.",
+    "placeholders": {
+      "serverUrl": {"type": "String", "example": "http://example.com/"},
+      "error": {"type": "String", "example": "Invalid format"}
+    }
+  },
   "errorSharingFailed": "Sharing failed",
   "@errorSharingFailed": {
     "description": "Error message when sharing a message failed."

lib/model/store.dart

@@ -20,6 +20,7 @@ import '../log.dart';
 import '../notifications/receive.dart';
 import 'autocomplete.dart';
 import 'database.dart';
+import 'localizations.dart';
 import 'message.dart';
 import 'message_list.dart';
 import 'recent_dm_conversations.dart';
@@ -771,8 +772,25 @@ class UpdateMachine {
     }());
   }
 
+  /// This controls when we start to report transient errors to the user when
+  /// polling.
+  ///
+  /// At the 6th failure, the expected time elapsed since the first failure
+  /// will be 1.55 seocnds.
+  static const transientFailureCountNotifyThreshold = 5;
+
   void poll() async {
     BackoffMachine? backoffMachine;
+    int accumulatedTransientFailureCount = 0;
+
+    /// This only reports transient errors after reaching
+    /// a pre-defined threshold of retries.
+    void maybeReportTransientError(String? message, {String? details}) {
+      accumulatedTransientFailureCount++;
+      if (accumulatedTransientFailureCount > transientFailureCountNotifyThreshold) {
+        reportErrorToUserBriefly(message, details: details);
+      }
+    }
 
     while (true) {
       if (_debugLoopSignal != null) {
@@ -789,6 +807,8 @@ class UpdateMachine {
           queueId: queueId, lastEventId: lastEventId);
       } catch (e) {
         store.isLoading = true;
+        final localizations = GlobalLocalizations.zulipLocalizations;
+        final serverUrl = store.connection.realmUrl.origin;
         switch (e) {
           case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'):
             assert(debugLog('Lost event queue for $store.  Replacing…'));
@@ -800,15 +820,22 @@ class UpdateMachine {
           case Server5xxException() || NetworkException():
             assert(debugLog('Transient error polling event queue for $store: $e\n'
                 'Backing off, then will retry…'));
-            // TODO tell user if transient polling errors persist
+            maybeReportTransientError(
+              localizations.errorConnectingToServerShort,
+              details: localizations.errorConnectingToServerDetails(
+                serverUrl, e.toString()));
             await (backoffMachine ??= BackoffMachine()).wait();
             assert(debugLog('… Backoff wait complete, retrying poll.'));
             continue;
 
           default:
             assert(debugLog('Error polling event queue for $store: $e\n'
                 'Backing off and retrying even though may be hopeless…'));
-            // TODO tell user on non-transient error in polling
+            // TODO(#186): Handle unrecoverable failures
+            reportErrorToUserBriefly(
+              localizations.errorConnectingToServerShort,
+              details: localizations.errorConnectingToServerDetails(
+                serverUrl, e.toString()));
             await (backoffMachine ??= BackoffMachine()).wait();
             assert(debugLog('… Backoff wait complete, retrying poll.'));
             continue;
@@ -832,6 +859,9 @@ class UpdateMachine {
       // and failures, the successes themselves should space out the requests.
       backoffMachine = null;
       store.isLoading = false;
+      // Dismiss existing errors, if any.
+      reportErrorToUserBriefly(null);
+      accumulatedTransientFailureCount = 0;
 
       final events = result.events;
       for (final event in events) {

test/model/store_test.dart

@@ -10,6 +10,7 @@ import 'package:zulip/api/route/events.dart';
 import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/model/message_list.dart';
 import 'package:zulip/model/narrow.dart';
+import 'package:zulip/log.dart';
 import 'package:zulip/model/store.dart';
 import 'package:zulip/notifications/receive.dart';
 
@@ -393,6 +394,22 @@ void main() {
       check(store.userSettings!.twentyFourHourTime).isTrue();
     }));
 
+    String? lastReportedError;
+    String? takeLastReportedError() {
+      final result = lastReportedError;
+      lastReportedError = null;
+      return result;
+    }
+
+    /// This is an alternative to [ZulipApp]'s implementation of
+    /// [reportErrorToUserBriefly] for testing.
+    Future<void> logAndReportErrorToUserBriefly(String? message, {
+      String? details,
+    }) async {
+      if (message == null) return;
+      lastReportedError = '$message\n$details';
+    }
+
     test('handles expired queue', () => awaitFakeAsync((async) async {
       await prepareStore();
       updateMachine.debugPauseLoop();
@@ -456,19 +473,52 @@ void main() {
     }));
 
     group('retries on errors', () {
-      void checkRetry(void Function() prepareError) {
+      /// Check if [UpdateMachine.poll] retries as expected when there are
+      /// errors.
+      ///
+      /// This also verifies that the first user-facing error message appears
+      /// after the `numFailedRequests`'th failed request.
+      void checkRetry(void Function() prepareError, {
+        required int numFailedRequests,
+      }) {
+        assert(numFailedRequests > 0);
+        reportErrorToUserBriefly = logAndReportErrorToUserBriefly;
+        addTearDown(() => reportErrorToUserBriefly = defaultReportErrorToUserBriefly);
+
+        final expectedErrorMessage =
+          'Error connecting to Zulip. Retrying…\n'
+          'Error connecting to Zulip at ${eg.realmUrl.origin}. Will retry';
+
         awaitFakeAsync((async) async {
           await prepareStore(lastEventId: 1);
           updateMachine.debugPauseLoop();
           updateMachine.poll();
           check(async.pendingTimers).length.equals(0);
 
-          // Make the request, inducing an error in it.
-          prepareError();
-          updateMachine.debugAdvanceLoop();
-          async.elapse(Duration.zero);
-          checkLastRequest(lastEventId: 1);
-          check(store).isLoading.isTrue();
+          for (int i = 0; i < numFailedRequests; i++) {
+            // Make the request, inducing an error in it.
+            prepareError();
+            updateMachine.debugAdvanceLoop();
+            async.elapse(Duration.zero);
+            checkLastRequest(lastEventId: 1);
+            check(store).isLoading.isTrue();
+
+            if (i != numFailedRequests - 1) {
+              // Skip polling backoff unless this is the final iteration.
+              // This allows the next `updateMachine.debugAdvanceLoop` call to
+              // trigger the next request without wait.
+              async.flushTimers();
+            }
+
+            if (i < numFailedRequests - 1) {
+              // The error message should not appear until the `updateMachine`
+              // has retried the given number of times.
+              check(takeLastReportedError()).isNull();
+              continue;
+            }
+            assert(i == numFailedRequests - 1);
+            check(takeLastReportedError()).isNotNull().contains(expectedErrorMessage);
+          }
 
           // Polling doesn't resume immediately; there's a timer.
           check(async.pendingTimers).length.equals(1);
@@ -489,20 +539,24 @@ void main() {
       }
 
       test('Server5xxException', () {
-        checkRetry(() => connection.prepare(httpStatus: 500, body: 'splat'));
+        checkRetry(() => connection.prepare(httpStatus: 500, body: 'splat'),
+          numFailedRequests: UpdateMachine.transientFailureCountNotifyThreshold + 1);
       });
 
       test('NetworkException', () {
-        checkRetry(() => connection.prepare(exception: Exception("failed")));
+        checkRetry(() => connection.prepare(exception: Exception("failed")),
+          numFailedRequests: UpdateMachine.transientFailureCountNotifyThreshold + 1);
       });
 
       test('ZulipApiException', () {
         checkRetry(() => connection.prepare(httpStatus: 400, json: {
-          'result': 'error', 'code': 'BAD_REQUEST', 'msg': 'Bad request'}));
+          'result': 'error', 'code': 'BAD_REQUEST', 'msg': 'Bad request'}),
+          numFailedRequests: 1);
       });
 
       test('MalformedServerResponseException', () {
-        checkRetry(() => connection.prepare(httpStatus: 200, body: 'nonsense'));
+        checkRetry(() => connection.prepare(httpStatus: 200, body: 'nonsense'),
+          numFailedRequests: 1);
       });
     });
   });