Skip to content
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.

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

When we use a fetch to "subscribe" to new data, cancel it if in progress at DEAD_QUEUE time. #5609

Closed
chrisbobbe opened this issue Dec 13, 2022 · 2 comments
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness blocked on other work To come back to after another related PR, or some other task.

Comments

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Dec 13, 2022

Often, we initialize a model with data from REGISTER_COMPLETE, then just listen for events and use them to update the model.

Sometimes, though, we "initialize" data in a model with the result of a non-initial fetch (like GET /messages), then we listen for events that affect the fetched data. The most famous example of this pattern is fetching messages; see https://github.com/zulip/zulip-mobile/blob/main/docs/architecture/realtime.md and https://zulip.readthedocs.io/en/latest/subsystems/events-system.html .

When we're using this pattern, we should cancel the fetch synchronously with dispatching the DEAD_QUEUE action, so that:

  • Our reducers don't process its payload while we're still waiting for a new event queue. If they did, we might update some UI that's reserved for live-updating data, like the message list, when we know the data won't actually get live-updated.
  • Our reducers don't process its payload after we get a new event queue, thus clobbering some state that we intentionally reset or replaced on REGISTER_COMPLETE.

We should also cancel these fetches when the active, logged-in account changes; that's #5009.

When canceling a fetch, we shouldn't forget to update any metadata that tracks its state, like state.fetching.

I believe these are the fetches we use this pattern with:

  • api.getMessages
  • api.getTopics
  • api.reportPresence (to subscribe to users that didn't exist at REGISTER_COMPLETE time)
  • (There may be more.)

Issues #4170 and #4659 cover parts of how we might do this. Marking as blocked on those.

@chrisbobbe chrisbobbe added a-data-sync Zulip's event system, event queues, staleness/liveness blocked on other work To come back to after another related PR, or some other task. labels Dec 13, 2022
@gnprice
Copy link
Member

gnprice commented Dec 16, 2022

I think we do want these fetches cancelled if they're still in progress at the time of REGISTER_COMPLETE, for this reason:

  • Our reducers don't process its payload after we get a new event queue, thus clobbering some state that we intentionally reset or replaced on REGISTER_COMPLETE.

On the other hand, though, if one of these fetches completes in the period between DEAD_QUEUE and REGISTER_COMPLETE, that seems like it's no worse than, and will sometimes be better than, having it not complete at all.

  • It's true that it won't get live-updated. But we should be showing a "Connecting…" banner or comparable notice that the stuff on the screen isn't getting live-updated, so this shouldn't be misleading.
  • If the user's device has a flaky network connection (or a slow one, and they're in a large realm where the initial data is big) then potentially it could be a while between DEAD_QUEUE and REGISTER_COMPLETE, but if we make some other fetch (e.g., they go visit some narrow and we try to fetch the messages there), that might nevertheless complete sooner. So it seems like in that case it'd be a better experience to be showing the data they asked for.

@chrisbobbe
Copy link
Contributor Author

Cool, right, makes sense. I've just filed #5623 for basically this same issue but with REGISTER_COMPLETE instead of DEAD_QUEUE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness blocked on other work To come back to after another related PR, or some other task.
Projects
None yet
Development

No branches or pull requests

2 participants