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

Refactor of stream_all_messages, fix flaky stream tests #835

Merged
merged 35 commits into from
Jul 16, 2024

Conversation

insipx
Copy link
Contributor

@insipx insipx commented Jun 11, 2024

  • refactors stream_all_messages to return a impl Stream
    • Limits missed messages by draining old stream and using std::mem::replace
      • missed messages are still possible, but should occur much less frequently since the surface area went from the future of creating a new stream, to the time it takes to replace the stream with mem::replace.
    • the callback now just spawns a task which loops over the stream
  • use tokio::sync::Notify to create formal sync points across streaming tests
  • remove StreamCloser duplication and instead use tokio::task::JoinHandle as StreamCloser in ffi code
    • StreamCloser now has another end function, end_and_wait. end_and_wait waits for the stream to end before exiting the function. the end function is still fire-and-forget (allowing it to remain synchronous).
  • stream functions are all uniformly synchronous which fixes the disparity between ffi & node. Stream functions return a StreamHandle that has async methods:
    • wait_for_ready: async waits until the stream is ready to accept messages
    • end_and_wait: ends the stream and waits for it to shutdown
  • end is still sync and just tells the stream to stop without blocking
  • new test for missed messages

fixes #772

Fixes most of the flaky tests, except for test_can_stream_group_messages_for_updates and test_can_stream_and_update_name_without_forking_group which are unrelated to the stream changes, and instead one of them suffers from the dblock issue (although only in CI), and the other is problematic I think b/c of publish_intents conflicts (also only in CI).

@insipx insipx changed the title quick pass at stream_all_msg refactor Slight refactor of stream_all_messages, fix flaky stream tests Jun 26, 2024
xmtp_mls/src/subscriptions.rs Outdated Show resolved Hide resolved
@insipx insipx changed the title Slight refactor of stream_all_messages, fix flaky stream tests Refactor of stream_all_messages, fix flaky stream tests Jul 1, 2024
@insipx insipx marked this pull request as ready for review July 9, 2024 19:47
@insipx insipx requested a review from a team as a code owner July 9, 2024 19:47
@insipx insipx requested a review from richardhuaaa July 9, 2024 19:47
Copy link
Contributor

@richardhuaaa richardhuaaa left a comment

Choose a reason for hiding this comment

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

Well done, this is a number of very helpful improvements! TIL about tokio::sync::notify, that helps a lot with the flaky tests. And handling the changeover between streams more gracefully is very helpful for reducing missed messages.

I think we need to propagate some changes up to the mobile SDK's, now that the stream methods are synchronous?

bindings_ffi/src/mls.rs Show resolved Hide resolved
@insipx
Copy link
Contributor Author

insipx commented Jul 11, 2024

I think we need to propagate some changes up to the mobile SDK's, now that the stream methods are synchronous?

Yeah there do need to be some minor sdk changes -- mostly from async to sync and one method has an error return removed

@insipx insipx merged commit 79b6c7b into main Jul 16, 2024
6 checks passed
@insipx insipx deleted the insipx/async-callback branch July 16, 2024 20:33
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.

Fix Flaky Test
3 participants