On Mon, 11 Mar 2013 01:24:17 -0400, Marco Leise <marco.le...@gmx.de> wrote:
Am Sun, 10 Mar 2013 21:10:24 -0400
schrieb "Steven Schveighoffer" <schvei...@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
