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

Safe DB connection sharing #351

Merged
merged 12 commits into from
Nov 29, 2023
Merged

Safe DB connection sharing #351

merged 12 commits into from
Nov 29, 2023

Conversation

richardhuaaa
Copy link
Contributor

@richardhuaaa richardhuaaa commented Nov 28, 2023

This refactor aims to do the following:

  1. Continue allow sharing DB connections between libxmtp logic and the OpenMLS keystore, including via transactions
  2. Prevent errors arising from concurrent mutable access to this connection (whether at compile-time via multiple &mut refs or run-time via multiple RefCell borrows)
  3. Simplify the DB interface

This is done by:

  1. Create an DbConnection struct that wraps the RawDbConnection in a RefCell. The DbConnection uses interior mutability, therefore a non-mut reference to it can be freely shared as many times as desired.
  2. Move all DB operations, as well as fetch and store onto this struct. Additionally, add a method to this struct that allows external callers to execute raw queries via a closure. This ensures that the RawDbConnection is only ever accessed internally within DbConnection, and uses function scope to make sure that borrows on the RefCell are always returned before they are used again.
  3. Use visibility to ensure that the RawDbConnection is completely inaccessible outside of EncryptedMessageStore and DbConnection. It is only possible for external callers to interact with the database via DbConnection, and all references to RawDbConnection in the code have been replaced by DbConnection.

@richardhuaaa richardhuaaa force-pushed the rich/wrapped-connection branch from 44f707a to b6e9615 Compare November 29, 2023 03:10
@richardhuaaa richardhuaaa changed the title Wrapped connection WIP Safe DB connection sharing Nov 29, 2023
@richardhuaaa richardhuaaa marked this pull request as ready for review November 29, 2023 03:56
@richardhuaaa richardhuaaa requested a review from a team November 29, 2023 03:56
@richardhuaaa
Copy link
Contributor Author

This is a beefy refactor - feel free to directly modify/merge if you are blocked on this/want to avoid conflicts

@insipx
Copy link
Contributor

insipx commented Nov 29, 2023

Cleaner and so much better, nice job!

Copy link
Contributor

@neekolas neekolas left a comment

Choose a reason for hiding this comment

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

These changes look great. Think my only beef is with the name. Not sure slapping Xmtp onto the front of the name adds any useful context. Given this is the only DbConnection anyone is using, I'd advocate for just calling it something straightforward.

@richardhuaaa richardhuaaa enabled auto-merge (squash) November 29, 2023 22:35
@richardhuaaa richardhuaaa merged commit 099a525 into main Nov 29, 2023
7 checks passed
@richardhuaaa richardhuaaa deleted the rich/wrapped-connection branch November 29, 2023 22:41
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.

3 participants