Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix rare deadlock when using read/write locks #16133

Merged
merged 2 commits into from Aug 23, 2023
Merged

Conversation

erikjohnston
Copy link
Member

Introduced in #15782

@erikjohnston erikjohnston marked this pull request as ready for review August 18, 2023 12:22
@erikjohnston erikjohnston requested a review from a team as a code owner August 18, 2023 12:22
@reivilibre
Copy link
Contributor

I can't really follow what change this is making or what race it's trying to cover up, any chance you can elaborate? :)

@erikjohnston
Copy link
Member Author

Whoops! Yes, basically: we have a _mode table that tracks (for a given lock name/key) that tracks whether its been locked and if so if its a read or a write lock and a token of a currently held lock. When we drop a lock we need to either delete that row, or update the token to another token (if there is another read lock).

If we try and drop two locks at the same time, we'll try and update the _mode table to point to the token of the other lock (that we're also dropping in another transaction), and that results in a deadlock.

The fix here is to make sure that we "serialise" changes to the lock with a given name/key. We do this by taking a row level lock on _mode table before we insert and delete (and we already do that for insert).

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

not dead obvious (but mostly just because I didn't have a clue how this locking stuff was put together) but makes sense now.

@erikjohnston erikjohnston merged commit 4adaba9 into develop Aug 23, 2023
37 checks passed
@erikjohnston erikjohnston deleted the erikj/fix_deadlock branch August 23, 2023 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants