From 710c9f73961c0b49898125a87b100913afeaefdb Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 21 Jan 2020 17:25:32 -0800 Subject: [PATCH 1/3] eventActions: Don't wait before handling dead queue error. In startEventPolling, there's no use delaying the response to a loop-breaking BAD_EVENT_QUEUE_ID with backoffMachine.wait(). So, move that wait so it happens after DEAD_QUEUE is dispatched. --- src/events/eventActions.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/events/eventActions.js b/src/events/eventActions.js index 09b4f49c345..3990096d3bd 100644 --- a/src/events/eventActions.js +++ b/src/events/eventActions.js @@ -86,14 +86,14 @@ export const startEventPolling = (queueId: number, eventId: number) => async ( break; } - // protection from inadvertent DDOS - await backoffMachine.wait(); - if (e instanceof ApiError && e.code === 'BAD_EVENT_QUEUE_ID') { // The event queue is too old or has been garbage collected. dispatch(deadQueue()); break; } + + // protection from inadvertent DOS + await backoffMachine.wait(); } } }; From 82fc0abf7f9448c0679c1e41fef31bc0ca8b6641 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 21 Jan 2020 16:29:09 -0800 Subject: [PATCH 2/3] accountMisc: Add authEquivalent function. This is a central function to determine that two Auth objects refer to the same account, to be used in the next commit. --- src/account/accountMisc.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/account/accountMisc.js b/src/account/accountMisc.js index 5f2252906ff..a246033bd43 100644 --- a/src/account/accountMisc.js +++ b/src/account/accountMisc.js @@ -10,7 +10,20 @@ export const identityOfAccount: Account => Identity = identitySlice; /** A string corresponding uniquely to an identity, for use in `Map`s. */ export const keyOfIdentity = ({ realm, email }: Identity): string => `${realm}\0${email}`; +const keyOfAuth = (auth: Auth): string => keyOfIdentity(identityOfAuth(auth)); + export const authOfAccount = (account: Account): Auth => { const { realm, email, apiKey } = account; return { realm, email, apiKey }; }; + +/** + * Takes two Auth objects and confirms that they are equal, either by + * 1.) strict equality + * 2.) equality of their identity key (realm + email) and API key + */ +export const authEquivalent = (auth1: Auth | void, auth2: Auth | void): boolean => + auth1 === auth2 + || (auth1 !== undefined + && auth2 !== undefined + && (auth1.apiKey === auth2.apiKey && keyOfAuth(auth1) === keyOfAuth(auth2))); From 0abc2cb318fa648df14cedef0325a5df1ca800af Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 21 Jan 2020 17:05:56 -0800 Subject: [PATCH 3/3] eventActions: Clarify loop-break logic, fix race 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. --- src/events/eventActions.js | 20 ++++++++++++-------- src/message/fetchActions.js | 15 ++++++++++++++- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/events/eventActions.js b/src/events/eventActions.js index 3990096d3bd..a82200f61de 100644 --- a/src/events/eventActions.js +++ b/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 { BackoffMachine } from '../utils/async'; 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 async ( +export const startEventPolling = (auth: Auth, queueId: number, eventId: number) => async ( dispatch: Dispatch, getState: GetState, ) => { @@ -59,17 +61,19 @@ export const startEventPolling = (queueId: number, eventId: number) => async ( // 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 + // called from 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; } diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index d59c339a095..9f180b745d9 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -12,6 +12,7 @@ import type { InitialData } from '../api/initialDataTypes'; import * as api from '../api'; import { getAuth, + tryGetAuth, getSession, getFirstMessageId, getLastMessageId, @@ -252,7 +253,19 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat dispatch(realmInit(initData, serverSettings.zulip_version)); 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());