The new std.process is ready for review

[Marco Leise]

Am Tue, 12 Mar 2013 14:37:31 -0400
schrieb "Steven Schveighoffer" <schveiguy at yahoo.com>:

> On Tue, 12 Mar 2013 13:48:04 -0400, Marco Leise <Marco.Leise at gmx.de> wrote:
> 
> > What makes you think Phobos should carry on all the OS
> > specific quirks? I think a cross-platform library should
> > offer the same behavior on all systems. In this case it can
> > also be seen as covering up for an arguably bad Posix API
> > design.
> 
> D's design should not preclude what is possible.  Someone may have a good  
> reason to use that bad API.

I don't want to stop anyone from doing that. I'm just trying
not to write book pages here. :)
It's good that we have "unwrappers" in "File":
getFP (for the C stream) and fileno (for the file descriptor)
as well as the open() syscall.

> What I want to avoid is having to require the user to pay attention to  
> handle inheritance if he doesn't care, but also I don't want Phobos to  
> puke when you decide to (or have to) circumvent phobos when creating file  
> descriptors.

Exactly! And the thousands of bug reports (literally!) in other
software seem to indicate that an uninstructed human being
assumes that files don't remain open in sub-processes. We
aren't even talking about fork here, but really new processes
created with the exec family of calls.

Also common logic suggests that this behavior would not make
sense, since what can a sub-process do with open files when I
didn't give it the descriptor numbers?

> The most robust method is to close all the descriptors we don't want to  
> pass, regardless of how they are opened/configured.

Ok I give in. After reading the Windows API documentation and
also the SysV daemon writing tips, I had to come to the same
conclusion. (In addition to not creating leaking file
descriptors in Phobos in the first place that is.)

> Linux file descriptors are integers.  Not quite that hard to pass another  
> file descriptor via number 3 or 4 for example.

No, but at that point you no longer ignorant about leaking
descriptors, check what the Phobos file abstraction layer does
and can unset FD_CLOEXEC:

int fileno = file.fileno();
int fdflags = fcntl(fileno, F_GETFD);
fcntl(fileno, F_SETFD, fdflags & ~FD_CLOEXEC);

or use syscalls directly.

> This is not a good position to have as the should-be-agnostic standard  
> library.  Phobos should make the most useful/common/safe idioms the  
> default, but make the non-safe ones possible.  The idea of marking all  
> descriptors as close on exec puts unnecessary burden on those who want to  
> use straight fork/exec and want to pass Phobos-created descriptors.   

We agree on the "possible" and also on the most
"useful/common/safe" aspect. But the conclusions that we draw
are different. To you (as I read it) Phobos offers
functionality to create Posix file descriptors, to me it only
creates file abstractions that encapsulate and level OS quirks.

It looks to me like the issue was just not considered when
writing Phobos and we can do it like Ruby and add that
property to files that allows them to stay open in
sub-processes. But this should not be a Posix only option.
SetFileSecurity should do the job on Windows just as well.

> Having spawnProcess depend on those flag settings puts unnecessary burden  
> on people who use non-phobos calls to open file descriptors and want to  
> use spawnProcess.  I'd rather avoid that.
> 
> Both methods are very similar, I just feel calling close on all  
> descriptors between fork and exec is more effective, and puts the burden  
> on spawnProcess instead of the user or other parts of Phobos.
> 
> -Steve

Yeah, alright. Close them all by default. But keep in mind
what I said in the last post: as likely as it is that people
don't use Phobos to open files, a library might use fork/exec
directly and leak our Phobos files. The problem should not be
tackled in std.process alone.
I'll probably try myself on a pull request in the coming days
that closes Phobos file descriptors on exec and see about a
simple system-agnostic(!) property to enable it. After all,
why should Windows folks not be able to pass handles to
sub-processes?

-- 
Marco