From dca19099a95cdafcd09651dbd3fdb2e87e27b297 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 5 Jun 2020 14:56:41 -0400 Subject: [PATCH] async exception safety Masking ensures that EndStderrHandler gets written, so the helper threads shut down. However, nothing currently guarantees that calls to closeP2PSshConnection are async exception safe, so made a note about it. At this point, I've audited all calls to async, and made them all async exception safe, except for ones in the assistant, and a few in leaf commands (remotedaemon, enable-tor, multicast, p2p) which don't need to be. --- Remote/Helper/Ssh.hs | 12 +++++++----- ...ment_13_6e5fb676ae08026abeb500d01ab86414._comment | 7 ++++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/Remote/Helper/Ssh.hs b/Remote/Helper/Ssh.hs index 18098255c2..be3a9b7f3e 100644 --- a/Remote/Helper/Ssh.hs +++ b/Remote/Helper/Ssh.hs @@ -198,11 +198,13 @@ data StderrHandlerState = DiscardStderr | DisplayStderr | EndStderrHandler closeP2PSshConnection :: P2PSshConnection -> IO (P2PSshConnection, Maybe ExitCode) closeP2PSshConnection P2P.ClosedConnection = return (P2P.ClosedConnection, Nothing) -closeP2PSshConnection (P2P.OpenConnection (_st, conn, pid, stderrhandlerst)) = do - P2P.closeConnection conn - atomically $ writeTVar stderrhandlerst EndStderrHandler - exitcode <- waitForProcess pid - return (P2P.ClosedConnection, Just exitcode) +closeP2PSshConnection (P2P.OpenConnection (_st, conn, pid, stderrhandlerst)) = + -- mask async exceptions, avoid cleanup being interrupted + mask $ const $ do + P2P.closeConnection conn + atomically $ writeTVar stderrhandlerst EndStderrHandler + exitcode <- waitForProcess pid + return (P2P.ClosedConnection, Just exitcode) -- Pool of connections over ssh to git-annex-shell p2pstdio. type P2PSshConnectionPool = TVar (Maybe P2PSshConnectionPoolState) diff --git a/doc/todo/more_extensive_retries_to_mask_transient_failures/comment_13_6e5fb676ae08026abeb500d01ab86414._comment b/doc/todo/more_extensive_retries_to_mask_transient_failures/comment_13_6e5fb676ae08026abeb500d01ab86414._comment index 3625a3b448..1cb763fdb5 100644 --- a/doc/todo/more_extensive_retries_to_mask_transient_failures/comment_13_6e5fb676ae08026abeb500d01ab86414._comment +++ b/doc/todo/more_extensive_retries_to_mask_transient_failures/comment_13_6e5fb676ae08026abeb500d01ab86414._comment @@ -4,9 +4,10 @@ date="2020-06-04T19:39:23Z" content=""" I've converted everything to withCreateProcess, except for process pools -(P2P.IO, Assistant.TransferrerPool, Utility.CoProcess, and Remote.External), -which need to be handled as discussed in comment 8. And also -Git.Command.pipeReadLazy may (or may not) need to be converted. +(P2P.IO, Assistant.TransferrerPool, Utility.CoProcess, Remote.External, +and P2PSshConnectionPool), which need to be handled as discussed in +comment 8. And also Git.Command.pipeReadLazy may (or may not) need to be +converted. During this conversion, I did not watch out for interactive processes that might block on a password, so any timeout would also affect them. Really,