Handle disposal of message store correctly. #909

PIG208 · 2024-08-22T23:14:43Z

Fixes: #810

PIG208 · 2024-08-22T23:29:09Z

The CI failures reproduce the error mentioned in the issue: https://github.com/zulip/zulip-flutter/actions/runs/10517036561/job/29140404556.

PIG208 · 2024-08-23T02:00:19Z

Pushed again to resolve merge conflicts.

chrisbobbe · 2024-08-27T03:07:44Z

Thanks!

[…]

This fixes a bug caused by MessageListView instances unregistering themselves
from the collection containing these instances, while we are iterating the exact
same set when disposing the message store.

[…]

For the part before the comma, MessageListView.dispose is in that call stack, right? Could you point to what's calling that? I don't think I understand yet what all the concurrent modifications are.

PIG208 · 2024-08-27T03:10:35Z

For the part before the comma, MessageListView.dispose is in that call stack, right?

Yes, this is called from PerAccountStore.dispose, which recursively disposes the message store, and the message store disposes the registered message list views.

chrisbobbe · 2024-08-28T22:13:48Z

PerAccountStore.dispose, which recursively disposes the message store

I don't understand this yet. Since you said "recursively", I'm looking for a function that, when called, causes itself to be called. Is that what you have in mind, and could you point more specifically to which function this is and where the recursive call is?

I see that the message store disposes the registered message list views, but that shouldn't cause a concurrent modification error by itself. I'm looking for what else is trying to modify the collection while that's happening, and I haven't found it yet.

PIG208 · 2024-08-28T22:24:59Z

I guess the correct word would be propagatively. The for loop modified in the fixup commit iterates over the registered message lists and calls dispose on them, those message lists then unregistered themselves to cause modification concurrent to the for loop.

chrisbobbe · 2024-08-28T22:39:09Z

Ohh, I see, yeah. Part of the work of disposing a MessageListView is to remove it from a collection of MessageListViews, and it's a problem if we're iterating over that same collection in order to dispose all its members one after another.

chrisbobbe · 2024-08-28T22:47:18Z

One commit-message clarification might be to say "removing" instead of "unregistering" in "MessageListView instances unregistering themselves from the collection". It's true that there's code involved with the name unregisterMessageList, but "remove" is the canonical term for the operation on a collection, and that operation is contributing to the concurrent modification bug.

chrisbobbe · 2024-08-28T22:50:48Z

How about, to replace the paragraph I quoted in #909 (comment) , something like:

Part of the work of disposing a MessageListView is to remove it from a collection of MessageListViews, and we've been doing this in a loop over the exact same collection (the set MessageStoreImpl._messageListViews), and this has been causing concurrent modification errors. This fix avoids those errors by having the loop iterate over a clone of that collection instead.

PIG208 · 2024-08-29T03:18:33Z

Updated the description and tweaked the comment. Thanks for the feedback!

chrisbobbe

Thanks! LGTM except for one nit below.

chrisbobbe · 2024-08-29T21:16:02Z

lib/model/message.dart

+    // When a MessageListView is disposed, it removes itself from the set
+    // `MessageStoreImpl._messageListViews`, which is the exact collection this
+    // iterates on. We need to make a copy of the collection to avoid concurrent
+    // modifications.
+    for (final view in _messageListViews.toList()) {


nit: Well, it isn't the exact collection this iterates on :) because we're avoiding doing that, for the reason this mentions. How about:

Suggested change

// When a MessageListView is disposed, it removes itself from the set

// `MessageStoreImpl._messageListViews`, which is the exact collection this

// iterates on. We need to make a copy of the collection to avoid concurrent

// modifications.

for (final view in _messageListViews.toList()) {

// When a MessageListView is disposed, it removes itself from the Set

// `MessageStoreImpl._messageListViews`. Instead of iterating on that Set,

// iterate on a copy, to avoid concurrent modifications.

PIG208 · 2024-08-29T23:39:30Z

Updated the PR. Thanks!

chrisbobbe · 2024-09-03T22:34:49Z

Thanks for the revision! In the changed comment, let's be consistent in saying "set" or "Set", and there's a comma that should be a period.

gnprice

Thanks @PIG208, and thanks @chrisbobbe for the reviews!

Generally this looks good. Small comments below.

gnprice · 2024-09-07T02:38:14Z

lib/model/message.dart

@@ -37,6 +39,9 @@ class MessageStoreImpl with MessageStore {

  final Set<MessageListView> _messageListViews = {};

+  @visibleForTesting
+  Set<MessageListView> get debugMessageLists => _messageListViews;


keep consistent names for the same thing:

Suggested change

Set<MessageListView> get debugMessageLists => _messageListViews;

Set<MessageListView> get debugMessageListViews => _messageListViews;

gnprice · 2024-09-07T02:44:05Z

test/model/message_test.dart

+    Condition<Object?> conditionMessageListView(Narrow narrow) =>
+      (it) => it.isA<MessageListView>().narrow.equals(narrow);
+
+    check(store.debugMessageStore.debugMessageLists).deepEquals([


These can be simplified:

Suggested change

check(store.debugMessageStore.debugMessageLists).deepEquals([

check(store.debugMessageListViews).deepEquals([

by putting debugMessageListViews on the MessageStore mixin and overriding it separately on the mixin's two subclasses MessageStoreImpl and PerAccountStore. That's the same pattern followed by members like messages and registerMessageList.

I like that pattern for those members because it means that code consuming the store doesn't have to worry about which sub-area of the store we've organized a given piece of data into — it's all just methods on the store. I think the same logic works for this debug member too.

That's pretty neat. Thanks!

(This also caused me to notice that I'd introduced a deviation from that pattern over on ChannelStore; sent a fix for that as #931, along with adding some docs.)

gnprice · 2024-09-07T02:47:48Z

test/model/message_test.dart

@@ -919,4 +919,23 @@ void main() {
      });
    });
  });
+
+  test('disposing multiple registered MessageListView instances', () async {


nit: organize tests in same order as code under test; here dispose comes before reconcileMessages, near top of file

gnprice · 2024-09-07T03:02:12Z

test/model/store_test.dart

+      await Future<void>.delayed(Duration.zero);
+      check(store).isLoading.isTrue();
+      check(store.debugMessageStore.debugMessageLists).isEmpty();
+
+      // The global store has a new store.
+      check(globalStore.perAccountSync(store.accountId)).not((it) => it.identicalTo(store));
+      updateFromGlobalStore();
+      check(store).isLoading.isFalse();


This can be more focused on the aspect this particular test is about:

Suggested change

await Future<void>.delayed(Duration.zero);

check(store).isLoading.isTrue();

check(store.debugMessageStore.debugMessageLists).isEmpty();

// The global store has a new store.

check(globalStore.perAccountSync(store.accountId)).not((it) => it.identicalTo(store));

updateFromGlobalStore();

check(store).isLoading.isFalse();

await Future<void>.delayed(Duration.zero);

// The old store's [MessageListView]s have been disposed.

// (And no exception was thrown; that was #810.)

check(store.debugMessageStore.debugMessageListViews).isEmpty();

The other checks are effectively covered by the preceding test case.

Meanwhile the explicit comment about no exception is helpful because in this test case that's the main point of the test. Every test case implicitly checks that no miscellaneous exceptions were thrown from the code under test; but usually that's not the main thing the test is about, so when it's not mentioned the reader might overlook it.

gnprice · 2024-09-07T03:08:08Z

test/model/message_test.dart

+    Condition<Object?> conditionMessageListView(Narrow narrow) =>
+      (it) => it.isA<MessageListView>().narrow.equals(narrow);
+
+    check(store.debugMessageStore.debugMessageLists).deepEquals([
+      conditionMessageListView(const MentionsNarrow()),
+      conditionMessageListView(const StarredMessagesNarrow()),
+    ]);


My first thought here is that this is probably clearer if conditionMessageListView just gets inlined at its two call sites. Less abstraction to work through, and not really any longer.

But I think this can actually go a step further and just check the length of the list of [MessageListView]s, as you do in the other test. The fact that it's these two particular narrows is sort of inevitable given the setup of the test, and it's not what this test is intended to check on.

(Also conceivably it could iterate in the other order. The current implementation doesn't do that, because the set comes from a set literal {} and that's a LinkedHashSet, which preserves the insertion order; but that's not a guarantee that MessageStoreImpl otherwise has any reason to make, and if it didn't then this test would sometimes break.)

gnprice · 2024-09-07T03:09:36Z

lib/model/message.dart

+    // When a MessageListView is disposed, it removes itself from the set
+    // `MessageStoreImpl._messageListViews`. Instead of iterating on that set,
+    // iterate on a copy, to avoid concurrent modifications.
+    for (final view in _messageListViews.toList()) {


nit in commit message:

This fixes a bug caused by MessageListView instances unregistering themselves from the collection containing these instances, while we are iterating the exact same set when disposing the message store.

lines too long for prose; from https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#formatting-guidelines :

Your commit message should be line-wrapped to about 68 characters per line, but no more than 70, so that your commit message will be easy to read in git log in a normal terminal.

These lines are as long as 80 columns.

See also the tip there about configuring Git and your editor.

This follows the pattern we generally use for other members of ChannelStore and MessageStore, and lets us eliminate the getter returning a ChannelStoreImpl. This pattern -- exposing all the data as members directly on the overall PerAccountStore, delegating to the underlying channel store and message store as needed -- is convenient because it means that code consuming the store doesn't have to worry about which sub-area of the store we've organized a given piece of data into: it's all just methods on the store. Noticed this was here while reviewing a PR that included a new similar deviation from this pattern: zulip#909 (comment)

PIG208 · 2024-09-07T04:25:09Z

Updated the PR, thanks for the review! Also removed the visibleForTesting annotation in this revision.

This follows the pattern we generally use for other members of ChannelStore and MessageStore, and lets us eliminate the getter returning a ChannelStoreImpl. This pattern -- exposing all the data as members directly on the overall PerAccountStore, delegating to the underlying channel store and message store as needed -- is convenient because it means that code consuming the store doesn't have to worry about which sub-area of the store we've organized a given piece of data into: it's all just methods on the store. Noticed this was here while reviewing a PR that included a new similar deviation from this pattern: #909 (comment)

PIG208 · 2024-09-12T22:26:26Z

Updated test comments and the commit message.

Part of the work of disposing a MessageListView is to remove it from a collection of MessageListViews, and we've been doing this in a loop over the same collection (MessageStoreImpl._messageListViews). This has been causing concurrent modification errors. The errors originated from this requirement of dart Iterable: > Changing a collection while it is being iterated is generally not allowed This fix avoids those errors by having the loop iterate over a clone of that collection instead. See also: https://api.flutter.dev/flutter/dart-core/Iterable-class.html Fixes: zulip#810 Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-09-16T23:23:14Z

Thanks for all the revisions! Looks good — merging.

PIG208 self-assigned this Aug 22, 2024

PIG208 force-pushed the pr-dispose branch from b72088f to fae0d0d Compare August 22, 2024 23:30

PIG208 marked this pull request as ready for review August 22, 2024 23:45

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Aug 22, 2024

PIG208 requested a review from chrisbobbe August 22, 2024 23:45

PIG208 force-pushed the pr-dispose branch from fae0d0d to 075247a Compare August 23, 2024 01:59

PIG208 force-pushed the pr-dispose branch 3 times, most recently from 685fa62 to 8d47ec5 Compare August 29, 2024 03:17

chrisbobbe reviewed Aug 29, 2024

View reviewed changes

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 29, 2024

PIG208 force-pushed the pr-dispose branch from 8d47ec5 to 9e189f7 Compare August 29, 2024 23:39

PIG208 force-pushed the pr-dispose branch 2 times, most recently from dd47ba8 to 3f14407 Compare September 4, 2024 00:15

gnprice reviewed Sep 7, 2024

View reviewed changes

PIG208 force-pushed the pr-dispose branch 2 times, most recently from f0579df to 270159c Compare September 7, 2024 03:59

PIG208 force-pushed the pr-dispose branch 4 times, most recently from fd3e9ba to 1f00049 Compare September 7, 2024 04:11

gnprice mentioned this pull request Sep 7, 2024

channel [nfc]: Document methods on ChannelStore; expose debugTopicVisibility directly #931

Merged

PIG208 requested a review from gnprice September 7, 2024 05:12

PIG208 force-pushed the pr-dispose branch 3 times, most recently from f6ca31c to 0b849f6 Compare September 12, 2024 22:25

PIG208 removed their assignment Sep 13, 2024

gnprice force-pushed the pr-dispose branch from 0b849f6 to 208d5c1 Compare September 16, 2024 23:23

gnprice merged commit 208d5c1 into zulip:main Sep 16, 2024
1 check passed

PIG208 deleted the pr-dispose branch September 17, 2024 00:19

	Set<MessageListView> get debugMessageLists => _messageListViews;
	Set<MessageListView> get debugMessageListViews => _messageListViews;

	check(store.debugMessageStore.debugMessageLists).deepEquals([
	check(store.debugMessageListViews).deepEquals([

Handle disposal of message store correctly. #909

Handle disposal of message store correctly. #909

Uh oh!

Conversation

PIG208 commented Aug 22, 2024

Uh oh!

PIG208 commented Aug 22, 2024

Uh oh!

PIG208 commented Aug 23, 2024

Uh oh!

chrisbobbe commented Aug 27, 2024

Uh oh!

PIG208 commented Aug 27, 2024

Uh oh!

chrisbobbe commented Aug 28, 2024

Uh oh!

PIG208 commented Aug 28, 2024

Uh oh!

chrisbobbe commented Aug 28, 2024

Uh oh!

chrisbobbe commented Aug 28, 2024

Uh oh!

chrisbobbe commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PIG208 commented Aug 29, 2024

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PIG208 commented Aug 29, 2024

Uh oh!

chrisbobbe commented Sep 3, 2024

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!

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!

PIG208 commented Sep 7, 2024

Uh oh!

PIG208 commented Sep 12, 2024

Uh oh!

gnprice commented Sep 16, 2024

Uh oh!

Uh oh!

Uh oh!

chrisbobbe commented Aug 28, 2024 •

edited

Loading