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

I think this patch is a false solution and should be reverted. Oleg should have been told to use sys.version to make the correct call. Given how Python is, that is the correct solution to his problem.

The result of this patch is #19021: an AttributeError on shutdown when getattr gets cleared before the Popen instance.

The problem of Python putting out an uncatchable error message when deleting an uninitialized instance is generic and not specific to Popen.

class C():
    def __init__(self): self.a = 1
    def __del__(self): self.a

try:
    C(1)
except (TypeError, AttributeError): pass

# prints
Exception AttributeError: "'C' object has no attribute 'a'" in <bound method C.__del__ of <__main__.C object at 0x00000000033FB128>> ignored

If there is to be any change, it should be as generic as the problem. Perhaps that message should simply be eliminated. Perhaps it should be a Warning, which could be blocked. Perhaps there should be a real AttributeError chained with the TypeError.


What we should not do and should not tell people to do is add at least one getattr call to every __del__ method.

The same issue arises when an exception is raised during the __init__ execution, rather than in the call argument match phase, and the exception is caught.

class C():
    def __init__(self): self.a = self.b
    def __del__(self): self.a

try:
    C()
except AttributeError): pass
# print same message as before about self.a in __del__

What is different is that if the exception raised within __init__ is not caught, only the normal traceback is printed.

As Victor suggested in msg136939 we can use a class attribute. This technique is used in some other places in the stdlib (perhaps for other purposes). For example in subprocess.Handle.

We should check all defined __del__-s in stdlib and fix them all if they use a instance attribute created in __init__.

A class attribute is still a special case fix to a generic problem, if indeed the message is a problem.

If class attribute backup is to become a requirement of all delete methods, it needs to first be documented, after pydev discussion. To apply the class attribute fix generally is tricky. If one does not create a class attribute backup for every attribute referenced in __del__, one must analyze the __init__ method for all points of possible failure, to see which attributes referenced in __del__ might be missing. Changing __init__ might change the analysis. This looks like a bad path to me.

The whole point of the special case ignoring of AttributeError in __delete__ methods is that AttributeErrors are *expected* in certain circumstances.

I opened a thread on pydev to discuss this issue.
"Revert #12085 fix for __del__ attribute error message"

The OP can avoid this issue entirely by using a conditional
  if sys.version_info < (3, 2, 0)
I consider this better code than intentionally creating an uninitialized instance.

__del__ methods are in general tricky because they are in the general case run asynchronously.  Therefore any proposal to "attach" the message to another message is a non-starter.

If a __del__ method depends on attributes set in the __init__, then the programmer needs to decide if they want to handle the possibility of __init__ failing, and therefore __del__ running without __init__ having completed.  For the stdlib, I think I'd lean toward handling such cases, in which case IMO the Pythonic thing to do is indeed to have a class attribute to provide the pre-__init__ default.

FWIW, using class attributes to ensure __del__ does not hit AttributeError when __init__ failed is more idiomatic than using three-argument getattr().

The reason: in general it is possible that __del__ calls almost any other method on a class (e.g. for a buffered I.O stream open for writing, __del__ might call close() which might attempt to flush the buffer).  It is not reasonable (and ugly :-) to use three-argument in all code that might be called by __del__.  But it is reasonable to use class attributes to pre-initialize instance variables set by __init__ to "safe" defaults like None.

> The whole point of the special case ignoring of AttributeError in 
> __delete__ methods is that AttributeErrors are *expected* in certain
> circumstances.

You are completely misunderstanding this. There is no special case for AttributeError inside __del__, every exception is treated the same.
And by the way, this behaviour is documented:
http://docs.python.org/3.3/reference/datamodel.html#object.__del__
("Due to the precarious circumstances under which __del__() methods are invoked, exceptions that occur during their execution are ignored, and a warning is printed to sys.stderr instead.")

+1 for using a class attribute here, much cleaner than a getattr() dance.

The message problem can arise during exit if __del__ depends an any attribute of any object. It is hard to imagine a __del__ method that does not. Any __del__ method, including that of Popen, could handle AttributeErrors by wrapping the whole body in

try:
  <body>
except AttributeError:
  pass

The is essentially what is done by the code that calls __del__, except that 'pass' is replaced by "print(message, file=sys.stderr)".

If we patch Popen at all, I think this try:except should be the fix, not a class attribute.

To explain what I meant by the class attribute hack being tricky, consider the original version of Popen.__del__, minus the comments. 

    def __del__(self, _maxsize=sys.maxsize, _active=_active):
        if not self._child_created:
            return
        self._internal_poll(_deadstate=_maxsize)
        if self.returncode is None and _active is not None:
            _active.append(self)

Since self._internal_poll is an instance method, it is not a problem. But what about the self.returncode data attribute? Should we also add a class 'returncode' attribute? If so, what should be its value? None? or object()? Or is it guaranteed that when _child_created is set true, returncode will be defined, so that a class attribute is not needed?

I don't know the answers to these questions. Popen.__init__ is about 130 lines and self._child_created is set to True in two other methods. I did not look where returncode is set, but it is not near the spots where _child_created is set True.

Reading Antoine's message more carefully, and the cited doc line, the generic fix to prevent the warning would be

try:
 <__del__ body>
except Exception:
  pass

The question is, do we only want to block the warning when someone calls Popen with the wrong number of arguments, or always?

No, that is not a good fix.  It would mask other programming errors.  There is a *reason* the error/traceback is printed when an error occurs.

The fact that the Popen logic may be a bit complex is not an argument in favor of a fix that hides errors.

> But what about the self.returncode data attribute? Should we also add a
> class 'returncode' attribute? If so, what should be its value? None? or
> object()? Or is it guaranteed that when _child_created is set true,
> returncode will be defined, so that a class attribute is not needed?

For what it's worth, returncode is indeed guaranteed to be defined when _child_created is True: it is initialized in __init__ before _execute_child is run. Of course, this does not help the general case of __del__ methods in other classes.

Silencing all AttributeErrors in all __del__ calls may be an easy and generic solution, but it will also hide genuine logic errors. I think it is reasonable to expect classes with __del__ to be careful about using attributes that exist, just like they must be careful about performing high-level operations that are valid in whatever state the object might be: destroy/close only things that have been created/opened, undo only actions that have been done etc.

Right. If _internal_poll raises, it should not be masked as that would be a true bug.

More research. 'self.returncode = None' comes before the only call to the appropriate posix/windows version of ._execute_child(), which is the only place where '_child_created = True'. So class level
    _child_created = False  # needed for __del__ if __init__ call fails
should be sufficient. With that added,
        self._child_created = False
in __init__ would not be really needed. 

As I said on pydev, making the warning a Warning would be a different issue.

Here is a patch. It fixes also other issues with globals nullification (similar to issue19021).

def _handle_exitstatus(self, sts, _WIFSIGNALED=os.WIFSIGNALED,
                 _WTERMSIG=os.WTERMSIG, _WIFEXITED=os.WIFEXITED,
-                _WEXITSTATUS=os.WEXITSTATUS):
+                _WEXITSTATUS=os.WEXITSTATUS, _SubprocessError=SubprocessError):
             # This method is called (indirectly) by __del__, so it cannot
             # refer to anything outside of its local scope."""

The non-effective """ should be removed. Otherwise LFTM.
Good catch on the non-local dependencies that violated the comment.

New changeset 734da14489c1 by Serhiy Storchaka in branch '2.7':
issue12085: Use more Pythonic way to check _child_created.
http://hg.python.org/cpython/rev/734da14489c1

New changeset 79a6300f6421 by Serhiy Storchaka in branch '3.3':
issue12085: Use more Pythonic way to check _child_created.
http://hg.python.org/cpython/rev/79a6300f6421

New changeset a7a62a88380a by Serhiy Storchaka in branch 'default':
issue12085: Use more Pythonic way to check _child_created.
http://hg.python.org/cpython/rev/a7a62a88380a

I excluded from patches caching of SubprocessError and OSError, because it shouldn't be an issue after committing issue19255. I also removed caching of _active because it is explicitly checked for None and it is set to None on shutdown with a purpose.