Add new API that can correctly wait for termination of processes forked with exec on Windows. #80

Mistuke · 2016-12-11T16:02:34Z

In the end I had to make a new API, since the handle returned by the Job should never be waited on.
Existing code would probably call the old wait function on it and then block indefinitely (or until a timeout if any was specified).

Instead I have created a new API and replaced the internals functions that do a fork/exec/wait with the new api.

I have also fixed the issue with the exit code from processes forked with exec not correctly being returned.

simonmar · 2016-12-12T09:03:15Z

System/Process.hs

+-- ---------------------------------------------------------------------------
+-- executeAndWait
+
+-- | Create a new process and wait for it's termination.


I really should stop making this typo...

simonmar · 2016-12-12T09:04:38Z

System/Process.hs

+-- ---------------------------------------------------------------------------
+-- executeAndWait
+
+-- | Create a new process and wait for it's termination.


Please explain how this is different from createProcess followed by waitForProcess. It's quite worrying to have two APIs for the same thing that have subtly different behaviour, but I didn't follow all the details of the problem. Is there really no way to make waitForProcess do the right thing?

Right, so the basic problem is this:
Say you have two programs A and B. Now A calls B. There are two ways to do this.

You can use the normal CreateProcess API, which is what normal Windows code do. Using this approach, the current waitForProcess works absolutely fine.

You can call the emulated POSIX function _exec, which of course is supposed to allow the child process to replace the parent.

With approach 2) waitForProcess falls apart because the Win32's process model does not allow this the same way as linux. _exec is emulated by first making a call to CreateProcess to spawn B and then immediately exiting from A. So you have two different processes.

waitForProcess is waiting on the termination of A. Because A is immediately killed, waitForProcess will return even though B is still running. This is why for instance the testsuite on Windows had lots of file locked errors.

This approach creates a new Job and assigned A to the job, but also all future processes spawned by A. This allows us to listen in on events, such as, when all processes in the job are finished, but also allows us to propagate exit codes from _exec calls.

The only reason we need this at all is because we don't interact with just actual native code on Windows, and instead have a lot of ported POSIX code.

I could add a new constructor to ProcessHandle and make the decision on what to do internally in the functions. This would probably generate warnings in user code that deconstruct ProcessHandle though.

Just to confirm my understanding: with the current API on Linux, if we have:

Process A calls rawSystem "B" []

Process B calls createProcess $ proc "C" [] and then exits

Then the rawSystem call in A will return when B exits, even if C is still running. With this new API on Windows, A would block until both B and C exit. Is this correct?

@Mistuke that's a really good explanation, thanks. (it would be great to include this as a comment in the code)

I could add a new constructor to ProcessHandle

But how would you know when to do that? Would this entail creating the Job for every process we spawn?

@snoyberg By default, yes. However CreateProcess has a flag CREATE_BREAKAWAY_FROM_JOB that allows a child process to break away from any parent job. So you can get the same behaviour as on linux by B using this flag when it spawns C. Though I agree, it is subtle.. (There's also a flag to prevent any children from breaking away, so it goes both ways :))

@simonmar I'll update the comment.
So there are two alternate approaches that could be taken here:

always use a job. In which case, if a user hasn't been deconstructing ProcessHandle then it should work transparently.

Combine the new function with the old one instead of having two calls now, and make the behaviour implicit on flag. But this would not be backwards compatible, since of course there's a new parameter.

I went with separate functions to avoid always using a job and breaking backwards compatibility.

If we go for 1) then, according to some very statistically unsound testing, the added overhead is about 0.0021 ms.

Should note, there's also one useful thing jobs give you: it allows you to get and set detailed resource limits on processes. e.g. not use more than x memory, don't run longer than y ms, etc.

Which is why I wanted to expose the job handle back to the user as well. Since while not provided here, the changes you would need to set these should be rather trivial now, this is what they are in general used for along with just monitoring processes.

always use a job. In which case, if a user hasn't been deconstructing ProcessHandle then it should work transparently.
...
If we go for 1) then, according to some very statistically unsound testing, the added overhead is about 0.0021 ms.

Seems like a no-brainer to me. 2 microseconds per process creation, and only on Windows (where process creation is already many milliseconds, IIRC).

ProcessHandle is already abstract in the basic System.Process API, you have to import System.Process.Internals to get it, so I can't imagine it would break much code.

Hmm so both of you seem to favor different options. How about a middle ground. I keep two separate functions for spawning, but only have one for waiting?

Yes, @simonmar and I do seem to be at odds. Simon: AFAICT, switching createProcess to have the new job-based behavior is a significantly breaking semantic change on Windows only, which is a huge recipe for disaster. Do you disagree with this assessment?

Yes, I see. What I'm concerned about is adding a new API that duplicates existing functionality but with subtly different behaviour on one platform. That's also a recipe for disaster.

What is using exec on Windows?

Mistuke · 2016-12-13T12:47:34Z

Also, I found a bug with this version when doing a GHC bootstrapping build. I'll fix that soon.

Mistuke · 2016-12-14T08:26:09Z

So, most blatant user at the moment are any msys utilities. Which we call from ghc. I've patched for instance timeout, but the amount of usages throughout I figured I should stop duplicating the code. Gcc also uses this when calling the compiler from the driver. I believe the MINGW guys patch these out to CreateProcess but not sure. Calling gcc for compilation seems to work or we'd have problems already. Calling it for preprocessing seems to subtly not work properly like in hsc2hs, again I can patch it in the driver, but I'm duplicating code everywhere. This also has a difference in the exit code. You'll get the exit code for the last process to exit. So if using exec you get the right exit code now. And when using CreateProcess since the last application would be the first caller. Is it really that confusing to have a 'createProcessWithJob' call?

…

On Wed, 14 Dec 2016, 08:13 Simon Marlow, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In System/Process.hs <#80>: > @@ -870,6 +869,22 @@ It will therefore behave more portably between operating systems than 'system'. The return codes and possible failures are the same as for 'system'. -} rawSystem :: String -> [String] -> IO ExitCode -rawSystem cmd args = do - (_,_,_,p) <- createProcess_ "rawSystem" (proc cmd args) { delegate_ctlc = True } +rawSystem cmd args = executeAndWait "rawSystem" (proc cmd args) { delegate_ctlc = True } + +-- --------------------------------------------------------------------------- +-- executeAndWait + +-- | Create a new process and wait for it's termination. Yes, I see. What I'm concerned about is adding a new API that duplicates existing functionality but with subtly different behaviour on one platform. That's also a recipe for disaster. What is using exec on Windows? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABH3Kc9RqPxvQG9bYf3wGSYSmOEaFxD7ks5rH6U5gaJpZM4LJ9UX> .

snoyberg · 2016-12-14T09:04:17Z

What about the alternative of having a setting in CreateProcess that switches to the job-based behavior, with the clear documentation that:

It only affects Windows, and has no impact on other OSes
It does not guarantee identical behavior to other OSes, but instead will have slightly different exit code behavior as we've discussed here

I think that will address the API proliferation issue (which I agree is a problem), and should hopefully be about as easy to work with from a user perspective.

Mistuke · 2016-12-14T09:13:31Z

That works for me as well. So then we're back to one function. And I assume we just leave the processes that do an execute and wait as they are now, such as `rawSystem`.

…

On Wed, 14 Dec 2016, 09:04 Michael Snoyman, ***@***.***> wrote: What about the alternative of having a setting in CreateProcess that switches to the job-based behavior, with the clear documentation that: - It only affects Windows, and has no impact on other OSes - It does not guarantee identical behavior to other OSes, but instead will have slightly different exit code behavior as we've discussed here I think that will address the API proliferation issue (which I agree is a problem), and should hopefully be about as easy to work with from a user perspective. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABH3KQ_fPXVqaKeDSGhogWfxBy2ohNtIks5rH7ESgaJpZM4LJ9UX> .

snoyberg · 2016-12-14T09:14:55Z

That's my preference. There's been a move over the past few releases to make CreateProcess an easier-to-use part of the API, and I'd recommend continuing in that directly such that it's fairly easy to replace rawSystem with some code that sets the appropriate option.

simonmar · 2016-12-14T10:25:10Z

Ok,sounds good to me too. Thanks!

snoyberg · 2016-12-14T10:26:21Z

BTW @Mistuke, I have not yet looked at the actual code on this at all, I figured we should hash out this design bit first. I'm assuming I should hold off on an actual code review until the PR is updated to reflect these decisions, do you agree?

Mistuke · 2016-12-14T10:32:47Z

@SNoyman yeah, that would be best since the haskell side is going to change anyway ☺️ I won't have time to update it till the weekend, but I'll get it done then.

…

On Wed, 14 Dec 2016, 10:26 Michael Snoyman, ***@***.***> wrote: BTW @Mistuke <https://github.com/Mistuke>, I have not yet looked at the actual code on this at all, I figured we should hash out this design bit first. I'm assuming I should hold off on an actual code review until the PR is updated to reflect these decisions, do you agree? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABH3KZqZ78a7yuEffnTztiT8fzVIN05vks5rH8ROgaJpZM4LJ9UX> .

Mistuke · 2017-01-10T21:34:49Z

Sorry for the delay, I'm almost done, just have to figure out a weird issue with stage-2 when using this version to build gcc.

Mistuke · 2017-01-16T03:37:01Z

Fixed the issue, now ready for review. Thanks!

Mistuke · 2017-01-17T08:06:44Z

Oops, forgot to add the note, I'll do that today.

Mistuke · 2017-01-27T12:20:32Z

Ping

snoyberg · 2017-01-28T16:34:22Z

I haven't had a chance to look into this yet, I'm hoping to this coming week.

…

On Fri, Jan 27, 2017 at 2:20 PM, Tamar Christina ***@***.***> wrote: Ping — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBB4yNjBzmxTFurbuFL7DigT-yDj8Mks5rWeEQgaJpZM4LJ9UX> .

Mistuke · 2017-01-28T16:43:11Z

Cool, no rush, just wasn't sure you knew it was updated :)

snoyberg · 2017-01-28T16:44:34Z

Am aware, keep meaning to review, keep getting dragged away by less interesting tasks :p

…

On Sat, Jan 28, 2017 at 6:43 PM, Tamar Christina ***@***.***> wrote: Cool, no rush, just wasn't sure you knew it was updated :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBB2A43Ee5ZEHJphB6BmTFUyZ0wniDks5rW3AggaJpZM4LJ9UX> .

snoyberg · 2017-01-29T16:23:39Z

System/Process.hs

@@ -604,7 +607,14 @@ waitForProcess ph@(ProcessHandle _ delegating_ctlc) = do
        when delegating_ctlc $
          endDelegateControlC e
        return e
-
+    OpenExtHandle _ job iocp -> do


Minor note: looks like the do is unnecessary

snoyberg · 2017-01-29T16:25:29Z

System/Process.hs

@@ -639,6 +650,10 @@ getProcessExitCode ph@(ProcessHandle _ delegating_ctlc) = do
    Just e | was_open && delegating_ctlc -> endDelegateControlC e
    _                                    -> return ()
  return m_e
+    where getHandle :: ProcessHandle__ -> PHANDLE
+          getHandle (OpenHandle        h) = h
+          getHandle (ClosedHandle      _) = error "getHandle: handle closed."


Not a hard requirement, but I'd prefer to avoid the partiality here. I was thinking something like:

case getHandle p_ of Left e -> return (p_, (Just e, False)) Right h -> do ... where getHandle (OpenHandle h) = Right h getHandle (ClosedHandle e) = Left e getHandle (OpenExtHandle h _ _) = Right h

snoyberg · 2017-01-29T16:26:27Z

System/Process/Common.hs

                                           --
                                           --   Default: @Nothing@
                                           --
                                           --   @since 1.4.0.0
+  use_process_jobs :: Bool                 -- ^ On Windows systems this flag indicates that we should wait for the entire process tree
+                                           --   to finish before unblocking. On POSIX system this flag is ignored.


s/system/systems

snoyberg · 2017-01-29T16:27:02Z

System/Process/Common.hs

+                                           --
+                                           --   Default: @False@
+                                           --
+                                           --   @since 1.x.x.x


I think we should call this 1.5.0.0

snoyberg · 2017-01-29T16:28:18Z

System/Process/Common.hs

 -}
-data ProcessHandle__ = OpenHandle PHANDLE | ClosedHandle ExitCode
+data ProcessHandle__ = OpenHandle PHANDLE
+                     | OpenExtHandle PHANDLE PHANDLE PHANDLE


Would it make sense to make these fields strict? It does not seem like laziness would ever be a virtue here. (I realize that will make it mismatched with the other constructors, still asking if it's the right thing to do.)

I'm don't think so in this case, since the only way they're used is with a foreign value. I don't think you can ever get a lazy value from an FFI call right?

My question is really: would we ever want the laziness here? I would generally make fields strict if there's no reason to do otherwise, since it excludes a potential source of bugs and confusion. Also, in this case, I think it can (slightly) help performance by allowing unpacking of a primitive type, but that's a minor concern. Either way, this isn't vital.

snoyberg · 2017-01-29T16:28:58Z

System/Process/Internals.hs

 #ifndef WINDOWS
    pPrPr_disableITimers, c_execvpe,
    ignoreSignal, defaultSignal,
+#else


For clarity: could you switch the conditional for #ifdef WINDOWS? It's slightly easier to read that way.

snoyberg · 2017-01-29T16:29:26Z

System/Process/Internals.hs

@@ -66,6 +70,18 @@ import System.Process.Posix
 -- * This function takes an extra @String@ argument to be used in creating
 --   error messages.
 --
+-- * 'use_process_jobs' can set in CreateProcess since 1.4.?.? in order to create


Probably "can be set", and again let's use 1.5.0.0.

snoyberg · 2017-01-29T16:30:58Z

System/Process/Windows.hsc

@@ -1,4 +1,5 @@
-{-# LANGUAGE CPP #-}


I'm going to review this file too, but my review will be much less authoritative. Basically: I'm assuming you know what's going on for Windows better than I do, which is a really safe assumption 😄.

Lol, fair enough :)

snoyberg · 2017-01-29T16:33:38Z

changelog.md

@@ -9,6 +9,8 @@

 * New exposed `withCreateProcess`
 * Derive `Show` and `Eq` for `CreateProcess`, `CmdSpec`, and `StdStream`
+* Add support for monitoring process tree for termination with the parameter `use_process_jobs`


This should go into "Unreleased changes"

snoyberg · 2017-01-29T16:33:57Z

process.cabal

@@ -1,5 +1,5 @@
 name:          process
-version:       1.4.3.0
+version:       1.4.3.1


As mentioned above, this would be 1.5.0.0

Mistuke · 2017-01-29T22:19:37Z

Seems the appveyor build is having trouble with sourceforge.

snoyberg · 2017-01-30T12:23:24Z

I've restarted the AppVeyor build, which now passed.

snoyberg · 2017-01-30T12:52:24Z

Thanks! What are your thoughts on roll-out of a new release for this? Do you want to get more testing of this with GHC before we release?

Mistuke · 2017-01-30T13:05:05Z

It should be OK, the default behavior should be unchanged. Though I would like to run a x86 build just to be sure. I'll run the full testsuite on both tonight and report back. Other than that I'm fairly confident. A similar code has been in use in timeout.hs for a while now. Thanks!

…

On Mon, 30 Jan 2017, 12:52 Michael Snoyman, ***@***.***> wrote: Thanks! What are your thoughts on roll-out of a new release for this? Do you want to get more testing of this with GHC before we release? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABH3KTGw8qbaIkJyZ6818RKZV4oqLQ0Vks5rXd0JgaJpZM4LJ9UX> .

snoyberg · 2017-01-30T13:06:13Z

Sure, thank you! On Mon, Jan 30, 2017 at 3:05 PM, Tamar Christina <notifications@github.com> wrote:

…

It should be OK, the default behavior should be unchanged. Though I would like to run a x86 build just to be sure. I'll run the full testsuite on both tonight and report back. Other than that I'm fairly confident. A similar code has been in use in timeout.hs for a while now. Thanks! On Mon, 30 Jan 2017, 12:52 Michael Snoyman, ***@***.***> wrote: > Thanks! What are your thoughts on roll-out of a new release for this? Do > you want to get more testing of this with GHC before we release? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#80 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ ABH3KTGw8qbaIkJyZ6818RKZV4oqLQ0Vks5rXd0JgaJpZM4LJ9UX> > . > — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBB0WYQN0qP3XHHR0GGLqOXvqPsVGNks5rXeACgaJpZM4LJ9UX> .

Mistuke · 2017-02-02T20:05:43Z

Ok, I think we're good. Whenever you want to make the release is fine with me! After you do I can take care of updating the rest of the dependencies to get it in the GHC tree if it's easier for you!

snoyberg · 2017-02-06T08:58:26Z

I've made the release to Hackage. If you could take care of the updates on the GHC side that would be great, much appreciated!

Mistuke · 2017-02-07T10:18:27Z

Alright, I'll do that then. Thanks!

…

On Mon, 6 Feb 2017, 08:58 Michael Snoyman, ***@***.***> wrote: I've made the release to Hackage. If you could take care of the updates on the GHC side that would be great, much appreciated! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABH3KW3MGwfxQCxKcQI7CzVCxuY_at_oks5rZuCzgaJpZM4LJ9UX> .

simonmar reviewed Dec 12, 2016

View reviewed changes

Mistuke force-pushed the gh-77-create-process-hook branch from f5027af to 03d5d37 Compare January 7, 2017 19:38

Mistuke force-pushed the gh-77-create-process-hook branch 2 times, most recently from f5027af to c4d9a7a Compare January 16, 2017 02:48

snoyberg requested changes Jan 29, 2017

View reviewed changes

Mistuke added 10 commits January 29, 2017 20:53

GH77: Add scaffolding.

684ce18

GH77: Fixed compilation

8080309

GH77: Add terminate job

d71248a

GH77: Update readme and export list.

57e0c7f

GH77: Replaced system and rawSystem

f6de652

GH77: Updated readme

3f440e2

GH77: Fix tests

86b273c

GH77: Add failing test for Windows.

e7827bb

GH77: Working

3bf217f

GH77: Finish implementation.

282aa2e

Mistuke added 14 commits January 29, 2017 20:53

GH77: rewrote implementation.

3a5935c

GH77: fix compile errors.

ae57e8c

GH77: Update readme.

7ef688e

GH77: restored compatibility.

c3c067b

GH77: rebased.

5a12fa4

GH77: fix Posix.

e41616e

GH77: remove typo.

5a0d7bc

GH77: fix pattern matching posix.

ad967f8

GH77: replace <$> with fmap

2d6933b

GH77: Add appropriate ifdefs.

4a423ad

GH77: fixed bug.

94a2140

GH77: Added note.'

523b3dd

Updated based on review

0f7b948

rebased and set back WINDOWS_CCONV

f8b53d8

Mistuke force-pushed the gh-77-create-process-hook branch from 129dd48 to f8b53d8 Compare January 29, 2017 21:43

fix build.

9bcbaeb

snoyberg approved these changes Jan 30, 2017

View reviewed changes

snoyberg merged commit d3d637d into haskell:master Jan 30, 2017

This was referenced Apr 26, 2017

process 1.6.0 #95

Closed

process 1.6 + stack commercialhaskell/stack#3145

Closed

bgamari mentioned this pull request Feb 10, 2020

Processes started on Windows with use_process_jobs=True may be terminated #167

Closed

nojb mentioned this pull request Jan 19, 2023

Semantics of Unix.exec* function on Windows ocaml/ocaml#11754

Closed

Add new API that can correctly wait for termination of processes forked with exec on Windows. #80

Add new API that can correctly wait for termination of processes forked with exec on Windows. #80

Uh oh!

Conversation

Mistuke commented Dec 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mistuke commented Dec 13, 2016

Uh oh!

Mistuke commented Dec 14, 2016 via email

Uh oh!

snoyberg commented Dec 14, 2016

Uh oh!

Mistuke commented Dec 14, 2016 via email

Uh oh!

snoyberg commented Dec 14, 2016

Uh oh!

simonmar commented Dec 14, 2016

Uh oh!

snoyberg commented Dec 14, 2016

Uh oh!

Mistuke commented Dec 14, 2016 via email

Uh oh!

Mistuke commented Jan 10, 2017

Uh oh!

Mistuke commented Jan 16, 2017

Uh oh!

Mistuke commented Jan 17, 2017

Uh oh!

Mistuke commented Jan 27, 2017

Uh oh!

snoyberg commented Jan 28, 2017 via email

Uh oh!

Mistuke commented Jan 28, 2017

Uh oh!

snoyberg commented Jan 28, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!