Cross-platform clean shutdown mechanism #767
Merged
Conversation
This file contains 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
rvl
reviewed
Apr 7, 2020
| @@ -249,6 +252,15 @@ parseValidateDB = | |||
| <> help "Validate all on-disk database files" | |||
| ) | |||
|
|
|||
| parseShutdownIPC :: Parser (Maybe Fd) | |||
There was a problem hiding this comment.
I don't think this will compile on Windows because it's HANDLE there. Perhaps use Int here and sort out the rest when it's time to use fdToHandle.
There was a problem hiding this comment.
GHC uses the MS C lib on Windows which does actually handle these Fds.
| where | ||
| waitForEOF :: IO () | ||
| waitForEOF = do | ||
| hnd <- IO.fdToHandle fd |
There was a problem hiding this comment.
This will need some CPP for Windows.
There was a problem hiding this comment.
Same here. Surprisingly this is not Unix specific code.
| | otherwise -> throwIO e | ||
|
|
||
| Right _ -> | ||
| throwIO $ IO.userError "--shutdown-ipc FD does not expect input" |
There was a problem hiding this comment.
Good idea. Reporting an error in this case is probably more sensible than consuming the input.
cardano-node/src/Cardano/Node/Run.hs
Outdated
| withShutdownHandler :: NodeProtocolMode -> Tracer IO String -> IO () -> IO () | ||
| withShutdownHandler (RealProtocolMode NodeCLI{shutdownIPC = Just (Fd fd)}) | ||
| tracer action = | ||
| race_ waitForEOF action |
There was a problem hiding this comment.
Will this need application of the forkIO/MVar hack to avoid deadlocking on Windows?
There was a problem hiding this comment.
Ah yes you're right. Sigh. Because this uses cancel on the left action when the right one finishes, and that will never complete...
dcoutts
force-pushed
the
dcoutts/issue-726-windows-clean-shutdown
branch
from
ca9fef4 to
8fe713d
Compare
Otherwise every time we add/remove/change fields in these records then a dozen places in the code need updating. Also add a TODO about the structure of these types. They now share nothing but this is really too extreme, as demonstrated by the places that this patch touches. There are a lot of places where we extract the same information for both mock and real modes. So we should share the things that are always the same.
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
force-pushed
the
dcoutts/issue-726-windows-clean-shutdown
branch
from
8fe713d to
45d2a06
Compare
3 tasks
Jimbo4350
reviewed
Apr 8, 2020
| -- process itself terminated, then we initiate a clean shutdown. | ||
| -- | ||
| withShutdownHandler :: NodeProtocolMode -> Tracer IO String -> IO () -> IO () | ||
| withShutdownHandler (RealProtocolMode NodeCLI{shutdownIPC = Just (Fd fd)}) |
deepfire
reviewed
Apr 8, 2020
| -> Consensus.TopLevelConfig blk | ||
| -> IO () | ||
| createTracers npm' tr tracer cfg = do | ||
| case npm' of | ||
| RealProtocolMode (NodeCLI _ rNodeAddr _ runDBValidation) -> do | ||
| RealProtocolMode NodeCLI{nodeAddr, validateDB} -> do |
There was a problem hiding this comment.
So NamedFieldPuns are good to go, unlike what was being categorically told by people.
And yes, personally, they make complete sense here.
There was a problem hiding this comment.
Yes. NamedFieldPuns are great. RecordWildcards is what makes some people grumpy.
rvl
reviewed
Apr 8, 2020
|
I've ran some tests on the Furthermore, integration tests are passing for non-brutal shutdowns working correctly. |
|
bors merge |
Build succeeded |
rvl
added a commit
to cardano-foundation/cardano-wallet
that referenced
this pull request
Apr 9, 2020
This modifies the clean shutdown handler to use a FD from the command line, rather than taking stdin. It also makes the logic even simpler, as shown in IntersectMBO/cardano-node#767 The command-line arguments are changed from --shutdown-handler to --shutdown-ipc FD so that they are identical to cardano-node.
rvl
added a commit
to cardano-foundation/cardano-wallet
that referenced
this pull request
Apr 10, 2020
This modifies the clean shutdown handler to use a FD from the command line, rather than taking stdin. It also makes the logic even simpler, as shown in IntersectMBO/cardano-node#767 The command-line arguments are changed from --shutdown-handler to --shutdown-ipc FD so that they are identical to cardano-node.
rvl
added a commit
to cardano-foundation/cardano-wallet
that referenced
this pull request
Apr 11, 2020
This modifies the clean shutdown handler to use a FD from the command line, rather than taking stdin. It also makes the logic even simpler, as shown in IntersectMBO/cardano-node#767 The command-line arguments are changed from --shutdown-handler to --shutdown-ipc FD so that they are identical to cardano-node.
rvl
added a commit
to cardano-foundation/cardano-wallet
that referenced
this pull request
Apr 12, 2020
This modifies the clean shutdown handler to use a FD from the command line, rather than taking stdin. It also makes the logic even simpler, as shown in IntersectMBO/cardano-node#767 The command-line arguments are changed from --shutdown-handler to --shutdown-ipc FD so that they are identical to cardano-node.
rvl
added a commit
to cardano-foundation/cardano-wallet
that referenced
this pull request
Apr 15, 2020
This modifies the clean shutdown handler to use a FD from the command line, rather than taking stdin. It also makes the logic even simpler, as shown in IntersectMBO/cardano-node#767 The command-line arguments are changed from --shutdown-handler to --shutdown-ipc FD so that they are identical to cardano-node.
rvl
added a commit
to cardano-foundation/cardano-wallet
that referenced
this pull request
May 5, 2020
This modifies the clean shutdown handler to use a FD from the command line, rather than taking stdin. It also makes the logic even simpler, as shown in IntersectMBO/cardano-node#767 The command-line arguments are changed from --shutdown-handler to --shutdown-ipc FD so that they are identical to cardano-node.
rvl
added a commit
to cardano-foundation/cardano-wallet
that referenced
this pull request
May 29, 2020
This modifies the clean shutdown handler to use a FD from the command line, rather than taking stdin. It also makes the logic even simpler, as shown in IntersectMBO/cardano-node#767 The command-line arguments are changed from --shutdown-handler to --shutdown-ipc FD so that they are identical to cardano-node.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Cross-platform clean shutdown support with
--shutdown-ipc FDflagFixes #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-ipcflag 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.