Skip to content

model: Message objects can be shared by message-list models, resulting in duplicated work on them #455

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.

Sign up for GitHub

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

Jump to bottom
Closed
chrisbobbe opened this issue Dec 15, 2023 · 0 comments · Fixed by #648
Closed

model: Message objects can be shared by message-list models, resulting in duplicated work on them #455

chrisbobbe opened this issue Dec 15, 2023 · 0 comments · Fixed by #648
Labels
a-model Implementing our data model (PerAccountStore, etc.) a-msglist The message-list screen, except what's label:a-content
Milestone
M3: Summer 2024

Comments

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Dec 15, 2023 •
edited
Loading

Quoting from #410 (comment) :

Oh, I've noticed a subtle bug in the model code. When you have two message lists open, and a new-message event arrives, we add the same Message object to both message lists (if it belongs in both), instead of adding copies. This means that reaction events are double-processed on a single Message object. (Processed once for each message list, on the same Message instance.)

Here's a recipe for reproducing one symptom:

  1. On your account, enable "Display names of reacting users when few users have reacted to a message."
  2. Check out this PR [unnecessary; #410 was merged] and log into your account in zulip-flutter.
  3. Open "All messages".
  4. Open a topic narrow by tapping a recipient header.
  5. Send a message in that topic.
  6. In the web app, add a reaction on the message. (Any reaction, from any user).
  7. In zulip-flutter, see the name of the reacting user (or "You") appears, as expected.
  8. Repeat step 6, but with a different reaction or user.
  9. This time, in zulip-flutter, the reacting user's name is not shown; it just shows numbers. This would be expected if there were three or more reactions on the message, but in this case there are just two.

In step 9, the [Reactions.total] is 4 instead of 2 because of the double-processing.

Greg replied there:

Interesting, yeah.

I think we can let that bug not block merging this [#410], and handle it later. The fix will probably involve having a central map of messages, akin to state.messages in zulip-mobile.
@chrisbobbe chrisbobbe added a-msglist The message-list screen, except what's label:a-content a-model Implementing our data model (PerAccountStore, etc.) labels Dec 15, 2023
@gnprice gnprice added this to the Beta 3 milestone Dec 22, 2023
This was referenced May 8, 2024
Merged
Open
Open
@gnprice gnprice mentioned this issue May 20, 2024
Merged
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue May 31, 2024
@gnprice @chrisbobbe
message: Mutate messages for reaction events here, instead of in msgl…
6e4c1e0 
…ists

This looks potentially NFC, but in fact it fixes part of 455:
it fixes the issue as far as reactions are concerned.  The change
is NFC only if the message appears in just one message list.

In addition to the symptom explained at zulip#455, this fixes another,
which exists now that we've added the central message store with
`reconcileMessages`. It's arguably a bit of an inverse of zulip#455, but
fundamentally the same issue: if there are currently *zero* message
lists that contain the message, then we would drop the update. Then
on later fetching the same message, we'd use the version we had (for
the good reason explained in reconcileMessages) but in fact it'd be
stale.

This zero-message-lists symptom also reproduces with other kinds of
updates that we handle: editing a message, and adding/removing
flags. So, in the regression test for this symptom, I've tried to
make some reusable test code that we can use when we fix those other
ways of updating messages.

Co-authored-by: Chris Bobbe <[email protected]>
Fixes-partly: zulip#455
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue May 31, 2024
@gnprice @chrisbobbe
message: Mutate messages for message edits here, instead of in msglists
fc7258b 
Like a recent commit did for reactions, this fixes the part of 455
that's concerned with message edits.

With message edits, unlike reactions, the double-processing issue
when there are two or more message lists wasn't user-facing, since
the mutation done by message lists was (naturally) idempotent. So
there isn't really an easy or helpful regression test to write for
that. But we can and do write one against the dropped-update symptom
when there are zero message lists open. For an explanation of that
symptom, see the recent commit where we dealt with reactions.

Co-authored-by: Chris Bobbe <[email protected]>
Fixes-partly: zulip#455
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue Jun 4, 2024
@gnprice @chrisbobbe
message: Store all messages centrally
3c7d46b 
In itself, this exacerbates zulip#455 by making it trigger in more
circumstances.  That's OK for the moment, because this also gives us
a foundation on which to fix that issue soon.

A key part of testing this is the line added in message_list_test's
`checkInvariants`, which checks that all messages tracked by the
MessageListView are present in the central message store.

Co-authored-by: Chris Bobbe <[email protected]>
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue Jun 4, 2024
@gnprice @chrisbobbe
message: Mutate messages for reaction events here, instead of in msgl…
ca6b5a0 
…ists

This looks potentially NFC, but in fact it fixes part of 455:
it fixes the issue as far as reactions are concerned.  The change
is NFC only if the message appears in just one message list.

In addition to the symptom explained at zulip#455, this fixes another,
which exists now that we've added the central message store with
`reconcileMessages`. It's arguably a bit of an inverse of zulip#455, but
fundamentally the same issue: if there are currently *zero* message
lists that contain the message, then we would drop the update. Then
on later fetching the same message, we'd use the version we had (for
the good reason explained in reconcileMessages) but in fact it'd be
stale.

This zero-message-lists symptom also reproduces with other kinds of
updates that we handle: editing a message, and adding/removing
flags. So, in the regression test for this symptom, I've tried to
make some reusable test code that we can use when we fix those other
ways of updating messages.

Co-authored-by: Chris Bobbe <[email protected]>
Fixes-partly: zulip#455
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue Jun 4, 2024
@gnprice @chrisbobbe
message: Mutate messages for message edits here, instead of in msglists
9c5d9d3 
Like a recent commit did for reactions, this fixes the part of 455
that's concerned with message edits.

With message edits, unlike reactions, the double-processing issue
when there are two or more message lists wasn't user-facing, since
the mutation done by message lists was (naturally) idempotent. So
there isn't really an easy or helpful regression test to write for
that. But we can and do write one against the dropped-update symptom
when there are zero message lists open. For an explanation of that
symptom, see the recent commit where we dealt with reactions.

Co-authored-by: Chris Bobbe <[email protected]>
Fixes-partly: zulip#455
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue Jun 5, 2024
@gnprice @chrisbobbe
message: Store all messages centrally
d8949c1 
In itself, this exacerbates zulip#455 by making it trigger in more
circumstances.  That's OK for the moment, because this also gives us
a foundation on which to fix that issue soon.

A key part of testing this is the line added in message_list_test's
`checkInvariants`, which checks that all messages tracked by the
MessageListView are present in the central message store.

Co-authored-by: Chris Bobbe <[email protected]>
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue Jun 5, 2024
@gnprice @chrisbobbe
message: Mutate messages for reaction events here, instead of in msgl…
273e464 
…ists

This looks potentially NFC, but in fact it fixes part of zulip#455: it
fixes the issue as far as reactions are concerned.  The change is
NFC only if the message appears in just one message list.

In addition to the symptom explained at zulip#455, this fixes another,
which exists now that we've added the central message store with
`reconcileMessages`. It's arguably a bit of an inverse of zulip#455, but
fundamentally the same issue: if there are currently *zero* message
lists that contain the message, then we would drop the update. Then
on later fetching the same message, we'd use the version we had (for
the good reason explained in reconcileMessages) but in fact it'd be
stale.

This zero-message-lists symptom also reproduces with other kinds of
updates that we handle: editing a message, and adding/removing
flags. So, in the regression test for this symptom, I've tried to
make some reusable test code that we can use when we fix those other
ways of updating messages.

Co-authored-by: Chris Bobbe <[email protected]>
Fixes-partly: zulip#455
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue Jun 5, 2024
@gnprice @chrisbobbe
message: Mutate messages for message edits here, instead of in msglists
4a59f87 
Like a recent commit did for reactions, this fixes the part of zulip#455
that's concerned with message edits.

With message edits, unlike reactions, the double-processing issue
when there are two or more message lists wasn't user-facing, since
the mutation done by message lists was (naturally) idempotent. So
there isn't really an easy or helpful regression test to write for
that. But we can and do write one against the dropped-update symptom
when there are zero message lists open. For an explanation of that
symptom, see the recent commit where we dealt with reactions.

Co-authored-by: Chris Bobbe <[email protected]>
Fixes-partly: zulip#455
chrisbobbe added a commit to gnprice/zulip-flutter that referenced this issue Jun 5, 2024
@chrisbobbe
message: Mutate messages for flags events here, instead of in msglists
d6b001c 
Like a recent commit did for message edits, and one before that did
for reactions, this fixes the part of zulip#455 that's concerned with
update-message-flags events.

Like in the commit that dealt with message edits, this fixes the
symptom of a dropped update when there are zero message lists open.
In this case I'm *pretty* sure there wasn't a user-facing symptom
with the double-processing situation when the message is in two or
more open message lists. That's because I believe we don't give
different behavior for a flag that's present multiple times in the
`flags` list, vs. just once.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
a-model Implementing our data model (PerAccountStore, etc.) a-msglist The message-list screen, except what's label:a-content
Projects
Flutter app
Status: Done
Milestone
M3: Summer 2024
Development

Successfully merging a pull request may close this issue.

Add a central message store gnprice/zulip-flutter
2 participants
@gnprice @chrisbobbe