waitForProcess race condition

[joeyh]

    -- don't hold the MVar while we call c_waitForProcess...
    -- (XXX but there's a small race window here during which another
    -- thread could close the handle or call waitForProcess)

This is a small race condition indeed, but today I managed to stumble over it, on a Linux system. I have a situation where 2 threads both need to wait for the same process to exit, so both call waitForProcess. One of the threads would then fairly reliably crash with "waitForProcess: does not exist (No child processes)"

Of course, I can work around it in my code by adding my own locking so only 1 thread calls waitForProcess on a process at a time. It could be similarly fixed in System.Process.. add another MVar to ProcessHandle, and make waitForProcess use it to prevent concurrent calls to c_waitForProcess

[snoyberg]

I worked around this limitation in streaming-commons. I'm not sure if
there's any advantage to the current structuring of the code, are you aware
of any downsides to changing it? And are you interested in sending a PR?

On Wed, Oct 28, 2015, 12:41 PM Joey Hess notifications@github.com wrote:

-- don't hold the MVar while we call c_waitForProcess...
-- (XXX but there's a small race window here during which another
-- thread could close the handle or call waitForProcess)

This is a small race condition indeed, but today I managed to stumble over
it, on a Linux system. I have a situation where 2 threads both need to wait
for the same process to exit, so both call waitForProcess. One of the
threads would then fairly reliably crash with "waitForProcess: does not
exist (No child processes)"

Of course, I can work around it in my code by adding my own locking so
only 1 thread calls waitForProcess on a process at a time. It could be
similarly fixed in System.Process.. add another MVar to ProcessHandle, and
make waitForProcess use it to prevent concurrent calls to c_waitForProcess

—
Reply to this email directly or view it on GitHub
#46.

[charles-cooper]

I ran into the same issue today, on a Linux machine (RTS -N, kernel 4.4.0). I also ended up working around it by catching the exception, which looks like the approach you both have taken.

I wrote up a fix along the lines of @joeyh's suggestion at charles-cooper@fa0d45b; I won't have time to test it for a couple days but @snoyberg perhaps you could take a look and see if I'm heading in the right direction?

[snoyberg]

It seems reasonable. My concerns would be (1) a negative performance hit and (2) breaking backwards compatibility. However, I lean towards doing this, as the bug here is significant enough. I'll look into this some time next week.

[charles-cooper]

Upon further inspection it seems the bug is due not to a race condition but failure to interpret the return code of waitpid. From the documentation (https://www.mkssoftware.com/docs/man3/waitpid.3.asp):

If more than one thread is suspended in waitpid() awaiting termination of the same process, exactly one thread returns the process status at the time of the target child process termination. The other threads return -1, with errno set to ECHILD.

Handling the case accordingly (charles-cooper@e70eb47) seems to fix the bug.

[ndmitchell]

I've been observing a similar bug on Windows for a while, and only today did I manage to track it down. If I call waitForProcess in the form:

forkIO $ void $ waitForProcess $ process ghci
void $ waitForProcess $ process ghci

Then I get an error about 5% of the time. A most annoying bug, and one I would encourage something be done to fix.