[commit: ghc] master: RTS/IOManager: fix trac issue #9722. (74625d6)

[git at git.haskell.org]

Repository : ssh://git@git.haskell.org/ghc

On branch  : master
Link       : http://ghc.haskell.org/trac/ghc/changeset/74625d6847e970e8bdc6991c327515b3e10b231b/ghc

>---------------------------------------------------------------

commit 74625d6847e970e8bdc6991c327515b3e10b231b
Author: Andreas Voellmy <andreas.voellmy at gmail.com>
Date:   Mon Mar 9 18:27:41 2015 -0400

    RTS/IOManager: fix trac issue #9722.
    
    Summary:
    Whenever the RTS has been inactive for idleGCDelayTime, the idle timer
    fires and calls wakeUpRts(), which in turn calls ioManagerWakeup(),
    which in turn writes a byte (or a few) to a file descriptor (stored in
    the io_manager_wakeup_fd variable) registered by the TimerManager and
    on which the TimerManager will wait. (Note that the write will only
    occur if the file descriptor is non-negative.) When the RTS shuts
    down, it shuts down the TimerManager, and in this process the file
    descriptor stored in io_manager_wakeup_fd is closed. In the error
    case, the idle timer fires after the close of the file occurs, and
    then the write() call in ioManagerWakeup() fails and the
    aforementioned error message gets printed.
    
    This patch solves the problem by (1) having the TimerManager (via
    Control) write -1 to io_manager_wakeup_fd just before closing the file
    descriptor written in io_manager_wakeup_fd, and (2) having
    ioManagerWakeup() ignore an error returned by write() in the case that
    the write returned -1 and the io_manager_wakeup_fd is -1.
    
    Reviewers: austin, simonmar, hvr, thomie
    
    Reviewed By: thomie
    
    Subscribers: thomie
    
    Differential Revision: https://phabricator.haskell.org/D722
    
    GHC Trac Issues: #9722


>---------------------------------------------------------------

74625d6847e970e8bdc6991c327515b3e10b231b
 libraries/base/GHC/Event/Control.hs |  7 +++++++
 rts/posix/Signals.c                 | 17 +++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/libraries/base/GHC/Event/Control.hs b/libraries/base/GHC/Event/Control.hs
index 747a416..5dcc66e 100644
--- a/libraries/base/GHC/Event/Control.hs
+++ b/libraries/base/GHC/Event/Control.hs
@@ -68,6 +68,7 @@ data Control = W {
     , wakeupReadFd   :: {-# UNPACK #-} !Fd
     , wakeupWriteFd  :: {-# UNPACK #-} !Fd
 #endif
+    , didRegisterWakeupFd :: !Bool
     } deriving (Show)
 
 #if defined(HAVE_EVENTFD)
@@ -108,13 +109,19 @@ newControl shouldRegister = allocaArray 2 $ \fds -> do
            , wakeupReadFd   = fromIntegral wake_rd
            , wakeupWriteFd  = fromIntegral wake_wr
 #endif
+           , didRegisterWakeupFd = shouldRegister
            }
 
 -- | Close the control structure used by the IO manager thread.
+-- N.B. If this Control is the Control whose wakeup file was registered with
+-- the RTS, then *BEFORE* the wakeup file is closed, we must call
+-- c_setIOManagerWakeupFd (-1), so that the RTS does not try to use the wakeup
+-- file after it has been closed.
 closeControl :: Control -> IO ()
 closeControl w = do
   _ <- c_close . fromIntegral . controlReadFd $ w
   _ <- c_close . fromIntegral . controlWriteFd $ w
+  when (didRegisterWakeupFd w) $ c_setIOManagerWakeupFd (-1)
 #if defined(HAVE_EVENTFD)
   _ <- c_close . fromIntegral . controlEventFd $ w
 #else
diff --git a/rts/posix/Signals.c b/rts/posix/Signals.c
index 5fbb917..a2fa07f 100644
--- a/rts/posix/Signals.c
+++ b/rts/posix/Signals.c
@@ -126,7 +126,7 @@ more_handlers(int sig)
 }
 
 // Here's the pipe into which we will send our signals
-static int io_manager_wakeup_fd = -1;
+static volatile int io_manager_wakeup_fd = -1;
 static int timer_manager_control_wr_fd = -1;
 
 #define IO_MANAGER_WAKEUP 0xff
@@ -161,7 +161,20 @@ ioManagerWakeup (void)
         StgWord8 byte = (StgWord8)IO_MANAGER_WAKEUP;
         r = write(io_manager_wakeup_fd, &byte, 1);
 #endif
-        if (r == -1) { sysErrorBelch("ioManagerWakeup: write"); }
+        /* N.B. If the TimerManager is shutting down as we run this
+         * then there is a possiblity that our first read of
+         * io_manager_wakeup_fd is non-negative, but before we get to the
+         * write the file is closed. If this occurs, io_manager_wakeup_fd
+         * will be written into with -1 (GHC.Event.Control does this prior
+         * to closing), so checking this allows us to distinguish this case.
+         * To ensure we observe the correct ordering, we declare the
+         * io_manager_wakeup_fd as volatile.
+         * Since this is not an error condition, we do not print the error
+         * message in this case.
+         */
+        if (r == -1 && io_manager_wakeup_fd >= 0) {
+            sysErrorBelch("ioManagerWakeup: write");
+        }
     }
 }