store: Replace event queue on expiry #466

gnprice · 2023-12-27T23:03:10Z

Fixes: #185

gnprice · 2023-12-27T23:07:06Z

In order to test this manually in the app, I used the following change locally:

--- a/lib/api/route/events.dart
+++ b/lib/api/route/events.dart
@@ -10,4 +10,5 @@ part 'events.g.dart';
 Future<InitialSnapshot> registerQueue(ApiConnection connection) {
   return connection.post('registerQueue', InitialSnapshot.fromJson, 'register', {
+    'queue_lifespan_secs': 1,
     'apply_markdown': true,
     'slim_presence': true,

and pointed it at my dev server, with a handful of local commits there which can be found in my dev-queue branch. Together these changes cause the event queue to be constantly lost every few seconds.

chrisbobbe

Thanks! Glad to be fixing this really visible behavior problem in the app.

I did notice some buggy behavior when I opened the "All messages" view and watched (with your helpful manual-testing steps; thanks!) how the app responded as event queues were invalidated: (link to video; you might want to watch with increased playback speed)

Possibly related to something I noticed in the code; comments below.

chrisbobbe · 2023-12-28T00:14:45Z

lib/model/store.dart

+  @override
+  void dispose() {
+    recentDmConversationsView.dispose();
+    for (final view in _messageListViews) {
+      view.dispose();
+    }
+    super.dispose();
+  }


This should also dispose unreads, right? I think that's currently the only other thing with a dispose method that needs to be called here. It seems like the kind of thing that's easy to forget.

Hmm yeah, I guess that's a ChangeNotifier and so does have a dispose. Good catch, thanks.

It seems like the kind of thing that's easy to forget.

Yeah.

chrisbobbe · 2023-12-28T00:21:20Z

test/model/test_store.dart

@@ -49,13 +58,17 @@ class TestGlobalStore extends GlobalStore {
  }

  @override
-  Future<PerAccountStore> loadPerAccount(Account account) {
-    return Future.value(PerAccountStore.fromInitialSnapshot(


Is there a reason to remove the Future.value?

Not a reason it needs to be. Sure, maybe it's clearer with the Future.value.

chrisbobbe · 2023-12-28T21:49:49Z

lib/model/store.dart

+    for (final view in _messageListViews) {
+      view.dispose();
+    }


When I have a message list open, I'm getting a concurrent modification error here, because this call to MessageListView.dispose—which is in a loop over _messageListViews—causes a call to _messageListViews.remove.

Hmm thanks. I guess I'll need to untangle that.

chrisbobbe · 2023-12-28T22:38:14Z

lib/widgets/message_list.dart

@@ -186,7 +186,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
  }

  @override
-  void onNewStore() {
+  void onNewStore() { // TODO(#464) try to keep using old model until new one gets messages
    model?.dispose();


Hmm. model, here, refers to the message-list view on the old per-account store. In the case of refreshing a dead event queue, those things (the old message-list view and the old per-account store) will have already been disposed, by the UpdateMachine, and so I think it's not appropriate to do anything with them, including this dispose call.

I was alerted to this by the following assert in PerAccountStore.unregisterMessageList, which was throwing in my manual testing:

void unregisterMessageList(MessageListView view) { final removed = _messageListViews.remove(view); assert(removed); }

One maybe-naïve fix is to just remove the model?.dispose(); line in this onNewStore. But the TODO(#464) seems fundamentally impossible to accomplish as long as the old model is disposed by the UpdateMachine before _MessageListState even learns (by a call to its onNewStore) that it has to refetch messages.

I was alerted to this by the following assert in PerAccountStore.unregisterMessageList, which was throwing in my manual testing:

Hmm, good catch, thanks.

will have already been disposed, by the UpdateMachine, and so I think it's not appropriate to do anything with them, including this dispose call.

In general dispose should be idempotent — the usual spec for it is "it's no longer valid to do anything with this object, except call dispose again".

… Or maybe not. Looking around for examples, both ChangeNotifier.dispose and State.dispose have asserts that will fail if you dispose twice.

One maybe-naïve fix is to just remove the model?.dispose(); line in this onNewStore. But the TODO(#464) seems fundamentally impossible to accomplish as long as the old model is disposed by the UpdateMachine before _MessageListState even learns (by a call to its onNewStore) that it has to refetch messages.

Yeah, #464 will definitely call for the PerAccountStore.dispose to get delayed. One thing that'll be a bit tricky in implementing that issue will be to ensure that it does eventually get disposed — something like, when it's already stale and then its last MessageListView goes away.

But in the meantime, this code can be written for the way that PerAccountStore.dispose currently works — so it can just drop this dispose call.

gnprice · 2023-12-28T23:27:07Z

I did notice some buggy behavior when I opened the "All messages" view and watched (with your helpful manual-testing steps; thanks!) how the app responded as event queues were invalidated: (link to video;

Wacky, yeah, thanks. Even with that assert throwing, I'm not sure quite how it ends up with that symptom. I guess the exception propagates up into the framework where it's trying to update the tree, and it ends up with the old MessageList hanging around while it adds a new one as a sibling. Anyway, presumably fixing that assert will fix it.

It seems like I must have neglected to repeat the manual testing after I added the dispose methods. Glad you caught these, thanks.

gnprice · 2023-12-29T00:06:10Z

Thanks for the review! Revision pushed.

I found there were also some TODO(#185) comments, about updating docs around PerAccountStoreAwareStateMixin. I acted on those, and also revised those docs to say to ignore the old store rather than to remove listeners on it — in other words to reflect the revision that was needed in that _MessageListState.onNewStore method.

…ntStore This change happened in 5c778bd, but some comments didn't get updated at the same time, oops.

This will be useful for tests.

The `shortHash` identity tags can be helpful in debugging.

This makes GlobalStore behave a bit more conventionally for a ChangeNotifier.

Fixes: zulip#185

chrisbobbe · 2024-01-03T00:53:46Z

Thanks, LGTM! Merged, after manually testing the fix.

chrisbobbe requested changes Dec 28, 2023

View reviewed changes

gnprice force-pushed the pr-queue branch from fa2aa5e to 6e2a6c8 Compare December 29, 2023 00:04

gnprice added 10 commits January 2, 2024 16:21

store [nfc]: Update comments for UpdateMachine replacing LivePerAccou…

fdb9605

…ntStore This change happened in 5c778bd, but some comments didn't get updated at the same time, oops.

store: Add debug-only timing control to polling loop

2d97b55

This will be useful for tests.

store test: Test UpdateMachine.poll in happy cases

6a66699

store: Implement toString on PerAccountStore and friends

35a9bb0

The `shortHash` identity tags can be helpful in debugging.

store [nfc]: On loading account, notify global listeners

6ef1996

This makes GlobalStore behave a bit more conventionally for a ChangeNotifier.

store [nfc]: Add GlobalStore reference on PerAccountStore

9eafed4

store: Implement PerAccountStore.dispose

fcb05a4

store: Implement UpdateMachine.dispose

f0c76da

test: Track update machines in TestGlobalStore

7465951

store: Replace event queue on expiry

30693a8

Fixes: zulip#185

chrisbobbe force-pushed the pr-queue branch from 6e2a6c8 to 30693a8 Compare January 3, 2024 00:52

chrisbobbe merged commit 30693a8 into zulip:main Jan 3, 2024

gnprice deleted the pr-queue branch January 3, 2024 02:19

gnprice mentioned this pull request Jul 13, 2024

Slow sometimes to replace event queue after expiry #809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

store: Replace event queue on expiry #466

store: Replace event queue on expiry #466

Uh oh!

gnprice commented Dec 27, 2023

Uh oh!

gnprice commented Dec 27, 2023

Uh oh!

chrisbobbe left a comment •

edited

Loading

Uh oh!

chrisbobbe Dec 28, 2023

Uh oh!

gnprice Dec 28, 2023

Uh oh!

chrisbobbe Dec 28, 2023

Uh oh!

gnprice Dec 28, 2023

Uh oh!

chrisbobbe Dec 28, 2023

Uh oh!

gnprice Dec 28, 2023

Uh oh!

chrisbobbe Dec 28, 2023

Uh oh!

gnprice Dec 28, 2023

Uh oh!

gnprice commented Dec 28, 2023

Uh oh!

gnprice commented Dec 29, 2023

Uh oh!

chrisbobbe commented Jan 3, 2024

Uh oh!

Uh oh!

store: Replace event queue on expiry #466

store: Replace event queue on expiry #466

Uh oh!

Conversation

gnprice commented Dec 27, 2023

Uh oh!

gnprice commented Dec 27, 2023

Uh oh!

chrisbobbe left a comment • 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!

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!

gnprice commented Dec 28, 2023

Uh oh!

gnprice commented Dec 29, 2023

Uh oh!

chrisbobbe commented Jan 3, 2024

Uh oh!

Uh oh!

chrisbobbe left a comment •

edited

Loading