Replace subprocess internals with _posixsubprocess extension module

[gregory.p.smith, 2010-03-01 04:45:44]

http://codereview.appspot.com/223077/diff/25/1009
File Lib/subprocess.py (right):

http://codereview.appspot.com/223077/diff/25/1009#newcode1195
Lib/subprocess.py:1195: data = os.read(errpipe_read, 1048576)
svn up and merge, this is called using _eintr_retry_call() now.  same with the
waitpid below.

http://codereview.appspot.com/223077/diff/25/1011
File Modules/_posixsubprocess.c (right):

http://codereview.appspot.com/223077/diff/25/1011#newcode249
Modules/_posixsubprocess.c:249: pid = fork();
If a preexec_fn exists:
  * I need to acquire the import lock before calling fork.
  * in the child process call PyOS_AfterFork.
  * in the parent process release the import lock after the fork.

(see posixmodule.c)
And call PyOS_AfterFork() in the child process afterwards

http://codereview.appspot.com/223077/diff/25/1011#newcode330
Modules/_posixsubprocess.c:330: {"fork_exec",	subprocess_fork_exec,    
METH_VARARGS, subprocess_fork_exec_doc},
line length

[Jeffrey Yasskin, 2010-03-02 02:17:11]

Ok! Aside from all that, this looks good. Thanks!

http://codereview.appspot.com/223077/diff/25/1007
File Doc/library/subprocess.rst (right):

http://codereview.appspot.com/223077/diff/25/1007#newcode110
Doc/library/subprocess.rst:110: If *preexec_fn* is set to a callable object,
this object will be called in the
Could you add the "not thread-safe" comment here too?

http://codereview.appspot.com/223077/diff/25/1009
File Lib/subprocess.py (right):

http://codereview.appspot.com/223077/diff/25/1009#newcode75
Lib/subprocess.py:75: If preexec_fn is set to a callable object, this object
will be called
Please mention here that passing preexec_fn makes POpen not thread-safe.

http://codereview.appspot.com/223077/diff/25/1009#newcode649
Lib/subprocess.py:649: # are None when not using PIPEs. The child objects are
None
Please update this comment to reflect that they're now -1.

http://codereview.appspot.com/223077/diff/25/1009#newcode1079
Lib/subprocess.py:1079: env_list = []
This line is useless, right?

http://codereview.appspot.com/223077/diff/25/1009#newcode1102
Lib/subprocess.py:1102: # Python code running in the child process after fork,
This isn't a complete sentence.

http://codereview.appspot.com/223077/diff/25/1008
File Lib/test/test_subprocess.py (right):

http://codereview.appspot.com/223077/diff/25/1008#newcode563
Lib/test/test_subprocess.py:563: # Its the perror no such file or directory
message.
Can you word this more clearly?

http://codereview.appspot.com/223077/diff/25/1008#newcode564
Lib/test/test_subprocess.py:564: # Can we even reliably test for this as it is a
Assert the errno value? Trigger another copy of the same error message and check
that they're equal?

http://codereview.appspot.com/223077/diff/25/1011
File Modules/_posixsubprocess.c (right):

http://codereview.appspot.com/223077/diff/25/1011#newcode4
Modules/_posixsubprocess.c:4: *   Licensed under the Apache License, Version 2.0
(the "License");
Do we really need this? Most of the other modules don't include it.

http://codereview.appspot.com/223077/diff/25/1011#newcode31
Modules/_posixsubprocess.c:31: static void child_exec(char *const exec_array[],
Please comment that all of the code in this function must only use
async-signal-safe functions, listed at `man 7 signal` or
http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html. 
This restriction is documented at
http://www.opengroup.org/onlinepubs/009695399/functions/fork.html.

http://codereview.appspot.com/223077/diff/25/1011#newcode100
Modules/_posixsubprocess.c:100: _Py_RestoreSignals();
Eep. Is this signal-safe? Ah, yes, after tracking down the definition.

http://codereview.appspot.com/223077/diff/25/1011#newcode102
Modules/_posixsubprocess.c:102: if (preexec_fn != Py_None &&
preexec_fn_args_tuple) {
Please comment that this block is where the user asks us to deadlock her
program.

http://codereview.appspot.com/223077/diff/25/1011#newcode106
Modules/_posixsubprocess.c:106: /* memory allocation and thus potential for
deadlock.    */
Consistentize your comment style?

http://codereview.appspot.com/223077/diff/25/1011#newcode138
Modules/_posixsubprocess.c:138: err_msg = strerror(saved_errno);
strerror() is not async-signal-safe (wow). You might use the deprecated
sys_errlist directly ... or map the errno back to a string in the parent
process?

http://codereview.appspot.com/223077/diff/25/1011#newcode139
Modules/_posixsubprocess.c:139: snprintf(hex_errno, sizeof(hex_errno), "%x",
saved_errno);
snprintf is not async-signal-safe. You'll need a loop:

char *cur = hex_errno + sizeof(hex_errno) - 1;
while (saved_errno != 0 && cur > hex_errno) {
  *cur-- = "0123456789ABCDEF"[saved_errno % 16];
  saved_errno /= 16;
}
write(errpipe_write, cur, hex_errno + sizeof(hex_errno) - cur);

http://codereview.appspot.com/223077/diff/25/1011#newcode168
Modules/_posixsubprocess.c:168: args, "OOOzOiiiiiiiiiO:fork_exec",
AAAEEEEiiiiOOOOOIIIIIzzzz!!

(sorry)

http://codereview.appspot.com/223077/diff/25/1011#newcode212
Modules/_posixsubprocess.c:212: if (!converted_args)
You use ==NULL elsewhere in this function.

http://codereview.appspot.com/223077/diff/25/1011#newcode217
Modules/_posixsubprocess.c:217: if (!borrowed_arg) {
_GET_ITEM can't fail.

http://codereview.appspot.com/223077/diff/25/1011#newcode218
Modules/_posixsubprocess.c:218: Py_DECREF(converted_args);
I would probably initialize these to NULL, Py_CLEAR them on the successful path,
and have cleanup: XDECREF them.

http://codereview.appspot.com/223077/diff/25/1011#newcode249
Modules/_posixsubprocess.c:249: pid = fork();
On 2010/03/01 04:45:44, gregory.p.smith wrote:
> If a preexec_fn exists:
>   * I need to acquire the import lock before calling fork.
>   * in the child process call PyOS_AfterFork.
>   * in the parent process release the import lock after the fork.
> 
> (see posixmodule.c)
> And call PyOS_AfterFork() in the child process afterwards 

Is this a TODO?

http://codereview.appspot.com/223077/diff/25/1011#newcode251
Modules/_posixsubprocess.c:251: /* Child process */
Please comment that code from here to _exit() must only use async-signal-safe
functions, listed at `man 7 signal` or
http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html.

http://codereview.appspot.com/223077/diff/25/1011#newcode269
Modules/_posixsubprocess.c:269: /* Reenable gc in the parent process (or if fork
failed). */
This needs to happen in cleanup too, right? Since there are several branches to
there after gc is disabled.

http://codereview.appspot.com/223077/diff/25/1011#newcode284
Modules/_posixsubprocess.c:284: return PyErr_SetFromErrno(PyExc_OSError);  /*
fork() failed */
You should collect errno immediately after the fork() because all the work since
then could change it.

http://codereview.appspot.com/223077/diff/25/1011#newcode339
Modules/_posixsubprocess.c:339: -1,  /* TODO(gps): double check if this could be
0 */
http://docs.python.org/3.1/c-api/module.html?highlight=pymoduledef#PyModuleDef
says "If no memory is needed, set this to -1."

http://codereview.appspot.com/223077/diff/25/1011#newcode354
Modules/_posixsubprocess.c:354: if (m == NULL)
You can just return m here.

http://codereview.appspot.com/223077/diff/25/1006
File Objects/abstract.c (right):

http://codereview.appspot.com/223077/diff/25/1006#newcode2733
Objects/abstract.c:2733: * be freed by the caller using free().
Or using _Py_FreeCharPArray()?

http://codereview.appspot.com/223077/diff/25/1006#newcode2735
Objects/abstract.c:2735: char ** _PySequence_BytesToCharpArray(PyObject* self)
Python style puts a newline after the return type.

http://codereview.appspot.com/223077/diff/25/1006#newcode2737
Objects/abstract.c:2737: const char ** array;
Be consistent about which side of the '*' your space goes on. Python almost
always puts it before.

http://codereview.appspot.com/223077/diff/25/1006#newcode2737
Objects/abstract.c:2737: const char ** array;
Your consts are inconsistent between here, the return type, and
_Py_FreeCharPArray()'s argument.

http://codereview.appspot.com/223077/diff/25/1006#newcode2744
Objects/abstract.c:2744: array = calloc(argc+1, sizeof(char *));
Why calloc instead of malloc? It looks like you initialize the memory yourself.

http://codereview.appspot.com/223077/diff/25/1001
File Python/pythonrun.c (right):

http://codereview.appspot.com/223077/diff/25/1001#newcode2142
Python/pythonrun.c:2142: _Py_RestoreSignals(void)
Please document that this and PyOS_setsig are (and must remain)
async-signal-safe.

http://codereview.appspot.com/223077/diff/25/1001#newcode2264
Python/pythonrun.c:2264: siginterrupt(sig, 1);
siginterrupt isn't listed in the async-signal-safe function list at
http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html, but
sigaction() is, and siginterrupt is specified to act like a series of calls to
sigaction, so it's probably safe for after-fork use, even though it may have a
race when used inside a signal handler.

[gregory.p.smith, 2010-03-02 08:45:27]

PTAL.

I also added test coverage for both the pure python implementation and the new
implementation as well as some previously untested code paths.  found & fixed
some bugs with this change as a result.  yay tests!

http://codereview.appspot.com/223077/diff/25/1007
File Doc/library/subprocess.rst (right):

http://codereview.appspot.com/223077/diff/25/1007#newcode110
Doc/library/subprocess.rst:110: If *preexec_fn* is set to a callable object,
this object will be called in the
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Could you add the "not thread-safe" comment here too?

Done.  What do you think of my wording?

http://codereview.appspot.com/223077/diff/25/1009
File Lib/subprocess.py (right):

http://codereview.appspot.com/223077/diff/25/1009#newcode75
Lib/subprocess.py:75: If preexec_fn is set to a callable object, this object
will be called
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Please mention here that passing preexec_fn makes POpen not thread-safe.

Done.

http://codereview.appspot.com/223077/diff/25/1009#newcode649
Lib/subprocess.py:649: # are None when not using PIPEs. The child objects are
None
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Please update this comment to reflect that they're now -1.

Done.

http://codereview.appspot.com/223077/diff/25/1009#newcode1079
Lib/subprocess.py:1079: env_list = []
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> This line is useless, right?

Done.

http://codereview.appspot.com/223077/diff/25/1009#newcode1102
Lib/subprocess.py:1102: # Python code running in the child process after fork,
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> This isn't a complete sentence.

heh must've been a leftover from some previous wording I was trying out.

http://codereview.appspot.com/223077/diff/25/1009#newcode1195
Lib/subprocess.py:1195: data = os.read(errpipe_read, 1048576)
On 2010/03/01 04:45:44, gregory.p.smith wrote:
> svn up and merge, this is called using _eintr_retry_call() now.  same with the
> waitpid below.

Done.

http://codereview.appspot.com/223077/diff/25/1008
File Lib/test/test_subprocess.py (right):

http://codereview.appspot.com/223077/diff/25/1008#newcode563
Lib/test/test_subprocess.py:563: # Its the perror no such file or directory
message.
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Can you word this more clearly?

Done.

http://codereview.appspot.com/223077/diff/25/1008#newcode564
Lib/test/test_subprocess.py:564: # Can we even reliably test for this as it is a
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Assert the errno value? Trigger another copy of the same error message and
check
> that they're equal?

Good idea.  done.

http://codereview.appspot.com/223077/diff/25/1011
File Modules/_posixsubprocess.c (right):

http://codereview.appspot.com/223077/diff/25/1011#newcode4
Modules/_posixsubprocess.c:4: *   Licensed under the Apache License, Version 2.0
(the "License");
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Do we really need this? Most of the other modules don't include it.

Probably not.  I'll remove it.

This is the boilerplate I put on code when I'm releasing it not as part of the
python project so that it could later be adopted if desired.

http://codereview.appspot.com/223077/diff/25/1011#newcode31
Modules/_posixsubprocess.c:31: static void child_exec(char *const exec_array[],
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Please comment that all of the code in this function must only use
> async-signal-safe functions, listed at `man 7 signal` or
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html. 
> This restriction is documented at
> http://www.opengroup.org/onlinepubs/009695399/functions/fork.html.

Good references.  Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode102
Modules/_posixsubprocess.c:102: if (preexec_fn != Py_None &&
preexec_fn_args_tuple) {
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Please comment that this block is where the user asks us to deadlock her
> program.

Done. :)

http://codereview.appspot.com/223077/diff/25/1011#newcode106
Modules/_posixsubprocess.c:106: /* memory allocation and thus potential for
deadlock.    */
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Consistentize your comment style?

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode138
Modules/_posixsubprocess.c:138: err_msg = strerror(saved_errno);
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> strerror() is not async-signal-safe (wow). You might use the deprecated
> sys_errlist directly ... or map the errno back to a string in the parent
> process?

Right, I'll just defer that to the parent.

http://codereview.appspot.com/223077/diff/25/1011#newcode139
Modules/_posixsubprocess.c:139: snprintf(hex_errno, sizeof(hex_errno), "%x",
saved_errno);
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> snprintf is not async-signal-safe. You'll need a loop:
> 
> char *cur = hex_errno + sizeof(hex_errno) - 1;
> while (saved_errno != 0 && cur > hex_errno) {
>   *cur-- = "0123456789ABCDEF"[saved_errno % 16];
>   saved_errno /= 16;
> }
> write(errpipe_write, cur, hex_errno + sizeof(hex_errno) - cur);

Thanks.  cur + 1 in the write due the -- in the loop.  Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode168
Modules/_posixsubprocess.c:168: args, "OOOzOiiiiiiiiiO:fork_exec",
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> AAAEEEEiiiiOOOOOIIIIIzzzz!!
> 
> (sorry)

hehehe :)

side note: I wonder how much time in python is spend in repeatedly parsing these
getargs strings.  probably negligible.  but it has always annoyed me that it is
yet another big switch statement.

http://codereview.appspot.com/223077/diff/25/1011#newcode212
Modules/_posixsubprocess.c:212: if (!converted_args)
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> You use ==NULL elsewhere in this function.

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode217
Modules/_posixsubprocess.c:217: if (!borrowed_arg) {
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> _GET_ITEM can't fail.

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode218
Modules/_posixsubprocess.c:218: Py_DECREF(converted_args);
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> I would probably initialize these to NULL, Py_CLEAR them on the successful
path,
> and have cleanup: XDECREF them.

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode249
Modules/_posixsubprocess.c:249: pid = fork();
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> On 2010/03/01 04:45:44, gregory.p.smith wrote:
> > If a preexec_fn exists:
> >   * I need to acquire the import lock before calling fork.
> >   * in the child process call PyOS_AfterFork.
> >   * in the parent process release the import lock after the fork.
> > 
> > (see posixmodule.c)
> > And call PyOS_AfterFork() in the child process afterwards 
> 
> Is this a TODO?

yep.  I didn't have the computer with my sandbox on it in front of me at the
time.

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode251
Modules/_posixsubprocess.c:251: /* Child process */
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Please comment that code from here to _exit() must only use async-signal-safe
> functions, listed at `man 7 signal` or
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html.

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode269
Modules/_posixsubprocess.c:269: /* Reenable gc in the parent process (or if fork
failed). */
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> This needs to happen in cleanup too, right? Since there are several branches
to
> there after gc is disabled.

Right.  I've refactored the enable call out to a function and called it where
needed.

http://codereview.appspot.com/223077/diff/25/1011#newcode284
Modules/_posixsubprocess.c:284: return PyErr_SetFromErrno(PyExc_OSError);  /*
fork() failed */
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> You should collect errno immediately after the fork() because all the work
since
> then could change it.

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode330
Modules/_posixsubprocess.c:330: {"fork_exec",	subprocess_fork_exec,    
METH_VARARGS, subprocess_fork_exec_doc},
On 2010/03/01 04:45:44, gregory.p.smith wrote:
> line length

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode339
Modules/_posixsubprocess.c:339: -1,  /* TODO(gps): double check if this could be
0 */
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> http://docs.python.org/3.1/c-api/module.html?highlight=pymoduledef#PyModuleDef
> says "If no memory is needed, set this to -1."

Done.

http://codereview.appspot.com/223077/diff/25/1011#newcode354
Modules/_posixsubprocess.c:354: if (m == NULL)
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> You can just return m here.

Done.

http://codereview.appspot.com/223077/diff/25/1006
File Objects/abstract.c (right):

http://codereview.appspot.com/223077/diff/25/1006#newcode2733
Objects/abstract.c:2733: * be freed by the caller using free().
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Or using _Py_FreeCharPArray()?

Done.

http://codereview.appspot.com/223077/diff/25/1006#newcode2735
Objects/abstract.c:2735: char ** _PySequence_BytesToCharpArray(PyObject* self)
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Python style puts a newline after the return type.

Done.

http://codereview.appspot.com/223077/diff/25/1006#newcode2737
Objects/abstract.c:2737: const char ** array;
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Your consts are inconsistent between here, the return type, and
> _Py_FreeCharPArray()'s argument.

Here I'm mallocing and assigning to it so no const really makes sense.  But I've
updated the return type and the uses elsewhere to be "char *const *"

I'm really not used to using const so I rely on the compiler to whine when I
misuse it.  No warnings with the updated code.  (I had to cast the pointer
passed to the final free to a void* to avoid a warning about discarding the
qualifier; annoying, but I don't mind doing that on free).

http://codereview.appspot.com/223077/diff/25/1006#newcode2744
Objects/abstract.c:2744: array = calloc(argc+1, sizeof(char *));
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Why calloc instead of malloc? It looks like you initialize the memory
yourself.

Good point.  Done.

http://codereview.appspot.com/223077/diff/25/1001
File Python/pythonrun.c (right):

http://codereview.appspot.com/223077/diff/25/1001#newcode2142
Python/pythonrun.c:2142: _Py_RestoreSignals(void)
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> Please document that this and PyOS_setsig are (and must remain)
> async-signal-safe.

Done.

http://codereview.appspot.com/223077/diff/25/1001#newcode2264
Python/pythonrun.c:2264: siginterrupt(sig, 1);
On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> siginterrupt isn't listed in the async-signal-safe function list at
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html,
but
> sigaction() is, and siginterrupt is specified to act like a series of calls to
> sigaction, so it's probably safe for after-fork use, even though it may have a
> race when used inside a signal handler.

Yeah thats my thought as well.

Regardless, I doubt many systems even compile this code.  I expect everything 
in use today will use the HAVE_SIGACTION code above.

[Jeffrey Yasskin, 2010-03-02 17:35:46]

http://codereview.appspot.com/223077/diff/25/1011
File Modules/_posixsubprocess.c (right):

http://codereview.appspot.com/223077/diff/25/1011#newcode168
Modules/_posixsubprocess.c:168: args, "OOOzOiiiiiiiiiO:fork_exec",
On 2010/03/02 08:45:27, gregory.p.smith wrote:
> On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> > AAAEEEEiiiiOOOOOIIIIIzzzz!!
> > 
> > (sorry)
> 
> hehehe :)
> 
> side note: I wonder how much time in python is spend in repeatedly parsing
these
> getargs strings.  probably negligible.  but it has always annoyed me that it
is
> yet another big switch statement.
> 

PyArg_ParseTuple is noticeable in oprofile, although not all of that is
necessarily the switch itself. If we were working in C++, it'd be reasonable to
change it, but in C the syntax would take too much space.

http://codereview.appspot.com/223077/diff/25/1006
File Objects/abstract.c (right):

http://codereview.appspot.com/223077/diff/25/1006#newcode2737
Objects/abstract.c:2737: const char ** array;
On 2010/03/02 08:45:27, gregory.p.smith wrote:
> On 2010/03/02 02:17:12, Jeffrey Yasskin wrote:
> > Your consts are inconsistent between here, the return type, and
> > _Py_FreeCharPArray()'s argument.
> 
> Here I'm mallocing and assigning to it so no const really makes sense.  But
I've
> updated the return type and the uses elsewhere to be "char *const *"

Yeah, I wouldn't mind no-const.

> I'm really not used to using const so I rely on the compiler to whine when I
> misuse it.  No warnings with the updated code.  (I had to cast the pointer
> passed to the final free to a void* to avoid a warning about discarding the
> qualifier; annoying, but I don't mind doing that on free).

Yep, that's right.

http://codereview.appspot.com/223077/diff/28/35
File Doc/library/subprocess.rst (right):

http://codereview.appspot.com/223077/diff/28/35#newcode31
Doc/library/subprocess.rst:31: .. class:: Popen(args, bufsize=0,
executable=None, stdin=None, stdout=None, stderr=None, preexec_fn=None,
close_fds=False, shell=False, cwd=None, env=None, universal_newlines=False,
startupinfo=None, creationflags=0, restore_signals=True, call_setsid=False)
s/call_setsid/start_new_session/ ?

http://codereview.appspot.com/223077/diff/28/35#newcode120
Doc/library/subprocess.rst:120: IO or anything else that might cause the Python
interpreter to allocate
Just calling a Python function allocates memory (the frame), and if you do
anything, it allocates more, so it's not possible to avoid the malloc lock.
Maybe just tell people to minimize the number of different libraries they call
into or to minimize the number of locks involved.

http://codereview.appspot.com/223077/diff/28/37
File Lib/subprocess.py (right):

http://codereview.appspot.com/223077/diff/28/37#newcode1194
Lib/subprocess.py:1194: except Exception as e:
I think you do have to catch everything, or the child may skip the os._exit,
which will cause really weird problems in the user's app.

http://codereview.appspot.com/223077/diff/28/37#newcode1199
Lib/subprocess.py:1199: message = '%s:%x:%s' % (type(e).__name__, errno, e)
80 cols

http://codereview.appspot.com/223077/diff/28/37#newcode1242
Lib/subprocess.py:1242: builtins, exception_name.decode('ascii'),
If exception_name wound up as 'RuntimeError', this decode will raise.

http://codereview.appspot.com/223077/diff/28/36
File Lib/test/test_subprocess.py (right):

http://codereview.appspot.com/223077/diff/28/36#newcode559
Lib/test/test_subprocess.py:559: nonexistant_dir = "/_this/pa.th/does/not/exist"
Google says "nonexistent".

http://codereview.appspot.com/223077/diff/28/36#newcode584
Lib/test/test_subprocess.py:584: # Code coverage for both values of
restore_signals.  Writing a test for the actual
80 cols, and below.

http://codereview.appspot.com/223077/diff/28/36#newcode590
Lib/test/test_subprocess.py:590: # This process will not die on its own as
SIGPIPE is still ignored.
I don't really understand the point of the contents of the command. How could
this test possibly fail?

http://codereview.appspot.com/223077/diff/28/36#newcode599
Lib/test/test_subprocess.py:599: p = subprocess.Popen([sys.executable, "-c",
""], call_setsid=True)
Do you want to have this print os.getpgid() (when there's no EPERM) to make sure
setsid had the expected effect? Or will that be too flaky?

http://codereview.appspot.com/223077/diff/28/36#newcode635
Lib/test/test_subprocess.py:635: self.fail("Exception raised by preexec_fn did
not make it to the parent.")
80 cols

http://codereview.appspot.com/223077/diff/28/39
File Modules/_posixsubprocess.c (right):

http://codereview.appspot.com/223077/diff/28/39#newcode155
Modules/_posixsubprocess.c:155: *cur-- = "0123456789ABCDEF"[saved_errno % 16];
I think the "hex_errno+sizeof(hex_errno)-cur" is still off by one (oops). Maybe
switch this to *--cur and delete all the +/-1s.

http://codereview.appspot.com/223077/diff/28/39#newcode161
Modules/_posixsubprocess.c:161: * The parent process look the error message up.
*/
"...process must-or-will look..."

http://codereview.appspot.com/223077/diff/28/39#newcode273
Modules/_posixsubprocess.c:273: * back into Python.  The user asked us to use
hope as a strategy
:)

http://codereview.appspot.com/223077/diff/28/39#newcode378
Modules/_posixsubprocess.c:378: #ifdef _SC_OPEN_MAX
Good catch.

[gregory.p.smith, 2010-03-14 06:09:55]

Please take another look.

The test_subprocess unittest passes in both debug and optimized builds on OS X.

http://codereview.appspot.com/223077/diff/28/35
File Doc/library/subprocess.rst (right):

http://codereview.appspot.com/223077/diff/28/35#newcode31
Doc/library/subprocess.rst:31: .. class:: Popen(args, bufsize=0,
executable=None, stdin=None, stdout=None, stderr=None, preexec_fn=None,
close_fds=False, shell=False, cwd=None, env=None, universal_newlines=False,
startupinfo=None, creationflags=0, restore_signals=True, call_setsid=False)
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> s/call_setsid/start_new_session/ ?

good idea, thats a better name.  done.

http://codereview.appspot.com/223077/diff/28/35#newcode120
Doc/library/subprocess.rst:120: IO or anything else that might cause the Python
interpreter to allocate
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> Just calling a Python function allocates memory (the frame), and if you do
> anything, it allocates more, so it's not possible to avoid the malloc lock.
> Maybe just tell people to minimize the number of different libraries they call
> into or to minimize the number of locks involved.

done.

http://codereview.appspot.com/223077/diff/28/37
File Lib/subprocess.py (right):

http://codereview.appspot.com/223077/diff/28/37#newcode1194
Lib/subprocess.py:1194: except Exception as e:
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> I think you do have to catch everything, or the child may skip the os._exit,
> which will cause really weird problems in the user's app.

right.  done.  with an additional try: except: around the entire except: handler
for good measure.

http://codereview.appspot.com/223077/diff/28/37#newcode1199
Lib/subprocess.py:1199: message = '%s:%x:%s' % (type(e).__name__, errno, e)
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> 80 cols

Done.

http://codereview.appspot.com/223077/diff/28/37#newcode1242
Lib/subprocess.py:1242: builtins, exception_name.decode('ascii'),
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> If exception_name wound up as 'RuntimeError', this decode will raise.

whoops, appropriate b''s applied above.

>>> getattr(builtins, b'RuntimeError'.decode('ascii'))
<class 'RuntimeError'>

http://codereview.appspot.com/223077/diff/28/36
File Lib/test/test_subprocess.py (right):

http://codereview.appspot.com/223077/diff/28/36#newcode559
Lib/test/test_subprocess.py:559: nonexistant_dir = "/_this/pa.th/does/not/exist"
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> Google says "nonexistent".

Done.

http://codereview.appspot.com/223077/diff/28/36#newcode584
Lib/test/test_subprocess.py:584: # Code coverage for both values of
restore_signals.  Writing a test for the actual
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> 80 cols, and below.

Done.

http://codereview.appspot.com/223077/diff/28/36#newcode590
Lib/test/test_subprocess.py:590: # This process will not die on its own as
SIGPIPE is still ignored.
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> I don't really understand the point of the contents of the command. How could
> this test possibly fail?

All I'm trying to test here is code coverage at the moment.  Writing a test to
verify the actual behavior is hard.

I'd like to launch a sys.executable subprocess to do it but as that sub-python
would resets the signals in question to SIG_IGN on startup... that won't work.

Anyways i've removed the dependency on the shell and shell commands.

http://codereview.appspot.com/223077/diff/28/36#newcode599
Lib/test/test_subprocess.py:599: p = subprocess.Popen([sys.executable, "-c",
""], call_setsid=True)
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> Do you want to have this print os.getpgid() (when there's no EPERM) to make
sure
> setsid had the expected effect? Or will that be too flaky?

Good idea.  I'm guessing it won't be too flaky.  If so we can revert it to a do
nothing script instead.

http://codereview.appspot.com/223077/diff/28/36#newcode635
Lib/test/test_subprocess.py:635: self.fail("Exception raised by preexec_fn did
not make it to the parent.")
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> 80 cols

Done.

http://codereview.appspot.com/223077/diff/28/39
File Modules/_posixsubprocess.c (right):

http://codereview.appspot.com/223077/diff/28/39#newcode155
Modules/_posixsubprocess.c:155: *cur-- = "0123456789ABCDEF"[saved_errno % 16];
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> I think the "hex_errno+sizeof(hex_errno)-cur" is still off by one (oops).
Maybe
> switch this to *--cur and delete all the +/-1s.

agreed, that looks cleaner.

http://codereview.appspot.com/223077/diff/28/39#newcode161
Modules/_posixsubprocess.c:161: * The parent process look the error message up.
*/
On 2010/03/02 17:35:46, Jeffrey Yasskin wrote:
> "...process must-or-will look..."

Done.

[Jeffrey Yasskin, 2010-03-14 06:26:44]

Looks good to me!

[guido, 2010-03-15 01:28:18]

On 2010/03/14 06:26:44, Jeffrey Yasskin wrote:
> Looks good to me!

Thanks Greg! Excellent news.