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.

Sign up for GitHub

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

Jump to bottom

More explicit resource management in ChainSync client and server #2893

Merged
merged 4 commits into from
Jan 26, 2021
Merged

More explicit resource management in ChainSync client and server #2893

merged 4 commits into from
Jan 26, 2021

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Jan 22, 2021 •
edited
Loading

Fixes #2892.

@nfrisby nfrisby linked an issue Jan 22, 2021 that may be closed by this pull request
Introduce the bracketWithRegistry combinator #2892
Closed
@nfrisby nfrisby added the consensus issues related to ouroboros-consensus label Jan 22, 2021
@nfrisby nfrisby changed the title Be even more explicit about resource management in ChainSync client and server More explicit resource management in ChainSync client and server Jan 22, 2021
@nfrisby
Copy link
Contributor Author

nfrisby commented Jan 22, 2021

@edsko This is what we talked about on Wednesday; let's scan it on our Friday call.

@nfrisby nfrisby force-pushed the nfrisby/issue-2892-regbracket branch from 025ef08 to caf1c7c Compare January 22, 2021 02:42
@nfrisby nfrisby force-pushed the nfrisby/issue-2892-regbracket branch 2 times, most recently from c8132f2 to a5c9769 Compare January 26, 2021 04:43
@nfrisby nfrisby marked this pull request as ready for review January 26, 2021 14:57
edsko
edsko approved these changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@edsko edsko left a comment

Choose a reason for hiding this comment

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

Subtle stuff, as always with the registry. Thanks for your observation @kderme , and well-spotted on the more general problem @nfrisby .

Peer-reviewed this over meet; good to go after some minor cleanup, the more general problem will be addressed separately.

@nfrisby nfrisby force-pushed the nfrisby/issue-2892-regbracket branch 2 times, most recently from e347aaf to 54433a4 Compare January 26, 2021 19:55
@nfrisby
consensus: introduce Watcher, use instead of registry in ChainSync cl…
a3bfbc3 
…ient

The ChainSync client was using the registry to manage a thread that could
simply be managed instead with a bracket. I cleaned up the
`Ouroboros.Consensus.Util.STM` module by adding this `Watcher` abstraction in
order to do just that. It is now up to the callee to decide if they want to
spawn the `Watcher` via `ResourceRegistry` or via a bracket.
@nfrisby
consensus: change onKnownSlotChange to knownSlotWatcher
e26d1f5 
This commit is propagating the new Watcher abstraction through an abbreviation.
@nfrisby
consensus: change onEachTick to tickWatcher
ca30193 
This commit is propagating the new Watcher abstraction through an abbreviation.
@nfrisby
consensus: add and use bracketWithPrivateRegistry
bcf8af8 
See the new declaration's docstring. This is intended to prevent the confusion
that lead to the recently fixed resource link the ChainSync server.
@nfrisby nfrisby force-pushed the nfrisby/issue-2892-regbracket branch from 54433a4 to bcf8af8 Compare January 26, 2021 20:00
@nfrisby
Copy link
Contributor Author

nfrisby commented Jan 26, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 26, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 4f2e9ca into master Jan 26, 2021
@iohk-bors iohk-bors bot deleted the nfrisby/issue-2892-regbracket branch January 26, 2021 20:30
@infinisil infinisil mentioned this pull request Mar 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kderme kderme kderme left review comments

@edsko edsko edsko approved these changes

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce the bracketWithRegistry combinator
3 participants
@nfrisby @edsko @kderme