Fix waitForProcess not closing process handles with delegate_ctlc #231
Conversation
| testKillDoubleWait | ||
| putStrLn ">>> Tests passed successfully" | ||
|
|
||
| run :: String -> IO () -> IO () |
|
I pushed some fixes for the CI failures. Once they pass I'd still like to clean up the commits a little. EDIT: This is done now (well, hoping the tests will pass next time they run.) |
| @@ -725,8 +729,6 @@ waitForProcess ph@(ProcessHandle _ delegating_ctlc _) = lockWaitpid $ do | |||
| OpenExtHandle ph' job' -> do | |||
| closePHANDLE ph' | |||
| closePHANDLE job' | |||
| when delegating_ctlc $ | |||
There was a problem hiding this comment.
Is there a reason to remove this code entirely? Is this because it's Windows-only?
There was a problem hiding this comment.
Yeah, see also the commit message:
Additionally, remove endDelegateControlC entirely from the Windows-only
OpenExtHandle branch, where it's a no-op and was also in the wrong place.
It seemed wrong to leave the misleading previous version, but also weird to write some
no-op version of the fix here since delegating_ctlc has no effect on Windows.
There was a problem hiding this comment.
I think an explicit comment in the code explaining why it's not needed would be a good thing. Otherwise someone may think it was just an oversight and add it back in the future.
endDelegateControlC throws UserInterrupt, which would cause modifyProcessHandle to roll back, leaving the handle open despite the waitpid call. Instead, we postpone the call to endDelegateControlC, as is already the case in getProcessExitCode. Additionally, remove endDelegateControlC entirely from the Windows-only OpenExtHandle branch, where it's a no-op and was also in the wrong place.
|
I ended up skipping those tests with |
I believe this might address #226, which I've run across myself when interrupting processes spawned with
delegate_ctlcwithinwithCreateProcess. It turns out this isn't a race condition (though I'm not saying there are none), it's just that the firstwithCreateProcesscall fails to close the handle when it throwsUserInterrupt.There's a test case that covers the original behaviour I saw (an uncaught exception when the subprocess is interrupted), and then a couple of test cases narrowing down to the actual bug.
I haven't tested this on Windows, and I'm not sure to what extent the tests make sense there -- we might need to adapt or skip them.