If subprocess.Popen is called with a keyword argument whose name is undefined or simply too many arguments, an instance of the Popen class is created but its __init__ method call fails due to the invalid argument list. (Immediately) afterwards, the new instance is destroyed and its __del__ method is called, which checks the _child_created field that is defined by __init__; but __init__ never got the chance to execute, so the field is not defined, and __del__ raises an AttributeError, which is written out to stderr.

>>> subprocess.Popen(fdsa=1)
Exception AttributeError: "'Popen' object has no attribute '_child_created'" in <bound method Popen.__del__ of <subprocess.Popen object at 0x1006ee790>> ignored
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() got an unexpected keyword argument 'fdsa'
>>> subprocess.Popen((), 0, None, None, None, None, None, True, False, None, None, False, None, 0, True, False, (), None)
Exception AttributeError: "'Popen' object has no attribute '_child_created'" in <bound method Popen.__del__ of <subprocess.Popen object at 0x1006ee790>> ignored
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() takes at most 18 positional arguments (19 given)

I encountered this while trying to write a piece of code compatible with Python 3.2 and earlier versions simultaneously. The subprocess module in Python 3.2 adds a new argument to the constructor of Popen, pass_fds, while changing the default value of another, close_fds, from False to True. In order to utilize pass_fds, I tried code that looked like this:

    try:
        process = Popen(*args, pass_fds=myfds, **kwargs)
    except TypeError:
        process = Popen(*args, **kwargs)

It worked but printed a message about the exception in __del__, which interfered with my own output. Without delving too much into the details of my code, I ended up just passing close_fds=False.

The attached patch avoids the exception by converting the reference to _child_created in __del__ into a three-argument getattr call.

IMO it should be explained in a comment why getattr is used instead of just self._child_created. Otherwise the patch looks good and useful.

We can use a class attribute to set the attribute before calling __init__:

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -664,6 +664,8 @@ _PLATFORM_DEFAULT_CLOSE_FDS = object()
 
 
 class Popen(object):
+    _child_created = False
+
     def __init__(self, args, bufsize=0, executable=None,
                  stdin=None, stdout=None, stderr=None,
                  preexec_fn=None, close_fds=_PLATFORM_DEFAULT_CLOSE_FDS,

STINNER Victor wrote:
> We can use a class attribute to set the attribute before calling __init__:

True, this is clever. Will you commit?

> True, this is clever. Will you commit?

I'm not sure that it's the pythonic way to solve such problem. Can you work on a patch including a test?

> I'm not sure that it's the pythonic way to solve such problem. Can you work on a patch including a test?

Yeah, getattr() might be more Pythonic indeed.

How can this be tested? According to the initial report, exceptions raised in __del__ seem to be ignored.

> We can use a class attribute to set the attribute before calling __init__

Ah, yes, I thought about adding a class attribute as well, but the class currently does not have any and initializes instance attributes to default values in __init__, so I chose getattr.

> How can this be tested? According to the initial report, exceptions raised in __del__ seem to be ignored.

Exceptions raised in __del__ are printed to sys.stderr, even if it has been reassigned by Python code:

>>> import sys, io, subprocess
>>> sys.stderr = io.StringIO()
>>> subprocess.Popen(fdsa=1)
>>> sys.stderr.getvalue()
'Exception AttributeError: "\'Popen\' object has no attribute \'_child_created\'" in <bound method Popen.__del__ of <subprocess.Popen object at 0x1006ee710>> ignored\nTraceback (most recent call last):\n  File "<stdin>", line 1, in <module>\nTypeError: __init__() got an unexpected keyword argument \'fdsa\'\n'

test_generators.py already uses this trick in a test similar to what would be needed for this issue around line 1856.

Should I attempt to modify my patch to include a comment and a test?

> Should I attempt to modify my patch to include a comment and a test?

Yes please, you can use support.captured_stderr() tool.

Oleg Oshmyan wrote:
> Should I attempt to modify my patch to include a comment and a test?

Absolutely, go ahead if it's not too much trouble.

Here is a new patch; please take a look. Do I understand correctly that I should now remove the old one?

Looking at my own patch, I've noticed that the comment I added to Popen.__del__ doesn't sound quite right to me in terms of language and doesn't end in a full stop while all other comments in the same method do. Am I being too picky? I can't really think of a wording that would sound any better without it also being awkward though.

I don't think you should remove the old patch, as it's a part of the discussion.

For the new patch, I'd explicitly state what can go wrong, e.g. add "(e.g. invalid parameters passed to __init__)" or something like that.

Otherwise, looks good.

I've added passing __init__ an undeclared keyword argument as an example to the comment following your suggestion. I've also reworded the comment to make it sound better to me and added a full stop. :-)

New changeset 0f2714e49583 by Victor Stinner in branch '3.2':
Close #12085: Fix an attribute error in subprocess.Popen destructor if the
http://hg.python.org/cpython/rev/0f2714e49583

New changeset 71dfd8cf4bf5 by Victor Stinner in branch 'default':
(Merge 3.2) Close #12085: Fix an attribute error in subprocess.Popen destructor
http://hg.python.org/cpython/rev/71dfd8cf4bf5

New changeset 26ea0a46aadd by Victor Stinner in branch '2.7':
Close #12085: Fix an attribute error in subprocess.Popen destructor if the
http://hg.python.org/cpython/rev/26ea0a46aadd

New changeset 07b43607a905 by Victor Stinner in branch '2.7':
Issue #12085: Fix test_subprocess for my previous commit
http://hg.python.org/cpython/rev/07b43607a905