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.

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

[LifecycleManager] Adding shutdown signal #1077

Merged
merged 5 commits into from
Aug 4, 2021

Conversation

synzhu
Copy link
Contributor

@synzhu synzhu commented Aug 3, 2021

This PR updates LifecycleManager to have a shutdown signal which can be used by the component using it to know when shutdown has commenced.

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2021

Codecov Report

Merging #1077 (d4093fb) into master (95c99c0) will decrease coverage by 0.00%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
- Coverage   53.34%   53.34%   -0.01%     
==========================================
  Files         318      318              
  Lines       21556    21559       +3     
==========================================
  Hits        11500    11500              
- Misses       8484     8487       +3     
  Partials     1572     1572              
Flag Coverage Δ
unittests 53.34% <66.66%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
module/lifecycle/lifecycle.go 98.18% <66.66%> (-1.82%) ⬇️
module/mempool/epochs/transactions.go 94.73% <0.00%> (-5.27%) ⬇️
cmd/util/ledger/migrations/storage_v4.go 41.56% <0.00%> (-0.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95c99c0...d4093fb. Read the comment docs.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Discussed that offline, but I wonder if this simple approach isn't just a detour on the way towards threading a context. See https://github.com/dapperlabs/flow-go/issues/5720

@@ -57,6 +57,7 @@ type LifecycleManager struct {
stopped chan struct{} // used to signal that shutdown has completed
startupCommenced bool // indicates whether OnStart() has been invoked
shutdownCommenced bool // indicates whether OnStop() has been invoked
shutdownSignal chan struct{} // used to signal that shutdown has commenced
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this already conveyed by shutdownCommenced ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but shutdownCommenced is not something which the user of the lifecycle manager has access to. This feature enables someone to "subscribe" to the shutdown commenced event (by reading from the channel), so goroutines launched by the user of the lifecycle can use this to know when they are signalled to shutdown

@synzhu synzhu merged commit bbc7976 into master Aug 4, 2021
@synzhu synzhu deleted the smnzhu/lifecycle-manager-upgrades branch August 4, 2021 17:08
@synzhu
Copy link
Contributor Author

synzhu commented Aug 4, 2021

Discussed that offline, but I wonder if this simple approach isn't just a detour on the way towards threading a context. See dapperlabs/flow-go#5720

@huitseeker Thanks for bringing this up, after some more thought I actually tend to agree. However, I realized that actually a lot more work needs to be done to refactor our codebase to be able to use contexts properly, at the root of which lies splitting the capability to check the state of a component (ReadyDone) from the capability to start and stop it (a new Runnable interface with a method Run(context.Context)).

I think Leo actually had a good point here about each component having a single owner. What I realize now is that we actually want to model a Go program as a call-graph where each goroutine has a single parent:

If a goroutine is responsible for creating a goroutine, it is also responsible for ensuring it can stop the goroutine.

If we have this, then contexts provide a very composable, elegant solution for managing branches of our call-graph:

  • If your component needs to cancel functions below it in the call-graph in some manner, then it will call one of these functions and pass in the Context it was given, and then pass the Context returned into its children.
  • If component doesn’t need to modify the cancellation behavior, then it simply passes on the Context it was given.

In this way, successive layers of the call-graph can create a Context that adheres to their needs without affecting their parents.

By splitting into two interfaces, we can ensure that only a single goroutine will have access to a component's Run method, while multiple components may have access to Ready and Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants