Skip to content

Clarify startEventPolling, fix a race condition #3830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Clarify startEventPolling, fix a race condition #3830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
710c9f7
eventActions: Don't wait before handling dead queue error.
Jan 22, 2020
82fc0ab
accountMisc: Add authEquivalent function.
Jan 22, 2020
0abc2cb
eventActions: Clarify loop-break logic, fix race in startEventPolling.
Jan 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/account/accountMisc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
26 changes: 15 additions & 11 deletions src/events/eventActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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 = (
Expand Down Expand Up @@ -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,
) => {
Expand All @@ -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.)
Comment on lines +64 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would make more sense immediately after the relevant await, rather than up here as far removed from it as possible. That wouldn't quite be [nfc] on its own, though, since it's entirely possible that this function is itself async-delayed (and whether or not it is is a bit fragile, anyway). You'd have to duplicate it to also occur before loop entry.

I wouldn't ordinarily suggest that... but given the duplicated parenthetical in the comment, perhaps this test should be pulled into a mini-closure anyway?

Copy link
Contributor

@rk-for-zulip rk-for-zulip May 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and also used on catch entry. If a user has logged in to another account, you probably don't want to fire off the deadQueue() logic – or, worse yet, logout().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and also used on catch entry.

Good catch! (No pun intended.)

And I agree these checks are best done immediately after the await.

Then one other point which can help keep things reasonably simple when checking this at the top of the catch: everything else in that try block doesn't need to be there, and can be moved to after the try .. catch. That error-handling code (including the backoff) only makes sense for an exception thrown inside the API call.

it's entirely possible that this function is itself async-delayed (and whether or not it is is a bit fragile, anyway). You'd have to duplicate it to also occur before loop entry.

I think the best fix for this is to move the tryGetAuth(getState()) from its caller down into the top of this function.

given the duplicated parenthetical in the comment

I think the parenthetical is best just deleted. Its content is in the summary line of the mentioned function's jsdoc anyway.

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;
}

Expand All @@ -86,14 +90,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();
}
}
};
15 changes: 14 additions & 1 deletion src/message/fetchActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { InitialData } from '../api/initialDataTypes';
import * as api from '../api';
import {
getAuth,
tryGetAuth,
getSession,
getFirstMessageId,
getLastMessageId,
Expand Down Expand Up @@ -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());

Expand Down