This repository was archived by the owner on Nov 15, 2023. It is now read-only.
node: fix shutdown #1308
Merged
node: fix shutdown #1308
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
bkchr
approved these changes
Dec 20, 2018
node/cli/src/lib.rs
Outdated
| let _telemetry = service.telemetry(); | ||
| drop(service); | ||
|
|
||
| // TODO [andre]: timeout this future |
There was a problem hiding this comment.
Please make a github issue for this.
node/cli/src/service.rs
Outdated
| @@ -120,7 +109,7 @@ construct_service_factory! { | |||
| grandpa::NetworkBridge::new(service.network()), | |||
| )?; | |||
|
|
|||
| executor.spawn(voter); | |||
| executor.spawn(voter.select(service.on_exit()).then(|_| Ok(()))); | |||
There was a problem hiding this comment.
Can we not take the on_exit as parameter for run_grandpa? I think that makes it more easy to use and people who write their own node does not forget to select on exit.
There was a problem hiding this comment.
agreed. longer-term I would like a special executor that just applies the exit future to everything passed to it.
I spent a little time optimizing exit-future yesterday so it should have minimal overhead now.
There was a problem hiding this comment.
I've updated the code according to your suggestions, but FWIW I think I prefer the original version because it's easier to check, we just make sure that all call-sites of spawn are exit-guarded. I think longer-term we should go with @rphmeier's suggestion though.
bkchr
reviewed
Dec 20, 2018
node/cli/src/service.rs
Outdated
| service.network(), | ||
| ); | ||
|
|
||
| executor.spawn(aura.select(service.on_exit()).then(|_| Ok(()))); |
rphmeier
added
A8-looksgood
and removed
A0-please_review
rphmeier
approved these changes
Dec 21, 2018
@andresilva, interesting! In theory it may be the real cause of #1036. Need to recheck that issue. |
gguoss
pushed a commit
to chainpool/substrate
that referenced
this pull request
Jan 11, 2019
* node: remove grandpa authority flags * node: exit-guard grandpa and aura spawned futures * node: wait for futures to stop running on shutdown * core: run connectivity tests on same ports * core: pass on_exit future when starting aura and grandpa * node: add issue number to todo * core: fix aura and grandpa tests
gguoss
pushed a commit
to chainpool/substrate
that referenced
this pull request
Jan 21, 2019
* node: remove grandpa authority flags * node: exit-guard grandpa and aura spawned futures * node: wait for futures to stop running on shutdown * core: run connectivity tests on same ports * core: pass on_exit future when starting aura and grandpa * node: add issue number to todo * core: fix aura and grandpa tests
gguoss
pushed a commit
to chainpool/substrate
that referenced
this pull request
Jan 21, 2019
* node: remove grandpa authority flags * node: exit-guard grandpa and aura spawned futures * node: wait for futures to stop running on shutdown * core: run connectivity tests on same ports * core: pass on_exit future when starting aura and grandpa * node: add issue number to todo * core: fix aura and grandpa tests
gguoss
pushed a commit
to chainpool/substrate
that referenced
this pull request
Jan 23, 2019
* node: remove grandpa authority flags * node: exit-guard grandpa and aura spawned futures * node: wait for futures to stop running on shutdown * core: run connectivity tests on same ports * core: pass on_exit future when starting aura and grandpa * node: add issue number to todo * core: fix aura and grandpa tests
gguoss
pushed a commit
to chainpool/substrate
that referenced
this pull request
Feb 11, 2019
* node: remove grandpa authority flags * node: exit-guard grandpa and aura spawned futures * node: wait for futures to stop running on shutdown * core: run connectivity tests on same ports * core: pass on_exit future when starting aura and grandpa * node: add issue number to todo * core: fix aura and grandpa tests
MTDK1
pushed a commit
to bdevux/substrate
that referenced
this pull request
Apr 12, 2019
* node: remove grandpa authority flags * node: exit-guard grandpa and aura spawned futures * node: wait for futures to stop running on shutdown * core: run connectivity tests on same ports * core: pass on_exit future when starting aura and grandpa * node: add issue number to todo * core: fix aura and grandpa tests
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Tokio doesn't run destructors for any futures still in the pool on shutdown. We blocked the tokio runtime on the outer exit future (triggered by ctrl-c), afterwards we would fire the exit signal but would not wait for all futures in the runtime to finish. Most significantly this can lead to database corruption since it isn't closed properly.
Now, after firing the exit signal we wait for the runtime to be idle, which means that all spawned futures must be guarded by the exit signal (aura and grandpa were missing).
Merge after #1306.
Fixes #1161, fixes #1034, fixes #1035.