Skip to content

Refactor connection locks #267

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

Merged
merged 5 commits into from
Apr 22, 2025
Merged

Refactor connection locks #267

merged 5 commits into from
Apr 22, 2025

Conversation

simolus3
Copy link
Contributor

The existing implementation around connect() and disconnect() has a bit of an odd control flow. I couldn't find any likely race conditions, but this still cleans up the implementation a bit to be easier to reason about. In particular, some of the things this addresses:

  1. While we acquire a mutex for connect(), we were not doing this for disconnect(). So if we call connect() and then immediately call disconnect(), it's possible that disconnect() does nothing because the AbortController has not yet been set.
  2. reconnect() doesn't actually check if we still need to connect, so a disconnect() call followed by an error on the isolate could cause another connect() that is not stopped.
  3. We set an onExit port when spawning the isolate, but there's a separate Isolate.current.addOnExitListener command in the background isolate. The two trigger different behavior, this has now been cleaned up.
  4. The Isolate.spawn call is not awaited, meaning that another connect() call could begin before the earlier one started setting up the sync client. I don't think this causes any issues, but it's still better to restrict concurrency around connect().

This also adds a lot of internal assertions to ensure we're not calling the internal connect methods concurrently. I'm still not sure how the problem with duplicate sync streams was caused though, the existing implementation should have guarded against that too.

@simolus3 simolus3 requested a review from stevensJourney April 15, 2025 14:17
@simolus3 simolus3 force-pushed the refactor-connection-locks branch from add4b1c to 3387dc0 Compare April 15, 2025 14:18
stevensJourney
stevensJourney previously approved these changes Apr 16, 2025
Copy link
Contributor

@stevensJourney stevensJourney 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 changes from my side.

Copy link
Contributor

@stevensJourney stevensJourney left a comment

Choose a reason for hiding this comment

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

The implementation looks solid from my side. I could not spot any glaring issues. I see there are some TODOs for the future, but they are documented nicely.

@simolus3 simolus3 merged commit 0f39f09 into main Apr 22, 2025
5 checks passed
@simolus3 simolus3 deleted the refactor-connection-locks branch April 22, 2025 14:35
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