Skip to content

Simplify control flow in sync client#410

Open
simolus3 wants to merge 3 commits intomainfrom
fix-reconnect-delay-after-subscription-change
Open

Simplify control flow in sync client#410
simolus3 wants to merge 3 commits intomainfrom
fix-reconnect-delay-after-subscription-change

Conversation

@simolus3
Copy link
Copy Markdown
Contributor

This closes #409. Previously, there was a potential race condition when adding new sync streams because we:

  1. First added a AbortCurrentIteration local sync event.
  2. Which is then handled asynchronously to break out of the client loop once handled, but it's possible for additional server-side events to come in before we get to that.

Instead of handling close events asynchronously, this refactors the control flow of the sync client, allowing us to just break out of a loop when the core extension tells us to instead of having to go through these hoops. We do this by making all instructions that won't interrupt a sync iteration extend a common class, so we can safely express which instructions we can handle in a separate methods and which ones need to be part of the loop.

This also updates the core extension and removes a workaround for a package:http bug related to cancelling streams on the web.

@override
Future<void> streamingSync() async {
assert(_abort == null);
final abort = _abort = AbortController();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This ensures a _rustStreamingSyncIteration always has access to the abort controller from its iteration and avoids a bunch of non-null assertions.

_crudLoop();
var invalidCredentials = false;
while (!aborted) {
_state.updateStatus((s) => s.setConnectingIfNotConnected());
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The core extension sets connecting: true on start, so no need to replicate that.

if (invalidCredentials) {
// This may error. In that case it will be retried again on the next
// iteration.
await connector.prefetchCredentials();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know what the point of this is - invalidCredentials is set on all exceptions but we also don't actually invalidate credentials? We already drop cached credentials on a 401 error, and obviously we fetch credentials when starting a connection.

But I might be missing something, so a double-check would be appreciated :D

@simolus3 simolus3 requested a review from stevensJourney April 23, 2026 15:54
@simolus3 simolus3 marked this pull request as ready for review April 23, 2026 15:54
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.

Performance regression in desktop integration tests after upgrading 1.18.0 to 2.0.1

1 participant