Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix graceful shutdown skipped if future ends with error #6769

Merged
3 commits merged into from
Jul 30, 2020
Merged

Fix graceful shutdown skipped if future ends with error #6769

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d3fd731
Initial commit
cecton Jul 30, 2020
a041b09
Fix graceful shutdown skipped if future ends with error
cecton Jul 30, 2020
7b224df
apply suggestion
cecton Jul 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions client/cli/src/runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,9 @@ impl<C: SubstrateCli> Runner<C> {
) -> Result<()> {
self.print_node_infos();
let mut task_manager = initialise(self.config)?;
self.tokio_runtime.block_on(main(task_manager.future().fuse()))
.map_err(|e| e.to_string())?;
let res = self.tokio_runtime.block_on(main(task_manager.future().fuse()));
self.tokio_runtime.block_on(task_manager.clean_shutdown());
Copy link
Contributor

@tomaka tomaka Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is a recurring question, but why do we even need a clean shut down? I've asked this question a couple times on Riot and nobody has ever given any answer.
It feels to me that someone one day has randomly put a line that waits for the shutdown, and nobody has ever dared remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TL;DR If we want to support any async runtime, we need to handle tasks cancelling.

All the details are here: #6642

There is also a link to the issue on async-std on the matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the question is more about why not just let the tasks be... If they are not cancelled, their objects are not properly dropped (Drop is not called).

Copy link
Contributor

Choose a reason for hiding this comment

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

The main issue (that came from parity-ethereum) was rocksdb corruption on improper shutdown. Don't remember if it has happened in substrate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That rings a bell. Maybe @arkpar would have more info on that?

Ok(())
Ok(res.map_err(|e| e.to_string())?)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ok(res.map_err(|e| e.to_string())?)
res.map_err(|e| e.to_string())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't work. I will a version with .into()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

7b224df

}

/// A helper function that runs a command with the configuration of this node
Expand Down