Skip to content

Refactor sync client and options #281

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

simolus3
Copy link
Contributor

This merges all options affecting the sync client (retry duration, crud throttle and client parameters) into a single object (SyncOptions) that can be passed to connect(). The old field for the retry duration and the independent parameters for throttling and client params have been deprecated in favor of those options.
The main motivation for the options class is that we'll have additional options in the future (e.g. to control whether to use the Dart or the Rust sync client, or whether to use websockets vs. HTTP streams). Having these options all in one place makes it easier to expand them later.

This also applies some refactoring to the client, with a focus on how we handle aborting syncs and errors:

  1. Instead of passing individual callbacks to the StreamingSyncImplementation, connector methods are now grouped with an interface. The old callbacks didn't have the same name as the methods on the connector, which was confusing to me (e.g. invalidCredentialsCallback should call prefetchCredentials - not exactly obvious).
  2. Fix addBroadcast to also cancel all streams when one of them emits a done event. Previously, we might leak subscriptions.
  3. The sync client had a "local ping" controller emitting null to advance the sync iteration. I've introduced sealed classes to be more precise about the event advancing the iteration.
  4. This simplifies closing the HTTP client and active streams. Previously, we would close the client to abort the current connection, and there was a comment there mentioning that "there is no other known way to cancel open streams". I'm not sure if I'm overlooking anything, but it seems to me that we can and should just use StreamSubscription.cancel? Doing that on the response stream will clean resources, and we can simply close the client afterwards. Thanks to fix number two, we can just close the "local ping" controller to cancel all source streams, which will make _streamingSyncIteration break out of the await for loop after everything has been cleaned up. So we no longer have to close the HTTP client just to cancel the stream.

Copy link
Contributor

@rkistner rkistner left a comment

Choose a reason for hiding this comment

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

Happy with the internal cases.

I can't remember why I didn't go for just cancelling the subscriptions. Just make sure to test the edge cases properly here. I think we cover at least some of it with unit/integration tests, but not sure how thorough those tests are.

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