On Sunday, 10 March 2013 at 03:06:03 UTC, Steven Schveighoffer wrote:
> Try shortening your executable path/environment?

Didn't help.

On Sat, 09 Mar 2013 22:07:26 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:

> On Sunday, 10 March 2013 at 02:54:44 UTC, Steven Schveighoffer wrote:
>> Vladimir, can you try compiling 32-bit windows and see if it works for you, just to confirm?
>
> I'm seeing the same exception with both 32 and 64 bit, Steven.
>
> I guess it must be something specific to my system, like the HANDLE_FLAG_INHERIT stuff.

IIRC, you were able to get something working there with std.process2.  Is that example still working?  I'm trying to see what could be different that's causing this.

I can try a test on a win7 box monday.  Likely the issue is xp vs. win7.

-Steve

On Sunday, 10 March 2013 at 03:48:36 UTC, Steven Schveighoffer wrote:
> On Sat, 09 Mar 2013 22:07:26 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
>
>> On Sunday, 10 March 2013 at 02:54:44 UTC, Steven Schveighoffer wrote:
>>> Vladimir, can you try compiling 32-bit windows and see if it works for you, just to confirm?
>>
>> I'm seeing the same exception with both 32 and 64 bit, Steven.
>>
>> I guess it must be something specific to my system, like the HANDLE_FLAG_INHERIT stuff.
>
> IIRC, you were able to get something working there with std.process2.  Is that example still working?  I'm trying to see what could be different that's causing this.

Yes, Steven. The ls test program still works:
http://dump.thecybershadow.net/935b0c4a47ce367313efcc1806f75076/lstest.d

Am Sat, 09 Mar 2013 13:35:26 -0500
schrieb "Steven Schveighoffer" <schveiguy@yahoo.com>:

> On Sat, 09 Mar 2013 11:05:14 -0500, Lars T. Kyllingstad <public@kyllingen.net> wrote:
> 
> > 1. Make a "special" spawnProcess() function for pipe redirection.
> > 2. Use the "process object" approach, like Tango and Qt.
> > 3. After fork(), in the child process, loop over the full range of
> > possible file descriptors and close the ones we don't want open.
> >
> > The last one would let us keep the current API (and would have the added benefit of cleaning up unused FDs) but I have no idea how it would impact performance.
> 
> I think 3 is the correct answer, it is consistent with Windows, and the most logical behavior.  For instance, if other threads are open and doing other things that aren't related (like network sockets), those too will be inherited!  We should close all file descriptors.

So that means on Posix any programming scheme involving passing open descriptors on to child processes is not going to work with std.process? Not that I know of any, but if that's what will happen it is good to know the cost. ;)

-- 
Marco

On Sunday, 10 March 2013 at 16:04:51 UTC, Marco Leise wrote:
> Am Sat, 09 Mar 2013 13:35:26 -0500
> schrieb "Steven Schveighoffer" <schveiguy@yahoo.com>:
>
>> On Sat, 09 Mar 2013 11:05:14 -0500, Lars T. Kyllingstad  <public@kyllingen.net> wrote:
>> 
>> > 1. Make a "special" spawnProcess() function for pipe redirection.
>> > 2. Use the "process object" approach, like Tango and Qt.
>> > 3. After fork(), in the child process, loop over the full range of  possible file descriptors and close the ones we don't want open.
>> >
>> > The last one would let us keep the current API (and would have the added  benefit of cleaning up unused FDs) but I have no idea how it would  impact performance.
>> 
>> I think 3 is the correct answer, it is consistent with Windows, and the  most logical behavior.  For instance, if other threads are open and doing  other things that aren't related (like network sockets), those too will be  inherited!  We should close all file descriptors.
>
> So that means on Posix any programming scheme involving
> passing open descriptors on to child processes is not going to
> work with std.process? Not that I know of any, but if that's
> what will happen it is good to know the cost. ;)

I think the idea is that if you rely on such platform-specific behavior, you probably shouldn't be using std.process in the first place - as its goal is to provide high-level cross-platform abstractions for the most common operations, rather than try to cover all features exposed by all operating system APIs.

> Of course, we have a problem if some other platform allows ulong.max open files...

You can increase (on Linux) maximal number of open files by adding something like

your_username                hard    nofile          65536

to /etc/security/limits.conf and using ulimit. You can increase it up to /proc/sys/fs/file-max (which is 394062 by default on my machine but can also be increased) that way.

I don't know what the maximal valid value of /proc/sys/fs/file-max is, but it can
not be more than int.max, since file descriptors are ints.

Am Sun, 10 Mar 2013 17:07:26 +0100
schrieb "Vladimir Panteleev" <vladimir@thecybershadow.net>:

> I think the idea is that if you rely on such platform-specific behavior, you probably shouldn't be using std.process in the first place - as its goal is to provide high-level cross-platform abstractions for the most common operations, rather than try to cover all features exposed by all operating system APIs.

Not necessarily. Why do you have to deal with this stuff in std.process in the first place? Because during development of the sockets and file APIs in Phobos noone thought about this issue. It should be a Phobos convention that descriptors are closed on exec by default and changed in the few places where sockets and files are created.

Someone who uses Posix APIs directly can thus rely on their behavior and std.process can stay ignorant to this platform difference and avoid ugly hacks.

-- 
Marco

Phobos isn't the first and wont be the last to tackle this issue:

http://www.google.de/search?q=%22set+FD_CLOEXEC%22+OR+%22FD_CLOEXEC+not+set%22+bug

Affected projects range from MySQL to Mozilla.

-- 
Marco

On Sun, 10 Mar 2013 13:03:20 -0400, Marco Leise <Marco.Leise@gmx.de> wrote:

> Am Sun, 10 Mar 2013 17:07:26 +0100
> schrieb "Vladimir Panteleev" <vladimir@thecybershadow.net>:
>
>> I think the idea is that if you rely on such platform-specific
>> behavior, you probably shouldn't be using std.process in the
>> first place - as its goal is to provide high-level cross-platform
>> abstractions for the most common operations, rather than try to
>> cover all features exposed by all operating system APIs.

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.

> Not necessarily. Why do you have to deal with this stuff in
> std.process in the first place? Because during development of
> the sockets and file APIs in Phobos noone thought about this
> issue. It should be a Phobos convention that descriptors are
> closed on exec by default and changed in the few places where
> sockets and files are created.

It only becomes a problem for long-living subprograms.  For example, if you run an "ls" command as a subprocess, who cares if it keeps open sockets for a fraction of a second?  The other real issue is if a child process unknowingly keeps its stdin/stderr/stdout open by having the other end open (Vladimir's situation).

But it certainly is a problem that we can and should solve.

However, I don't agree with your solution.  We should not be infiltrating std.process hacks into all creation of streams.  Not only is that difficult to maintain, but it decouples the purpose of code from the actual implementation by quite a bit.  A process may never even spawn a child process, 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.

I see no issue with std.process handling the historic flaws in process creation, that is where it belongs IMO.  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.

> Someone who uses Posix APIs directly can thus rely on their
> behavior and std.process can stay ignorant to this platform
> difference and avoid ugly hacks.

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

-Steve

Am Sun, 10 Mar 2013 21:10:24 -0400
schrieb "Steven Schveighoffer" <schveiguy@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