wip dedupe reload on poll error; and on non-transient request errors, reload rather than retry

Hmm or maybe for those non-transient request errors we should stick
with retrying?  Maybe up to a bounded number of times?

Those are by definition a bug either in the server or in our
formulating the request: it's either a 4xx we didn't expect,
or a malformed response (so in either case, a mismatch of our
expectations to the server's behavior, and therefore a bug in
one or the other), or something other than ApiRequestException
entirely (and so a bug in the API request code).

It's therefore hard to predict what's most likely to work.

Maybe the most likely cause is that the server is having operational
trouble that's not showing up as 5xx responses.

... Hmm, do we handle rate-limit errors?  No, we don't.
So *that's* one possible cause, anyway.  Certainly we should
back off if we get one of those!  ... Though I'm not sure
that getEvents is actually currently subject to rate-limiting,
being in Tornado rather than Django.

Also, when we abandon retrying getEvents and switch to re-register,
we do end up backing off if that fails.  It's just that that's a
heavier request in the first place.

Author: gnprice

lib/model/store.dart

@@ -968,13 +968,6 @@ class UpdateMachine {
 
           store.isLoading = true;
           switch (e) {
-            case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'):
-              assert(debugLog('Lost event queue for $store.  Replacing…'));
-              // This disposes the store, which disposes this update machine.
-              await store._globalStore._reloadPerAccount(store.accountId);
-              debugLog('… Event queue replaced.');
-              return;
-
             case Server5xxException() || NetworkException():
               assert(debugLog('Transient error polling event queue for $store: $e\n'
                   'Backing off, then will retry…'));
@@ -990,14 +983,7 @@ class UpdateMachine {
               continue;
 
             default:
-              assert(debugLog('Error polling event queue for $store: $e\n'
-                  'Backing off and retrying even though may be hopeless…'));
-              // TODO(#186): Handle unrecoverable failures
-              _reportToUserErrorConnectingToServer(e);
-              await (backoffMachine ??= BackoffMachine()).wait();
-              if (_disposed) return;
-              assert(debugLog('… Backoff wait complete, retrying poll.'));
-              continue;
+              rethrow;
           }
         }
 
@@ -1027,27 +1013,52 @@ class UpdateMachine {
           try {
             await store.handleEvent(event);
             if (_disposed) return;
-          } catch (e) {
-            assert(debugLog('BUG: Error handling an event: $e\n' // TODO(log)
-              '  event: $event\n'
-              'Replacing event queue…'));
-            // TODO maybe tell user (in beta) that event handling threw error
-            // TODO dedupe this with the other _reloadPerAccount sites?
-            // This disposes the store, which disposes this update machine.
-            await store._globalStore._reloadPerAccount(store.accountId);
-            debugLog('… Event queue replaced.');
-            return;
+          } catch (e, stackTrace) {
+            Error.throwWithStackTrace(
+              _EventHandlingException(cause: e, event: event),
+              stackTrace);
           }
         }
         if (events.isNotEmpty) {
           lastEventId = events.last.id;
         }
       }
     } catch (e) {
-      assert(debugLog('BUG: Unexpected error in event polling: $e\n' // TODO(log)
-        'Replacing event queue…'));
-      // TODO maybe tell user (in beta) that event poll loop threw error
-      // TODO dedupe this with the other _reloadPerAccount sites above?
+      // An error occurred, other than the transient request errors we retry on.
+      // This means either a lost/expired event queue on the server (which is
+      // normal after the app is offline for a period like 10 minutes),
+      // or an unexpected exception representing a bug in our code or the server.
+      // Either way, the show must go on.  So reload server data from scratch.
+
+      // First, log what happened.
+      switch (e) {
+        case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'):
+          assert(debugLog('Lost event queue for $store.  Replacing…'));
+          // The old event queue is gone, so we need a new one.  This is normal.
+
+        case _EventHandlingException(:final cause, :final event):
+          assert(debugLog('BUG: Error handling an event: $cause\n' // TODO(log)
+            '  event: $event\n'
+            'Replacing event queue…'));
+          // TODO maybe tell user (in beta) that event handling threw error
+          // We can't just continue with the next event, because our state
+          // may be garbled due to failing to apply this one (and worse,
+          // any invariants that were left in a broken state from where
+          // the exception was thrown).  So reload from scratch.
+          // Hopefully (probably?) the bug only affects our implementation of
+          // the *change* in state represented by the event, and when we get the
+          // new state in a fresh InitialSnapshot we'll handle that just fine.
+
+        default:
+          assert(debugLog('BUG: Unexpected error in event polling: $e\n' // TODO(log)
+            'Replacing event queue…'));
+          _reportToUserErrorConnectingToServer(e);
+          // Similar story to the _EventHandlingException case;
+          // separate only so that one can print more context.
+          // The bug here could be in the server if it's an ApiRequestException,
+          // but our remedy is the same either way.
+      }
+
       // This disposes the store, which disposes this update machine.
       await store._globalStore._reloadPerAccount(store.accountId);
       debugLog('… Event queue replaced.');
@@ -1143,3 +1154,10 @@ class UpdateMachine {
   @override
   String toString() => '${objectRuntimeType(this, 'UpdateMachine')}#${shortHash(this)}';
 }
+
+class _EventHandlingException implements Exception {
+  final Object cause;
+  final Event event;
+
+  _EventHandlingException({required this.cause, required this.event});
+}