Race condition in `waitForProcess`

[NorfairKing]

I'm not sure if this is supposed to go here or in process, but this code fails about 1 iteration in 200:

#!/usr/bin/env stack
-- stack --resolver lts-16.31 script

{-# LANGUAGE OverloadedStrings #-}

import Control.Monad
import qualified Data.ByteString as SB
import System.IO
import System.Mem
import System.Process.Typed

main :: IO ()
main =
  forM_ [1 .. 10000] $ \i -> do
    withProcessTerm (setStdin createPipe $ setStdout createPipe $ setStderr inherit $ proc "cat" []) $ \ph -> do
      putStrLn $ "Iteration: " <> show i
      let inHandle = getStdin ph
      let outHandle = getStdout ph
      let contents = "Hello world\n"
      SB.hPut inHandle contents
      hFlush inHandle
      output <- SB.hGet outHandle (SB.length contents)
      when (output /= contents) $ fail "failed to read."

I couldn't reproduce the failure without the interaction with the standard streams, so I may very well be doing something wrong, but every 200 iterations or so I see this:

Iteration: 134
Iteration: 135
Iteration: 136
repro.hs: waitForProcess: does not exist (No child processes)

[NorfairKing]

I tried reproducing this problem with process (without typed-process) but could not reproduce the problem like this:

#!/usr/bin/env stack
-- stack --resolver lts-16.31 script

{-# LANGUAGE OverloadedStrings #-}

import Control.Monad
import qualified Data.ByteString as SB
import System.IO
import System.Mem
import System.Process

main :: IO ()
main =
  forM_ [1 .. 10000] $ \i -> do
    withCreateProcess ((proc "cat" []) {std_in = CreatePipe, std_out = CreatePipe, std_err = Inherit}) $ \(Just inHandle) (Just outHandle) _ ph -> do
      putStrLn $ "Iteration: " <> show i
      let contents = "Hello world\n"
      SB.hPut inHandle contents
      hFlush inHandle
      output <- SB.hGet outHandle (SB.length contents)
      when (output /= contents) $ fail "failed to read."

[NorfairKing]

This is pretty easy to turn into a failing test. I wouldn't mind making a PR for that if it would help!

[snoyberg]

I'm not sure it's relevant to this test case failing, but the approach you're taking here is, generally, broken. You cannot write to stdin in the same thread that reads from stdout, since that can lead to deadlock on buffers filling up. Unlikely to be relevant here, but worth mentioning.

Reproducing case would be appreciated. I'll see if something jumps out at me in the code.

[snoyberg]

Also, it makes sense you can't reproduce with that process example, since it doesn't try to terminate the process afterwards (presumably where the error message is coming from).

[snoyberg]

Strangely enough, making this seemingly innocuous change seems to fix the problem on my system, can you confirm?

@@ -768,6 +768,7 @@ startProcess pConfig'@ProcessConfig {..} = liftIO $ do
                       -- then call waitForProcess ourselves
                       Left _ -> do
                           eres <- try $ P.terminateProcess pHandle
+                          P.getProcessExitCode pHandle
                           ec <-
                             case eres of
                               Left e

My theory: there's a subtle race condition in process where it's not fulfilling its responsibilities around waitForProcess being idempotent in some cases. Calling getProcessExitCode first forces the MVar to end up in a valid state, making the later call to waitForProcess succeed reliably.

[snoyberg]

The bug is definitely in process, and it's an async exception bug. It's exactly the kind of bug that typed-process is trying to work around. And unfortunately there's no obvious solution to it without breaking the goals of process.

Issue: we want waitForProcess to be interruptible. If it gets interrupted, we never get a chance to see the exit code. But if the waitpid system call succeeds, then the process is removed from the process table and can never be queried again.

In our case, the polling thread in typed-process is calling waitForProcess and gets canceled. It has to be canceled to avoid a separate race condition: terminateProcess getting called on the wrong process! However, because waitForProcess isn't actually async-exception-safe, none of this works.

I'm open to suggestions on best behavior here, but this is probably just going to remain a bug in some case no matter what, with some potential workarounds in place for common situations. The right solution is for process to handle async exceptions correctly, but I'm not sure if that's possible while allowing it to be interruptible.

[NorfairKing]

I'm not sure it's relevant to this test case failing, but the approach you're taking here is, generally, broken. You cannot write to stdin in the same thread that reads from stdout, since that can lead to deadlock on buffers filling up. Unlikely to be relevant here, but worth mentioning.

OH, good point! I hope this isn't an issue in this simple case. I just needed some example of interacting with an external process so I figured cat was the simplest example but I'm open to other suggestions.

Also, it makes sense you can't reproduce with that process example, since it doesn't try to terminate the process afterwards (presumably where the error message is coming from).

Wait what? I just had another look at the source and withCreateProcess calls cleanupProcess in a bracket (http://hackage.haskell.org/package/process-1.6.11.0/docs/src/System.Process.html#withCreateProcess) and cleanupProcess calls terminateProcess (http://hackage.haskell.org/package/process-1.6.11.0/docs/src/System.Process.html#cleanupProcess).
Am I missing something?

My theory: there's a subtle race condition in process where it's not fulfilling its responsibilities around waitForProcess being idempotent in some cases.

Yes that seems to match my intuition as well.

The bug is definitely in process, and it's an async exception bug. It's exactly the kind of bug that typed-process is trying to work around. And unfortunately there's no obvious solution to it without breaking the goals of process.
I'm open to suggestions on best behavior here, but this is probably just going to remain a bug in some case no matter what, with some potential workarounds in place for common situations. The right solution is for process to handle async exceptions correctly, but I'm not sure if that's possible while allowing it to be interruptible.

Ah shoot, that's what I was afraid of. To be quite honest, I'm not convinced that I fully grasp what's going on here, but I definitely expected my example to "just work" or at least not crash. (I'm using a fancy withX function afterall..)

Whatever we do to solve this issue, could we please make sure to have a regression test for this behaviour if we decide that this
example shouldn't crash? I'd be happy to write the necessary tests.

(P.S. I still don't understand why my process example doesn't exhibit the problem.)

[nh2]

Side note: This

      output <- SB.hGet outHandle (SB.length contents)
      when (output /= contents) $ fail "failed to read."

need not work. As far as I can tell, hGet reads "up to n" bytes, so output may well be Hello wo if the kernel decides so.

You need SB.hGetContents if you want to loop-read until the end.

Also nice repro!

[bitc]

I'm not sure I fully understand the problem, but would this idea work?

When running a process, immediately do an internal forkIO that just calls the waitpid system call. Then this mini-thread will never be interrupted directly with an async exception (and it can be internally interacted with via STM or MVars)

[snoyberg]

See my comment above

In our case, the polling thread in typed-process is calling waitForProcess and gets canceled. It has to be canceled to avoid a separate race condition: terminateProcess getting called on the wrong process! However, because waitForProcess isn't actually async-exception-safe, none of this works.