Skip to content

Start new event queue when old one has expired #185

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

Closed
gnprice opened this issue Jun 14, 2023 · 0 comments · Fixed by #466
Closed

Start new event queue when old one has expired #185

gnprice opened this issue Jun 14, 2023 · 0 comments · Fixed by #466
Assignees
Labels
a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) a-sync Event queue; retry; local echo; races beta feedback Things beta users have specifically asked for

Comments

@gnprice
Copy link
Member

gnprice commented Jun 14, 2023

As described in #184, when a long-poll for events fails, the current prototype just stops polling. That issue is about transient failures; this one is for an important, non-transient, but recoverable, class of error.

Namely, if the long-poll fails because the event queue is gone, we should create a new event queue and go from there. Here's from zulip-mobile at startEventPolling:

    while (true) {
      // …
      try {
        const response = await api.pollForEvents(auth, queueId, lastEventId);
        // …
      } catch (errorIllTyped) {
        const e: mixed = errorIllTyped; // https://github.com/facebook/flow/issues/2470
        // We had an error polling the server for events.

        // …

        if (e instanceof ApiError && e.code === 'BAD_EVENT_QUEUE_ID') {
          // The event queue is too old or has been garbage collected.
          dispatch(deadQueue()); // eslint-disable-line no-use-before-define
          break;

and we can use similar logic for detecting this condition.

For what to do once the condition is detected, zulip-mobile is not as good a model and we should think it through afresh. We'll need to register a new event queue, get a new initial snapshot, re-initialize data from there, and resume polling. For any open message lists, we'll also want to re-fetch messages, as that's the one major area of the data model that isn't covered by the initial snapshot. (For any minor such areas, we'll want to similarly refresh any data we have.)

I think the cleanest way to approach this will be to actually create a new PerAccountStore for the fresh data, and then tell each PerAccountStoreWidget that had the old store to replace it with the new one. Further thoughts:

  • For the bulk of our application code, this should work smoothly straight out of the box. That's because the normal pattern is to say final store = PerAccountStoreWidget.of(context); whenever in need of a PerAccountStore, and never to hang on to a PerAccountStore beyond the end of the build method or UI-event handler. That .of method creates a dependency on the ancestor widget; so when there's a new store, the descendant element will get notified and rebuilt. The rebuild will get the store afresh, as always, and therefore use the new store and won't even notice that it isn't the same object as the old store.
    • This underlines why the proper idiom is to always get a PerAccountStore fresh like that, and not to hang onto one or pass one around. Instead, pass around a BuildContext.
  • After creating a fresh PerAccountStore, we can go on to create fresh MessageListView objects corresponding to each of those that are currently registered, and have them fetch messages corresponding to the range of messages the existing views have.
    • This is one of the advantages of making a fresh PerAccountStore — those MessageListViews can refer to the new PerAccountStore while the old MessageListViews continue to refer to the old store, and there's no risk of inconsistent data from having an old view-model refer to a store with new data or vice versa.
    • → Split this extra bit of polish out as msglist: On new event queue, try to re-fetch messages before switching to new store #464.
  • The PerAccountStore can also grow a field like bool isStale, which can be set to true immediately when the expired queue is detected, while we're still working on fetching data for the new store. Then that field can drive a "Connecting…" banner, or equivalent UI for letting the user know that the data they're looking at may be out of date and the app is working on it.
@gnprice gnprice added a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) labels Jun 14, 2023
@gnprice gnprice added this to the Alpha milestone Jun 14, 2023
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jul 25, 2023
As with other uses of PerAccountStoreAwareStateMixin, we expect the
benefit to be noticeable after our planned implementation of zulip#185,
"Start new event queue when old one has expired".

At that time, this should mean that an expiring event queue won't
break autocomplete: if the lifetime of your autocomplete intent
spans across a queue-renewal, you can continue to edit your query
after the renewal, and the app will ask the new store for the query
results, instead of asking the old one.
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Jul 31, 2023
As with other uses of PerAccountStoreAwareStateMixin, we expect the
benefit to be noticeable after our planned implementation of zulip#185,
"Start new event queue when old one has expired".

At that time, this should mean that an expiring event queue won't
break autocomplete: if the lifetime of your autocomplete intent
spans across a queue-renewal, you can continue to edit your query
after the renewal, and the app will ask the new store for the query
results, instead of asking the old one.
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 1, 2023
As with other uses of PerAccountStoreAwareStateMixin, we expect the
benefit to be noticeable after our planned implementation of zulip#185,
"Start new event queue when old one has expired".

At that time, this should mean that an expiring event queue won't
break autocomplete: if the lifetime of your autocomplete intent
spans across a queue-renewal, you can continue to edit your query
after the renewal, and the app will ask the new store for the query
results, instead of asking the old one.
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 1, 2023
As with other uses of PerAccountStoreAwareStateMixin, we expect the
benefit to be noticeable after our planned implementation of zulip#185,
"Start new event queue when old one has expired".

At that time, this should mean that an expiring event queue won't
break autocomplete: if the lifetime of your autocomplete intent
spans across a queue-renewal, you can continue to edit your query
after the renewal, and the app will ask the new store for the query
results, instead of asking the old one.
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 1, 2023
As with other uses of PerAccountStoreAwareStateMixin, we expect the
benefit to be noticeable after our planned implementation of zulip#185,
"Start new event queue when old one has expired".

At that time, this should mean that an expiring event queue won't
break autocomplete: if the lifetime of your autocomplete intent
spans across a queue-renewal, you can continue to edit your query
after the renewal, and the app will ask the new store for the query
results, instead of asking the old one.
@gnprice gnprice modified the milestones: Alpha, Beta Sep 22, 2023
@gnprice gnprice modified the milestones: Beta 1, Beta 2 Dec 19, 2023
@gnprice gnprice added the beta feedback Things beta users have specifically asked for label Dec 23, 2023
@gnprice gnprice self-assigned this Dec 27, 2023
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Dec 27, 2023
@gnprice gnprice added the a-sync Event queue; retry; local echo; races label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) a-sync Event queue; retry; local echo; races beta feedback Things beta users have specifically asked for
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant