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

Simplify aborting in streams #989

Merged
merged 1 commit into from
Aug 23, 2024
Merged

Conversation

neekolas
Copy link
Contributor

@neekolas neekolas commented Aug 23, 2024

Summary

  • When you receive a commit from a stream, we need to abort processing the message and sync instead. Previously we did that by decrypting the message and looking at the content to see what kind of message it was.
  • There is a better way. There was a private method on the PrivateMessageIn that I have now made public that will let us see what kind of message it is before decrypting.
  • This should make streaming more performant and reliable, since we don't have to decrypt the message twice

Related

xmtp/openmls#36

Copy link
Contributor Author

neekolas commented Aug 23, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @neekolas and the rest of your teammates on Graphite Graphite

@neekolas neekolas requested review from cameronvoell, richardhuaaa and a team August 23, 2024 17:53
@neekolas neekolas marked this pull request as ready for review August 23, 2024 17:53
@neekolas neekolas force-pushed the 08-23-simplify_aborting_in_streams branch 11 times, most recently from 47e1afa to 824b4ec Compare August 23, 2024 22:10
@@ -83,7 +85,7 @@ impl DbConnection {
entity_kind,
cursor: 0,
};
new_state.store(self)?;
new_state.store_or_ignore(self)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix was a bit of a journey. Streaming got fast enough we uncovered a race condition that has been there the whole time. When you add a member to a group and have a stream running there are two syncs that happen. One from the initial add, and one triggered by reading the message from the stream.

If there is a database error like this in the sync originating from the stream, it won't wait to process the message. Then the stream handler will try to look up the message before it has been saved to the DB and return None, making it seem like the message couldn't be processed when in fact it was just processed later.

All this was causing the test_subscribe_membership_changes test to behave flakily.

Copy link
Contributor Author

neekolas commented Aug 23, 2024

Merge activity

  • Aug 23, 3:37 PM PDT: @neekolas started a stack merge that includes this pull request via Graphite.
  • Aug 23, 3:48 PM PDT: Graphite rebased this pull request as part of a merge.
  • Aug 23, 3:53 PM PDT: @neekolas merged this pull request with Graphite.

@neekolas neekolas changed the base branch from 08-22-refactor_process_own_message_to_not_mutate_intent_state_directly to graphite-base/989 August 23, 2024 22:39
@neekolas neekolas changed the base branch from graphite-base/989 to main August 23, 2024 22:46
@neekolas neekolas force-pushed the 08-23-simplify_aborting_in_streams branch from 824b4ec to 2200c1d Compare August 23, 2024 22:47
@neekolas neekolas merged commit bda540d into main Aug 23, 2024
10 checks passed
@neekolas neekolas deleted the 08-23-simplify_aborting_in_streams branch August 23, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants