The new std.process is ready for review

[Lars T. Kyllingstad]

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