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

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

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

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

> On Sunday, 10 March 2013 at 03:48:36 UTC, Steven Schveighoffer wrote:

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

OK, I was able to reproduce on a windows 7 box, and I found the problem.  Really dumb.  It was the difference in the spawnProcess calls between the two programs that gave me the hint.

So the environment pointer to CreateProcess is a list of null-separated "var=value" strings.  The last string is followed by an additional null, so the function can identify the end of the list.

HOWEVER, if you have NO variables, there is no double null, because there isn't the null from the last variable.  However, on XP, a single null character is fine, whereas on windows 7, it REQUIRES an extra null character, even if you didn't have any variables.  Adding the extra null character in toEnvz fixed the problem.

Now, that actually brings up another bug I think.  pipeProcess is sending a null to spawnProcess for the environment.  However, because AA's are structs, and null is the same as an empty AA, it gets translated into "kill the entire environment" for the child process.  I think this is wrong.  But at the same time, what if you DID want to kill the entire environment?  Should we even support that?  Either way I don't think pipeProcess should kill the environment, so that needs to be changed.

Passing null to CreateProcessW copies the parent's environment, I think that should be the ultimate default when the environment is not specified, even for pipeProcess.  But what should we do if the spawnProcess overload that takes an environment receives a null environment?  My instinct is to detect an empty AA, and interpret that as "copy parent environment," even Lars seems to have interpreted it that way (maybe it works that way on Linux as it's written now?) in how he wrote pipeProcess.

I think we can forgo the pull request, the solution is simply to add another '\0', that will handle the case where the environment is empty and be a noop for when the environment has stuff in it.  But maybe we want to make toEnvz return null if it gets an empty AA to avoid killing the environment?  I have no idea what the right answer is.

-Steve

On Monday, 11 March 2013 at 23:21:18 UTC, Steven Schveighoffer wrote:
>
> OK, I was able to reproduce on a windows 7 box, and I found the problem.  Really dumb.  It was the difference in the spawnProcess calls between the two programs that gave me the hint.

Awesome! :)


> So the environment pointer to CreateProcess is a list of null-separated "var=value" strings.  The last string is followed by an additional null, so the function can identify the end of the list.
>
> HOWEVER, if you have NO variables, there is no double null, because there isn't the null from the last variable.  However, on XP, a single null character is fine, whereas on windows 7, it REQUIRES an extra null character, even if you didn't have any variables.  Adding the extra null character in toEnvz fixed the problem.

I never would have thought of trying that, especially considering that the CreateProcess() documentation is rather vague on this point.  It says:

"A Unicode environment block is terminated by four zero bytes: two for the last string, two more to terminate the block."

(This confused me slightly, until I remembered that one zero wchar is two zero bytes.)  It does not explicitly mention the case where there are *no* environment variables, but from the above statement, I had inferred that one zero character would suffice, considering there is no "last string" to terminate.  But it seems I was wrong. :)


> Now, that actually brings up another bug I think.  pipeProcess is sending a null to spawnProcess for the environment.  However, because AA's are structs, and null is the same as an empty AA, it gets translated into "kill the entire environment" for the child process.  I think this is wrong.  But at the same time, what if you DID want to kill the entire environment?  Should we even support that?  Either way I don't think pipeProcess should kill the environment, so that needs to be changed.
>
> Passing null to CreateProcessW copies the parent's environment, I think that should be the ultimate default when the environment is not specified, even for pipeProcess.  But what should we do if the spawnProcess overload that takes an environment receives a null environment?  My instinct is to detect an empty AA, and interpret that as "copy parent environment," even Lars seems to have interpreted it that way (maybe it works that way on Linux as it's written now?) in how he wrote pipeProcess.
>
> I think we can forgo the pull request, the solution is simply to add another '\0', that will handle the case where the environment is empty and be a noop for when the environment has stuff in it.  But maybe we want to make toEnvz return null if it gets an empty AA to avoid killing the environment?  I have no idea what the right answer is.

Nice catch!  Fortunately, the only bug here is that I've specified a null parameter in the spawn call in pipeProcessImpl().  If you look at the various spawnProcess() overloads, you'll see that they are designed as follows:

If you omit the AA parameter altogether, the child inherits the parent's environment.  A null pointer is passed as 'envz' to spawnProcessImpl(), which in turn passes this straight to CreateProcess() on Windows and replaces it by 'environ' on POSIX.

If you do specify the AA parameter, it is passed through toEnvz() on its way to spawnProcessImpl().  If the AA is empty/null, toEnvz() will create an empty (but non-null) environment block, and the child's environment will be empty.

I think the bug stems from the fact that, at some intermediate stage of the development, pipeProcessImpl() used to call spawnProcessImpl() directly.  I later changed this to spawnProcess(), but forgot to remove the null parameter.  I'll simply remove it, and everything will work as intended.

Initially, I wrote spawnProcess() the way you suggest, i.e. that a passing a null AA is equivalent to omitting it.  But then we are left with no simple way to explicitly clear the child's environment.  We could make a distinction between a "null" and an "empty" AA, but making a non-null empty AA is a hassle.  I think it is better the way it is now.

I don't think we need to support clearing the child's environment in pipeProcess().  I suspect it is a rare need, and the user will just have to deal with spawnProcess() directly in that case.  (It's like how there are all kinds of different exec*() functions in C, but only one, simple, popen() function.)

Lars

On Wednesday, 6 March 2013 at 07:27:19 UTC, Lars T. Kyllingstad wrote:
> On Tuesday, 5 March 2013 at 21:04:15 UTC, Vladimir Panteleev wrote:
>> 5. How about that Environment.opIn_r?
>
> Forgot about it. :)  I'll add it.

So I sat down to write this function, but then I reconsidered.  The thing is, checking whether the variable exists is exactly the same operation as retrieving it.  In other words, this:

   if (key in environment)
   {
       auto val = environment[key];
       ...
   }

is equivalent to:

   if (environment.get(key) !is null)
   {
       auto val = environment.get(key);
       ...
   }

That just seems... wrong, somehow.  Instead, I think we should encourage code like this:

   auto val = environment.get(key);
   if (val !is null)
   {
       ...
   }

or, even better, IMO:

   if (auto val = environment.get(key))
   {
       ...
   }

But feel free to convince me otherwise, you've had great success with that so far. :)

Lars

On Tuesday, 12 March 2013 at 07:41:03 UTC, Lars T. Kyllingstad wrote:
> On Wednesday, 6 March 2013 at 07:27:19 UTC, Lars T. Kyllingstad wrote:
>> On Tuesday, 5 March 2013 at 21:04:15 UTC, Vladimir Panteleev wrote:
>>> 5. How about that Environment.opIn_r?
>>
>> Forgot about it. :)  I'll add it.
>
> So I sat down to write this function, but then I reconsidered.  The thing is, checking whether the variable exists is exactly the same operation as retrieving it.  In other words, this:
>
>    if (key in environment)
>    {
>        auto val = environment[key];
>        ...
>    }
>
> is equivalent to:
>
>    if (environment.get(key) !is null)
>    {
>        auto val = environment.get(key);
>        ...
>    }

Yes, it's just syntax sugar, and an operation supported by AAs (which environment imitates). It's useful if you don't want to retrieve the value of a variable right after checking if it exists - you just want to see if it's there or not.

On Tuesday, 12 March 2013 at 08:28:02 UTC, Vladimir Panteleev wrote:
> On Tuesday, 12 March 2013 at 07:41:03 UTC, Lars T. Kyllingstad wrote:
>> On Wednesday, 6 March 2013 at 07:27:19 UTC, Lars T. Kyllingstad wrote:
>>> On Tuesday, 5 March 2013 at 21:04:15 UTC, Vladimir Panteleev wrote:
>>>> 5. How about that Environment.opIn_r?
>>>
>>> Forgot about it. :)  I'll add it.
>>
>> So I sat down to write this function, but then I reconsidered.
>>  The thing is, checking whether the variable exists is exactly the same operation as retrieving it.  In other words, this:
>>
>>   if (key in environment)
>>   {
>>       auto val = environment[key];
>>       ...
>>   }
>>
>> is equivalent to:
>>
>>   if (environment.get(key) !is null)
>>   {
>>       auto val = environment.get(key);
>>       ...
>>   }
>
> Yes, it's just syntax sugar, and an operation supported by AAs (which environment imitates). It's useful if you don't want to retrieve the value of a variable right after checking if it exists - you just want to see if it's there or not.

For AAs, 'in' returns a pointer to the element, which is null if the element does not exist.  I can't think of a good way to implement this.  Since we have to convert the raw environment variable to a D string anyways, we'd have to do something like:

  string* opIn_r(string var)
  {
      auto val = get(var);
      if (val is null) return null;
      else return [val].ptr;
  }

but that seems rather pointless to me.

Lars

On Tuesday, 12 March 2013 at 08:58:13 UTC, Lars T. Kyllingstad wrote:
> For AAs, 'in' returns a pointer to the element, which is null if the element does not exist.  I can't think of a good way to implement this.  Since we have to convert the raw environment variable to a D string anyways, we'd have to do something like:
>
>   string* opIn_r(string var)
>   {
>       auto val = get(var);
>       if (val is null) return null;
>       else return [val].ptr;
>   }
>
> but that seems rather pointless to me.

Yes. That use of the "is" operator is mainly to allow updating the value without looking up the key twice. This behavior could be implemented using a proxy object, but this is not what I was talking about. I meant the specific case of "if (key in environment)".

On Sunday, 10 March 2013 at 02:01:33 UTC, Steven Schveighoffer wrote:
>> How about this: Set FD_CLOEXEC on all pipes just after creation, but clear the flag for the relevant pipes before exec?
>
> This doesn't help if other threads are randomly opening file descriptors.  That is a problem I don't think we considered.

OK, but it will still solve the specific problem with the other end being open.

On Tue, 12 Mar 2013 05:58:31 -0400, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:

> On Sunday, 10 March 2013 at 02:01:33 UTC, Steven Schveighoffer wrote:
>>> How about this: Set FD_CLOEXEC on all pipes just after creation, but clear the flag for the relevant pipes before exec?
>>
>> This doesn't help if other threads are randomly opening file descriptors.  That is a problem I don't think we considered.
>
> OK, but it will still solve the specific problem with the other end being open.

Yes it does.  I'd rather solve both, though.

-Steve

On Tue, 12 Mar 2013 05:13:59 -0400, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:

> On Tuesday, 12 March 2013 at 08:58:13 UTC, Lars T. Kyllingstad wrote:
>> For AAs, 'in' returns a pointer to the element, which is null if the element does not exist.  I can't think of a good way to implement this.  Since we have to convert the raw environment variable to a D string anyways, we'd have to do something like:
>>
>>   string* opIn_r(string var)
>>   {
>>       auto val = get(var);
>>       if (val is null) return null;
>>       else return [val].ptr;
>>   }
>>
>> but that seems rather pointless to me.
>
> Yes. That use of the "is" operator is mainly to allow updating the value

you meant "in", not "is", right?

> without looking up the key twice. This behavior could be implemented using a proxy object, but this is not what I was talking about. I meant the specific case of "if (key in environment)".

I think Valdimir wants to have opIn_r return bool?

-Steve