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

Make pool read-only, with a single write connection. #1517

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

codabrink
Copy link
Contributor

No description provided.

@@ -393,23 +396,23 @@ where
tracing::debug!("Transaction beginning");
{
let connection = self.conn_ref();
let mut connection = connection.inner_mut_ref();
let mut connection = connection.read_mut_ref();
Copy link
Contributor

Choose a reason for hiding this comment

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

Transactions are directly tied to their connection, so there may be trouble by using the read connection interchangeably with the write connection. The write transaction will have started COMMIT TRANSACTION but a BEGIN TRANSACTION pragma will never have happened because it occurred on the previous read reference

Copy link
Contributor

@insipx insipx Jan 16, 2025

Choose a reason for hiding this comment

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

This does pose another problem though, if we use begin_transaction and then release the mutex lock, I think we can still run into a db lock issue

  • begin transaction with write conn
  • do a write
  • tx releases lock
  • yield b/c tx is async and does a network call
  • begin another tx with write lock somewhere else
    • completely legal because we relinquisheed the mutex after begin_transaction
  • db lock even though we're using a mutex, because although the mutex exists in libxmtp, the transaction is open in SQLite with the same connection, which conflicts with another transaction on the same database

I think If we instead ensure the mutex is held for the entire transaction, we'll just run into a deadlock cycle waiting for access to the RwLock. Although i'm not certain on that.

Copy link
Contributor Author

@codabrink codabrink Jan 16, 2025

Choose a reason for hiding this comment

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

Yeah, you're right. It does deadlock if we do that

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