Stop event-polling for an account when it's logged out #5161

chrisbobbe · 2021-12-08T23:53:18Z

And make a few other cleanups and comments.

Fixes: #5022
Related: #5009

chrisbobbe · 2022-03-08T23:33:16Z

I've just rebased this past #5259. I'd appreciate a review! 🙂

gnprice

Thanks! Comments below.

gnprice · 2022-03-09T01:56:18Z

src/events/eventActions.js

      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.
+        // Pre-#5005, this will catch that the user has switched accounts,
+        //   as long as the old and new accounts' queue IDs don't collide.


Just after a conditional (at the start of the body), I like to have a short comment that states the fact we've learned is true. That then serves as an anchor for the reader to understand what comes next.

It's OK if there are some other cases where the same fact is true but that don't lead into this code path. That can be an important thing to go on to clarify, but it doesn't make the base comment invalid: if we are in this code path, then the fact is true.

If this condition holds, is the comment "The user switched accounts or logged out." not necessarily true? I feel like I'd have to do some digging to verify that it is true; but I don't see anything in this commit asserting that it isn't, only that it's incomplete.

gnprice · 2022-03-09T01:59:02Z

src/events/eventActions.js

+        //   (again, pre-#5005) is that it's caught before we're set up with
+        //   a new loop for the new account, so that the old and new account
+        //   aren't writing to the same piece of state at the same time.
+        // TODO(#5005): Remove the "Pre-#5005" comment above.


I don't understand this line. Is it saying to delete the entire paragraph above? But then there'd be no comment at all explaining what this condition is about.

(After the second commit, there's a further comment below saying to delete the whole conditional. But after the first commit there isn't; and once there is that further comment, does this line add anything distinct?)

gnprice · 2022-03-09T02:07:31Z

src/session/sessionReducer.js

+
+        // The server told us that the old queue ID is invalid. Forget it,
+        // so we don't try to use it.
+        eventQueueId: null,


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.

gnprice · 2022-03-09T02:18:48Z

src/chat/ChatScreen.js

-  // 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 ID, schedule a fetch.


nit:

Suggested change

// When we have a new event queue ID, schedule a fetch.

// When we have a new event queue, schedule a fetch.

chrisbobbe · 2022-03-09T22:32:12Z

Thanks for the review! Revision pushed. 🙂

gnprice

Thanks for the revision! Just small comments below.

gnprice · 2022-03-10T01:23:39Z

src/session/sessionReducer.js

 */
 export type PerAccountSessionState = $ReadOnly<{
+  /**
+   * The event queue ID that we're currently polling on, if any.


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

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.

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.

gnprice · 2022-03-10T01:26:17Z

src/session/sessionReducer.js

+   * - After the server tells us our old queue was invalid, and before we've
+   *   registered a new one
+   * - While this account is logged out
+   * - While this account is logged in but is not the active account


I don't think this item applies.

At present, the only per-account state we have is the one for the active account. So "this account" here is always the active account.

In a future where we have any per-account session state for accounts other than the active account, I think that will go together with maintaining an event queue for those accounts.

gnprice · 2022-03-10T01:27:46Z

src/session/sessionReducer.js

+        // Although it may be valid, forget the event queue ID -- we know
+        // the user won't care about it.


I'd frame this a bit differently:

Suggested change

// Although it may be valid, forget the event queue ID -- we know

// the user won't care about it.

// Stop polling this event queue.

gnprice · 2022-03-10T01:28:45Z

src/session/sessionReducer.js

+        // Although it may be valid, forget the old account's event queue --
+        // we know the user won't care about it. We also know we're about to
+        // request a new event queue; no use hanging on to the old one.


Similarly:

Suggested change

// Although it may be valid, forget the old account's event queue --

// we know the user won't care about it. We also know we're about to

// request a new event queue; no use hanging on to the old one.

// Stop polling this event queue. (We'll request a new one soon,

// for the new account.)

chrisbobbe · 2022-03-12T00:12:27Z

Thanks for the review! Revision pushed.

In a multi-account, post-zulip#5005 world, we're likely to want a live event queue for all the logged-in accounts. So we'll want this function to start polling for "this account", not for the global "active account". The action is already typed as a `ThunkAction`, not a `GlobalThunkAction`; that's a good start. The `auth` object we're working with after the code change is the same one as before, we just get it more directly. (`tryGetAuth` uses `getAccount`, which, pending zulip#5005, assumes "the account" is the active account; see the implementation comment in `getAccount`.) And, with zulip#5005, it will naturally change to be the Auth object for "this account", the one the caller cares about.

Fixes: zulip#5022

gnprice · 2022-03-12T02:07:13Z

Thanks! Looks good; merging.

chrisbobbe requested a review from gnprice December 8, 2021 23:53

chrisbobbe changed the title ~~Stop polling for an account on logout~~ Stop event-polling for an account when it's logged out Dec 9, 2021

chrisbobbe force-pushed the pr-stop-polling-on-logout branch from 3563894 to 227b92f Compare March 8, 2022 23:27

gnprice reviewed Mar 9, 2022

View reviewed changes

chrisbobbe force-pushed the pr-stop-polling-on-logout branch from 227b92f to d2d5543 Compare March 9, 2022 22:30

gnprice reviewed Mar 10, 2022

View reviewed changes

chrisbobbe force-pushed the pr-stop-polling-on-logout branch from d2d5543 to bed5dad Compare March 12, 2022 00:12

chrisbobbe added 3 commits March 11, 2022 18:06

session state: Give eventQueueId a jsdoc and align implementation

9b7eeb5

Fixes: zulip#5022

event polling [nfc]: Improve/clarify logic

069c82c

gnprice force-pushed the pr-stop-polling-on-logout branch from bed5dad to 069c82c Compare March 12, 2022 02:07

gnprice merged commit 069c82c into zulip:main Mar 12, 2022

chrisbobbe deleted the pr-stop-polling-on-logout branch March 12, 2022 02:14

	// When we have a new event queue ID, schedule a fetch.
	// When we have a new event queue, schedule a fetch.

		// Although it may be valid, forget the event queue ID -- we know
		// the user won't care about it.

	// Although it may be valid, forget the event queue ID -- we know
	// the user won't care about it.
	// Stop polling this event queue.

Uh oh!

Stop event-polling for an account when it's logged out #5161

Stop event-polling for an account when it's logged out #5161

Uh oh!

Conversation

chrisbobbe commented Dec 8, 2021

Uh oh!

chrisbobbe commented Mar 8, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Mar 9, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Mar 12, 2022

Uh oh!

gnprice commented Mar 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe Mar 12, 2022 •

edited

Loading