Skip to content

Conversation

@coolreader18
Copy link
Collaborator

@coolreader18 coolreader18 commented Feb 15, 2024

Description of Changes

Call order that would lead to deadlock:

module host: read-lock subscriptions
module host: commit tx, release lock on db
add_subscriber: take read lock on db
module host: call DatabaseUpdate::from_writes. tries to take write lock on db. cannot, add_subscriber has a read lock
add_subscriber: tries to take write lock on subscriptions. cannot, module host still holds it
deadlock

This fixes by making from_writes not take a write-lock on the db

}

fn table_name_from_id_tx<'a>(&'a self, tx: &'a Self::Tx, table_id: TableId) -> Result<Option<Cow<'a, str>>> {
Ok(tx.table_exists(&table_id).map(Cow::Borrowed))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if this is okay - the mut version does a db scan but this seems simpler but maybe it can be out of sync with the system table? idk, but this is fine for now.

@bfops bfops added bugfix Fixes something that was expected to work differently release-any To be landed in any release window labels Feb 15, 2024
Copy link
Collaborator

@jdetter jdetter left a comment

Choose a reason for hiding this comment

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

I've tested this and it does fix the deadlock, there does seem to be a correctness issue present in master but I think it probably was not introduced by this PR. On that basis I'm fine to approve 👍

@coolreader18 coolreader18 added this pull request to the merge queue Feb 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 16, 2024
@coolreader18 coolreader18 added this pull request to the merge queue Feb 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 16, 2024
@coolreader18 coolreader18 added this pull request to the merge queue Feb 16, 2024
Merged via the queue into master with commit 06affca Feb 16, 2024
@coolreader18 coolreader18 deleted the noa/fix-deadlock branch February 16, 2024 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes something that was expected to work differently release-any To be landed in any release window

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants