From e1fc4f7594110f39429c8ca8d542da7d132ae149 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 3 Jun 2020 12:52:11 -0400 Subject: [PATCH] make safeCommand stop the process if the thread gets killed And a comment on a todo item that this commit is perhaps the start of solving. --- Utility/SafeCommand.hs | 6 ++-- ..._9c289d93f49ff59fddd214d51356b1f9._comment | 33 +++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 doc/todo/more_extensive_retries_to_mask_transient_failures/comment_11_9c289d93f49ff59fddd214d51356b1f9._comment diff --git a/Utility/SafeCommand.hs b/Utility/SafeCommand.hs index 19d5f2026e..bb467aca3f 100644 --- a/Utility/SafeCommand.hs +++ b/Utility/SafeCommand.hs @@ -81,9 +81,9 @@ safeSystem :: FilePath -> [CommandParam] -> IO ExitCode safeSystem command params = safeSystem' command params id safeSystem' :: FilePath -> [CommandParam] -> (CreateProcess -> CreateProcess) -> IO ExitCode -safeSystem' command params mkprocess = do - (_, _, _, pid) <- createProcess p - waitForProcess pid +safeSystem' command params mkprocess = + withCreateProcess p $ \_ _ _ pid -> + waitForProcess pid where p = mkprocess $ proc command (toCommand params) diff --git a/doc/todo/more_extensive_retries_to_mask_transient_failures/comment_11_9c289d93f49ff59fddd214d51356b1f9._comment b/doc/todo/more_extensive_retries_to_mask_transient_failures/comment_11_9c289d93f49ff59fddd214d51356b1f9._comment new file mode 100644 index 0000000000..a61eca8677 --- /dev/null +++ b/doc/todo/more_extensive_retries_to_mask_transient_failures/comment_11_9c289d93f49ff59fddd214d51356b1f9._comment @@ -0,0 +1,33 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 11""" + date="2020-06-03T16:10:44Z" + content=""" +Ah, withCreateProcess is the bracketing version that's needed, it uses +cleanupProcess. And readProcess already uses it. It was easy to make +safeSystem use it, so I've already done so. Other calls to waitForProcess +probably indicate places that need to use it. + +Reading its code, I found a couple of gotchas: + +* If another thread has a Handle for the process, and are "holding the + handle lock", and lead to deadlock when cleanupProcess tries to close + them. This would seem to involve something that uses withHandle and + blocks for some reason, maybe just reading/writing to the Handle? + I've tried a few things like passing the handle into another thread + started with async, that uses hGetLine, but could not produce a deadlock, + the process was always killed anyway. + + Seems like using withAsync would help make sure any thread the handle + is passed to does get killed. + +* It sends SIGTERM, which doesn't necessarily kill every process. So + withCreateProcess might exit cleanly but leave the process hanging + around. + + I doubt this is really a problem in anything git-annex runs. + And if some program did ignore SIGTERM, wouldn't ctrl-c of git-annex + also leave that running? Never seen that happen which confirms my feeling + it's likely not a problem in something git-annex runs, but you never + know. +"""]]