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

Cross-platform clean shutdown #726

Closed
rvl opened this issue Mar 26, 2020 · 8 comments · Fixed by #767
Closed

Cross-platform clean shutdown #726

rvl opened this issue Mar 26, 2020 · 8 comments · Fixed by #767
Assignees
@dcoutts
Labels
byron Required for a Byron mainnet: replace the old core nodes with cardano-node. enhancement New feature or request priority high issues/PRs that MUST be addressed. The release can't happen without this;

Comments

@rvl
Copy link
Contributor

rvl commented Mar 26, 2020

Provide a method which cardano-launcher can use to politely request that cardano-node exits.

If cardano-node is killed impolitely (with SIGKILL on POSIX or TerminateProcess on Windows), it will do a complete revalidation of the chain on the next startup. This can take some minutes.

See cardano-launcher/docs/windows-clean-shutdown.md for a discussion of the issue and suggested solution.

This is the implementation in cardano-wallet - withShutdownHandler. This code is enabled when the --shutdown-handler switch is provided to the cardano-wallet CLI.
@rvl rvl added the enhancement New feature or request label Mar 26, 2020
@mrBliss
Copy link
Contributor

mrBliss commented Mar 26, 2020

If cardano-node is killed impolitely (with SIGKILL on POSIX or TerminateProcess on Windows), it will do a complete revalidation of the chain on the next startup. This can take some minutes.

Correct. But why are you killing with SIGKILL? Just use SIGTERM (which should be the default, SIGKILL should only be tried when SIGTERM doesn't work) no revalidation is needed.

@rvl
Copy link
Contributor Author

rvl commented Mar 26, 2020

Windows -- check the linked doc.

@mrBliss
Copy link
Contributor

mrBliss commented Mar 26, 2020

Windows -- check the linked doc.

I see. Windows 🙄

@dcoutts
Copy link
Contributor

dcoutts commented Apr 5, 2020

@rvl can you try GenerateConsoleCtrlEvent with either the ctc+c or break events? This may require that the launcher put the node into it's own process group, as mentioned in the MSDN docs.

https://docs.microsoft.com/en-us/windows/console/generateconsolectrlevent

This is the closest I can find to a standard method on Windows. It only works for console applications, but the node and wallet are console so that's ok.

I know that the wallet takes the approach of terminating when stdin is closed, but this isn't standard behaviour on Windows except for console applications that actually use stdin. It is also not something you want to do uniformly on other platforms: process monitors like systemd will by default connect their stdin to /dev/null so the "block until stdin is closed" trick will immediately fail.

@rvl
Copy link
Contributor Author

rvl commented Apr 6, 2020

@dcoutts Thanks for investigating.

  1. I do not know how to call GenerateConsoleCtrlEvent from nodejs. I can try to look into it. But even if it is possible, it wouldn't be as easy as closing a file.

  2. This method will not help in the case where Daedalus crashes or is killed. There would have been no chance to send ctrl-c, so the cardano-node.exe process will stay running. On the other hand, if it's trying to read stdin, an exception will be thrown and cardano-node knows that it should exit. I added a test case for this in cardano-launcher because I think this feature is essential.

  3. I totally agree that it is not something you would want to do uniformly on other platforms, and that it won't work with systemd. That's why in cardano-wallet the clean shutdown handler is only enabled when the --shutdown-handler command-line switch is provided. You can do the same in cardano-node.

@dcoutts
Copy link
Contributor

dcoutts commented Apr 6, 2020

  1. that's true.

  2. Perhaps we can do it with a specific pipe FD passed in. --shutdown-ipc FD. I don't like overloading stdin for this.

@rvl
Copy link
Contributor Author

rvl commented Apr 6, 2020

--shutdown-ipc FD would be fine. FD will be a number, e.g. 3, right?

@dcoutts
Copy link
Contributor

dcoutts commented Apr 6, 2020

Yes. The FD in the child process of the pipe you create for the purpose. Normal Unix FD / Win32 HANDLE inheritance mechanism.

@vhulchenko-iohk vhulchenko-iohk added byron Required for a Byron mainnet: replace the old core nodes with cardano-node. priority high issues/PRs that MUST be addressed. The release can't happen without this; labels Apr 7, 2020
dcoutts added a commit that referenced this issue Apr 7, 2020
@dcoutts
Cross-platform clean shutdown support with --shutdown-ipc FD flag
ca9fef4 
Fixes #726

On Windows there is no standard reliable mechanism to politely ask a
process to stop. There is only the brutal TerminateProcess. Using that
by default is not great since it means the node always has to revalidate
its chain DB on startup.

This adds an ad-hoc mechainsim that Daedalus can use to reliably shut
down the node process. The --shutdown-ipc flag takes the FD of the read
ebd of an inherited pipe. If provided, the node will monitor that pipe
when when the write end of the pipe is closed then the node will
initiate a clean shutdown. So Daedalus can explicitly terminate the node
by closing the write end of the pipe. If Daedalus terminates uncleanly
then the pipe will also be closed and the node will also shut down.

Although this mechanism is needed for Windows, it is also cross-platform
so it can be used by Daedalus across all platforms.
@dcoutts dcoutts mentioned this issue Apr 7, 2020
Merged
dcoutts added a commit that referenced this issue Apr 7, 2020
@dcoutts
Cross-platform clean shutdown support with --shutdown-ipc FD flag
632bc0d 
Fixes #726

On Windows there is no standard reliable mechanism to politely ask a
process to stop. There is only the brutal TerminateProcess. Using that
by default is not great since it means the node always has to revalidate
its chain DB on startup.

This adds an ad-hoc mechainsim that Daedalus can use to reliably shut
down the node process. The --shutdown-ipc flag takes the FD of the read
ebd of an inherited pipe. If provided, the node will monitor that pipe
when when the write end of the pipe is closed then the node will
initiate a clean shutdown. So Daedalus can explicitly terminate the node
by closing the write end of the pipe. If Daedalus terminates uncleanly
then the pipe will also be closed and the node will also shut down.

Although this mechanism is needed for Windows, it is also cross-platform
so it can be used by Daedalus across all platforms.
iohk-bors bot added a commit that referenced this issue Apr 8, 2020
@iohk-bors @dcoutts @deepfire
Merge #767
c75d2f0 
767: Cross-platform clean shutdown mechanism r=dcoutts a=dcoutts

Cross-platform clean shutdown support with `--shutdown-ipc FD` flag

Fixes #726

On Windows there is no standard reliable mechanism to politely ask a process to stop. There is only the brutal TerminateProcess. Using that by default is not great since it means the node always has to revalidate its chain DB on startup.

This adds an ad-hoc mechainsim that Daedalus can use to reliably shut down the node process. The `--shutdown-ipc` flag takes the FD of the read end of an inherited pipe. If provided, the node will monitor that pipe when when the write end of the pipe is closed then the node will initiate a clean shutdown. So Daedalus can explicitly terminate the node by closing the write end of the pipe. If Daedalus terminates uncleanly then the pipe will also be closed and the node will also shut down.

Although this mechanism is needed for Windows, it is also cross-platform so it can be used by Daedalus across all platforms.



Co-authored-by: Duncan Coutts <[email protected]>
Co-authored-by: Kosyrev Serge <[email protected]>
@iohk-bors iohk-bors bot closed this as completed in 2b88db8 Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dcoutts dcoutts

Labels
byron Required for a Byron mainnet: replace the old core nodes with cardano-node. enhancement New feature or request priority high issues/PRs that MUST be addressed. The release can't happen without this;
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Cross-platform clean shutdown mechanism IntersectMBO/cardano-node
4 participants
@mrBliss @rvl @dcoutts @vhulchenko-iohk