eventActions.js: Clarify loop-break logic and fix race condition in startEventPolling.

This follows a discussion at and around
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Event.20polling.20optimization.3F/near/810784.

The `queueId !== getState().session.eventQueueId` is not the most
direct way to check that the same user has remained logged in. So,
replace that check and the `auth` check above it with a call to the
new helper function authEquivalent, to check the maybe-stale auth
against the current auth.

Also, the `queueId !== ...` check allows a race condition! We change
`session.eventQueueId` at `REALM_INIT`, which only happens after a
`/register` request *completes*. So a user could switch accounts,
triggering a new `/register` request, but the old account's event
poll could return before that new `/register` completes, and the
`queueId !== ...` will pass, triggering a state update with the old
account's events instead of the new account's. Checking auth
information is what we want here.

It's true that this auth check will not catch the case that a queue
was killed following a period of inactivity (i.e., the
BAD_EVENT_QUEUE_ID). However, this is not a flaw, because the loop
already handles that case correctly without the `queueId !== ...`
check. In particular, if a queue is killed, api.pollForEvents will
reject, and that rejection is handled in the catch block.

When modifying startEventPolling to accept an auth parameter, I
noticed (and added a comment to explain) that some actions being
dispatched in doInitialFetch will throw unhandled errors if a user
logs out while the /register request is in progress. This will be of
concern when we work on #3706.

Author: Chris Bobbe

src/events/eventActions.js

@@ -1,7 +1,7 @@
 /* @flow strict-local */
 import { batchActions } from 'redux-batched-actions';
 
-import type { Action, Dispatch, GeneralEvent, GetState, GlobalState } from '../types';
+import type { Auth, Action, Dispatch, GeneralEvent, GetState, GlobalState } from '../types';
 import * as api from '../api';
 import { logout } from '../account/accountActions';
 import { deadQueue } from '../session/sessionActions';
@@ -11,6 +11,7 @@ import { tryGetAuth } from '../selectors';
 import actionCreator from '../actionCreator';
 import progressiveTimeout from '../utils/progressiveTimeout';
 import { ApiError } from '../api/apiErrors';
+import { authEquivalent } from '../account/accountMisc';
 
 /** Convert an `/events` response into a sequence of our Redux actions. */
 export const responseToActions = (
@@ -44,12 +45,13 @@ export const dispatchOrBatch = (dispatch: Dispatch, actions: $ReadOnlyArray<Acti
 };
 
 /**
- * Poll an event queue on the Zulip server for updates, in a loop.
+ * Poll an event queue on the Zulip server for updates, in a loop, as long as
+ * the user remains logged into the same account.
  *
  * This is part of our use of the Zulip events system; see `doInitialFetch`
  * for discussion.
  */
-export const startEventPolling = (queueId: number, eventId: number) => async (
+export const startEventPolling = (auth: Auth, queueId: number, eventId: number) => async (
   dispatch: Dispatch,
   getState: GetState,
 ) => {
@@ -58,17 +60,18 @@ export const startEventPolling = (queueId: number, eventId: number) => async (
   /* eslint-disable no-await-in-loop */
   // eslint-disable-next-line no-constant-condition
   while (true) {
-    const auth = tryGetAuth(getState());
-    if (!auth) {
-      // User switched accounts or logged out
+    if (!authEquivalent(auth, tryGetAuth(getState()))) {
+      // The user logged out or switched accounts during progressiveTimeout (see catch block, below).
+      // (If tryGetAuth returns undefined, the user is not logged in.)
       break;
     }
 
     try {
       const { events } = await api.pollForEvents(auth, queueId, lastEventId);
 
-      // User switched accounts or logged out
-      if (queueId !== getState().session.eventQueueId) {
+      if (!authEquivalent(auth, tryGetAuth(getState()))) {
+        // The user logged out or switched accounts while api.pollForEvents was in progress.
+        // (If tryGetAuth returns undefined, the user is not logged in.)
         break;
       }
 

src/message/fetchActions.js

@@ -3,6 +3,7 @@ import type { Narrow, Dispatch, GetState, GlobalState, Message, Action } from '.
 import * as api from '../api';
 import {
   getAuth,
+  tryGetAuth,
   getSession,
   getFirstMessageId,
   getLastMessageId,
@@ -242,7 +243,19 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat
   dispatch(realmInit(initData));
   dispatch(fetchTopMostNarrow());
   dispatch(initialFetchComplete());
-  dispatch(startEventPolling(initData.queue_id, initData.last_event_id));
+
+  const authNow = tryGetAuth(getState());
+  if (authNow !== undefined) {
+    // The user may have logged out while api.registerForEvents was in progress.
+    // If so, don't start polling events.
+
+    // TODO: If the user did log out during that time, other actions that are dispatched
+    // here (in doInitialFetch, after api.registerForEvents) will throw an error,
+    // which may be a cause of #3706! These include the calls to fetchTopMostNarrow,
+    // fetchPrivateMessages, and fetchMessagesInNarrow, because they all dispatch
+    // fetchMessages, which calls getAuth, which throws with 'Active account not logged in'.
+    dispatch(startEventPolling(authNow, initData.queue_id, initData.last_event_id));
+  }
 
   dispatch(fetchPrivateMessages());