The new std.process is ready for review

[Steven Schveighoffer]

On Mon, 11 Mar 2013 01:24:17 -0400, Marco Leise <Marco.Leise at gmx.de> wrote:

> Am Sun, 10 Mar 2013 21:10:24 -0400
> schrieb "Steven Schveighoffer" <schveiguy at yahoo.com>:
>
>> I think we can find a mix.  Since fork gives you a convenient location  
>> to
>> close all open file descriptors, we should do so by default.  But if you
>> want standard posix behavior and you want to rely on F_CLOEXEC, you  
>> should
>> be able to do that with a flag to spawnProcess.  We already have a  
>> Config
>> parameter that is already used to control stream closing behavior.  We
>> should extend that.
>>
>> [...]
>>
>> However, I don't agree with your solution.  We should not be  
>> infiltrating
>> std.process hacks into all creation of streams.
>
> I don't see it as introducing std.process hacks everywhere but
> fixing Phobos file handle semantics in a clean way. Just try to
> look at it from that perspective.

Any time you are fixing an OS flaw in user code, it's a hack :)

>> Not only is that
>> difficult to maintain, but it decouples the purpose of code from the
>> actual implementation by quite a bit.
>
> Windows has a flag in both locations as well: When you create
> the (file) handle and when you create a sub process. And a
> common file handle/descriptor property in all OSs is
> whether it is inheritable. Whereas no such common ground
> exists for spawning processes.

What I mean is, this is an issue with spawning child processes.  It  
belongs in a function that spawns child processes.

>> A process may never even spawn a child process,
>
> No harm done in that case.
>
>> or it may call functions that create pipes/threads that
>> DON'T set F_CLOEXEC.  Maybe the 3rd party library didn't get that memo.
>
> Yes and yes. Its not a BUG to do that deliberately and
> cleanly supported by Windows as you can open stuff with
> bInheritHandle set in SECURITY_ATTRIBUTES, duplicating
> that behavior.
>
> Consider these changes of perspective:
>
> * A hardcore Posix fanatic could just as well argue that
>   Windows code forgetting to set bInheritHandle for all opened
>   files is at fault. Since inheritance should be the default.

I don't think that would be a good solution either (setting all handles to  
inherit on Windows).  IMO, D should be as compatible as possible with the  
target OS.  This means:

a) libraries written in other languages which D deals with are not run in  
an environment that is unexpected (e.g. all handles are unexpectedly  
marked CLOEXEC).
b) D's code can run without expecting to have special conditions.

> * You mentioned libs opening file descriptors without Phobos.
>   By the same thinking someone could spawn child processes
>   without Phobos - still inheriting the open descriptors.

With your solution, they would have to open all their handles WITHOUT  
using phobos.  Likely a huge pain.

>> I see no issue with std.process handling the historic flaws in process
>> creation, that is where it belongs IMO.
>
> But it also - as a file handle/descriptor property - belongs
> to creating those.

The property specifically deals with creating processes.  It's not a  
handle property, it's just stored with the handle because it requires one  
flag per handle, and the space is there.

Technically, even Windows has this wrong.  Because they don't give a place  
for you to do the "right thing" per execution of CreateProcess (like fork  
does), it's possible to have a race where you inadvertently inherit  
handles you don't mean to.

example:

thread 1 wants to spawn a process, creates pipes for the standard handles,  
sets the appropriate ends to inheritable
thread 2 wants to spawn a process, creates pipes for the standard handles,  
sets the appropriate ends to inheritable
thread 1 spawns his process, which assigns it's pipes to the standard  
handles, but also it has inherited thread 2's pipes!

This is like Vladimir's problem, but involves a race, so it will only  
happen once in a blue moon!  Unfortunately, we have no recourse for this...

>
>> What's nice about it is, with the "close all open descriptors" method,
>> it handles all these cases quite well.  We should also give the user
>> a "roll your own" option where it doesn't close these descriptors for
>> you, you must set F_CLOEXEC manually.
>
> Assuming this is the compromise - with the Windows code path
> using bInheritHandles for CreateProcess - this still leaves us
> with Phobos creating inheritable handles on Posix and
> non-inheritable ones on Windows. Where it should be opt-in on
> both.

I don't agree.  It should do the default that the OS provides.  Variations  
are available by using OS-specific functions.

> I've placed some example implementations from other languages
> in the other thread...

It's good that there is precedent for your idea.  But I don't agree with  
the design regardless of whether other implementations have done it.

-Steve