-
Notifications
You must be signed in to change notification settings - Fork 33
Fix double read-lock acquisition in V1 #1746
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
jasagredo
merged 6 commits into
cardano-node-10.6-backports
from
js/fix-double-read-lock
Oct 31, 2025
Merged
Fix double read-lock acquisition in V1 #1746
jasagredo
merged 6 commits into
cardano-node-10.6-backports
from
js/fix-double-read-lock
Oct 31, 2025
+108
−40
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amesgen
reviewed
Oct 30, 2025
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V1.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V1.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V1/Lock.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V1.hs
Outdated
Show resolved
Hide resolved
6b07410 to
fc27af4
Compare
amesgen
reviewed
Oct 30, 2025
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V1.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V1/Forker.hs
Outdated
Show resolved
Hide resolved
a152e4d to
e1d2758
Compare
e1d2758 to
d668ffa
Compare
amesgen
approved these changes
Oct 31, 2025
Member
amesgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Maybe we should broaden the scope of #1704 to also (try to) simplify the resource handling of the V1 LedgerDB
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V1.hs
Outdated
Show resolved
Hide resolved
b32e7ee to
fc029b9
Compare
This was referenced Oct 31, 2025
fc029b9 to
ef34b1b
Compare
ef34b1b to
dc8a26c
Compare
Merged
via the queue into
cardano-node-10.6-backports
with commit Oct 31, 2025
9a2dfcc
15 of 18 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Double read lock acquisition
We were running an
unsafeAcquireReadAccessinside awithReadLockblock. If a write lock arrives exactly in between, the second acquisition cannot continue and neither can the write lock, deadlocking the whole node.This is solved by acquiring the read lock once, and then "transferring its ownership" onto the forker.
Track the read lock in the resource registry
As we then acquire the read lock, we need to make sure it is cleaned up properly in the presence of exceptions, so we put the allocation in the resource registry. However this means that the registry will try to double free if we actually closed the forker. So we use a TVar that contains a releasing action such that it is the null action if we created the forker.
Track the forker in the resource registry
Now we also need to track the forker once opened in the resource registry, for it to be deallocated if an exception comes. However the deallocation logic of the forker was meant to be run only once, hence it was using
takeMVar. We now usetryTakeMVarsuch that if the forker was properly closed, then the release from the resource registry is void.