Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 6 additions & 5 deletions src/chat/ChatScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,12 @@ const useMessagesWithFetch = args => {
}
}, [dispatch, narrow]);

// When the event queue changes, schedule a fetch. (Currently, we never
// set this to null from a non-null value, so this really does mean the
// event queue has changed; it can't mean that we had a queue ID and
// dropped it.)
React.useEffect(scheduleFetch, [eventQueueId]);
// When we have a new event queue, schedule a fetch.
React.useEffect(() => {
if (eventQueueId !== null) {
scheduleFetch();
}
}, [eventQueueId]);

// If we stop having any data at all about the messages in this narrow --
// we don't know any, and nor do we know if there are some -- then
Expand Down
38 changes: 28 additions & 10 deletions src/events/eventActions.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
/* @flow strict-local */
import { addBreadcrumb } from '@sentry/react-native';
// $FlowFixMe[untyped-import]
import isEqual from 'lodash.isequal';

import type { GeneralEvent, ThunkAction } from '../types';
import { assumeSecretlyGlobalState } from '../reduxTypes';
import * as api from '../api';
import { logout } from '../account/accountActions';
import { deadQueue } from '../session/sessionActions';
import eventToAction from './eventToAction';
import doEventActionSideEffects from './doEventActionSideEffects';
import { tryGetActiveAccountState, tryGetAuth } from '../selectors';
import { tryGetAuth, getIdentity } from '../selectors';
import { identityOfAuth } from '../account/accountMisc';
import { BackoffMachine } from '../utils/async';
import { ApiError } from '../api/apiErrors';
import * as logging from '../utils/logging';
Expand Down Expand Up @@ -61,11 +63,9 @@ export const startEventPolling = (

// eslint-disable-next-line no-constant-condition
while (true) {
const globalState = assumeSecretlyGlobalState(getState());
const state = tryGetActiveAccountState(globalState);
const auth = state ? tryGetAuth(state) : undefined;
const auth = tryGetAuth(getState());
if (!auth) {
// There is no logged-in active account.
// This account is not logged in.
break;
}
// `auth` represents the active account. It might be different from
Expand All @@ -77,10 +77,28 @@ export const startEventPolling = (
const response = await api.pollForEvents(auth, queueId, lastEventId);
events = response.events;

if (queueId !== getState().session.eventQueueId) {
// The user switched accounts or logged out.
// TODO(#5022): TODO(#5009): This doesn't seem like an adequate
// check for that; it looks like it only detects a new REGISTER_COMPLETE.
if (getState().session.eventQueueId === null) {
// We don't want to keep polling, e.g., because we've logged out;
// see `PerAccountSessionState.eventQueueId` for other cases.
break;
} else if (!isEqual(getIdentity(getState()), identityOfAuth(auth))) {
// During the last poll, the active account changed. Stop polling
// for the previous one.
// TODO(#5005): Remove this conditional as unreachable, once `auth`
// represents the current account (instead of secretly
// representing the global "active account" which can change.)
break;
} else if (queueId !== getState().session.eventQueueId) {
// While the most recent poll was happening, another queue was
// established for this account, and we've started polling on that
// one. Stop polling on this one.
//
// In theory this could happen if you logged out of this account and
// logged back in again.
//
// TODO(#5009): Instead of this conditional, abort the
// `api.pollForEvents` immediately when `eventQueueId` becomes
// `null`, then break at that time?
break;
}
} catch (e) {
Expand Down
26 changes: 26 additions & 0 deletions src/session/sessionReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ import { getHasAuth } from '../account/accountsSelectors';
* See {@link SessionState} for discussion of what "non-persistent" means.
*/
export type PerAccountSessionState = $ReadOnly<{
/**
* The event queue ID that we're currently polling on, if any.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like this summary line! This seems like a good basis for thinking about what the behavior should be.

Copy link
Copy Markdown
Collaborator Author

@chrisbobbe chrisbobbe Mar 12, 2022

Choose a reason for hiding this comment

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

Cool!

It has one inaccuracy, possibly not worth mentioning: this could be null while we still have a poll request in progress, since we don't cancel in-progress fetches yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. Given that in that situation when the request eventually finishes, we're going to throw away the result without doing anything with it, I think it's still fair to say that we're not "currently polling on" that queue -- we do still have an HTTP request open, but as far as the app's logical behavior is concerned it already may as well not exist.

*
* Null when we're not polling on any event queue:
* - Between startup and registering a queue
* - After the server tells us our old queue was invalid, and before we've
* registered a new one
* - While this account is logged out
*/
eventQueueId: string | null,

/**
Expand Down Expand Up @@ -162,26 +171,43 @@ export default (state: SessionState = initialState, action: Action): SessionStat
...state,
needsInitialFetch: true,
loading: false,

// The server told us that the old queue ID is invalid. Forget it,
// so we don't try to use it.
eventQueueId: null,
Comment on lines +174 to +177
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, good idea.

We should probably do the same thing when the active account changes, too, right? That is, on LOGOUT, LOGIN_SUCCESS, and ACCOUNT_SWITCH. (More precisely when the logged-in active account changes: the latter two are the two ways the active account changes, and the former causes the active account to no longer be logged-in.) Those are the actions that clear most of the rest of the state.

};

case LOGIN_SUCCESS:
return {
...state,
needsInitialFetch: true,

// We're about to request a new event queue; no use hanging on to
// any old one we might have.
eventQueueId: null,
};

case LOGOUT:
return {
...state,
needsInitialFetch: false,
loading: false,

// Stop polling this event queue.
eventQueueId: null,
};

case ACCOUNT_SWITCH:
return {
...state,
needsInitialFetch: true,
loading: false,

// Stop polling this event queue. (We'll request a new one soon,
// for the new account.)
// TODO(#5005): Keep polling on accounts other than the active
// account.
eventQueueId: null,
};

case REHYDRATE:
Expand Down