-
Notifications
You must be signed in to change notification settings - Fork 32
Fix leaking handles in uncommitted forkers #1640
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
base: js/remove-shelley-byron
Are you sure you want to change the base?
Conversation
002b71e
to
f3e4dac
Compare
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/LedgerDB/StateMachine.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V2/Forker.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V2.hs
Outdated
Show resolved
Hide resolved
2421e01
to
062bcc8
Compare
f3e4dac
to
2b121b4
Compare
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
Not in scope for this PR, but the story around safely and correctly releasing resources for forkers is quite complicated (even more so with this PR), with some parts being handled by a ResourceRegistry
and some parts by a special StrictTVar m (m ())
. I wonder whether we could make some general improvements here; maybe the concept of a temporary registry a la runWithTempRegistry
could be used to streamline this?
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V2.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/V2.hs
Show resolved
Hide resolved
I plan on merging this as a PR stack:
This is a graph of the dependencies of the PRs:
|
2b121b4
to
345d314
Compare
2939d33
to
05bdb97
Compare
05bdb97
to
450a323
Compare
450a323
to
773fa5c
Compare
No description provided.