The new std.process is ready for review

[Marco Leise]

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.

My thinking is that it works without any hack at all, by attacking
the issue at the _source_ where we can still simply ask Posix to do
the right thing(tm). We can level the operating system differences
right at the point where we open files, instead of messing with the
semantics of spawning sub processes later by closing all but 0, 1
and 2 by default, which is an actual 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.

> 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.

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

This is outside the scope of std.process: We have a security
relevant property of open files that is supported on all OS,
but we don't set it to the same value, which should be: safe by
default. (That is why I opened a new thread about it in case
you were wondering.)

> 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.

> 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.

> Both ways are ugly hacks. I'd rather have the hacks be in one place,
> and not require 3rd party libs to comply.
> 
> -Steve

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

-- 
Marco