Skip to content

Multiple Client Connection Locks #146

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 11 commits into from
Mar 20, 2025
Merged

Multiple Client Connection Locks #146

merged 11 commits into from
Mar 20, 2025

Conversation

stevensJourney
Copy link
Contributor

@stevensJourney stevensJourney commented Mar 18, 2025

Overview

Consumers might accidentally create multiple PowerSync clients for the same SQLite database. Calling connect on these clients simultaneously could cause the shared SQLiteDB to reach an inconsistent state. Having multiple clients for the same database could also cause issues with watched queries since each database is only aware of changes made by it's own write connection.

This PR adds static Mutex locks which prevent multiple clients of the same DB file from being connected at the same time. This also adds an internal mutex which queues multiple reconnect attempts on the same client.

A warning is logged if a PowerSync client tries to connect while the shared lock is held by another PowerSync client. In most cases consumers of this SDK should use this warning to fix multiple client instantiation issues.

Unit tests have been added to ensure the correct connect queueing behaviour. The Hello PowerSync demo has also been updated with buttons for connecting and disconnecting the client.

@stevensJourney stevensJourney requested a review from simolus3 March 18, 2025 15:44
@stevensJourney stevensJourney marked this pull request as ready for review March 18, 2025 15:44
Copy link
Contributor

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

The approach looks good to me.

I think we should also consider at least warning about the same database being opened multiple times alone (without any connect() issues). It wouldn't cause any corruption, but if one database is connected and another one isn't, update hooks wouldn't propagate changes. I think that's what one customer was seeing, so a warning may be appropriate. We can do that later too though.

@stevensJourney stevensJourney requested a review from simolus3 March 20, 2025 15:22
Copy link
Contributor

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

I'm happy with these changes. We might want to revisit the global map for mutexes at some point, but I think it's fine for now.

@stevensJourney stevensJourney merged commit 74e904e into main Mar 20, 2025
3 checks passed
@stevensJourney stevensJourney deleted the queue-connects branch March 20, 2025 16:14
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