Issue 12085: subprocess.Popen.del raises AttributeError if init was called with an invalid argument list (original) (raw)
Created on 2011-05-16 03:08 by chortos, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (31)
Author: Oleg Oshmyan (chortos)
Date: 2011-05-16 03:08
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 "", line 1, in 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 "", line 1, in 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.
Author: Petri Lehtinen (petri.lehtinen) * 
Date: 2011-05-26 11:04
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.
Author: STINNER Victor (vstinner) *
Date: 2011-05-26 11:07
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,
Author: Petri Lehtinen (petri.lehtinen) *
Date: 2011-05-26 11:09
STINNER Victor wrote:
We can use a class attribute to set the attribute before calling init:
True, this is clever. Will you commit?
Author: STINNER Victor (vstinner) *
Date: 2011-05-26 11:11
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?
Author: Petri Lehtinen (petri.lehtinen) *
Date: 2011-05-26 11:20
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.
Author: Oleg Oshmyan (chortos)
Date: 2011-05-26 12:00
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 "", line 1, in \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?
Author: STINNER Victor (vstinner) *
Date: 2011-05-26 12:01
Should I attempt to modify my patch to include a comment and a test?
Yes please, you can use support.captured_stderr() tool.
Author: Petri Lehtinen (petri.lehtinen) *
Date: 2011-05-26 12:03
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.
Author: Oleg Oshmyan (chortos)
Date: 2011-05-26 12:39
Here is a new patch; please take a look. Do I understand correctly that I should now remove the old one?
Author: Oleg Oshmyan (chortos)
Date: 2011-05-26 13:57
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.
Author: Petri Lehtinen (petri.lehtinen) *
Date: 2011-05-27 05:58
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.
Author: Oleg Oshmyan (chortos)
Date: 2011-05-27 17:59
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. :-)
Author: Roundup Robot (python-dev) 
Date: 2011-05-31 23:08
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
Author: Roundup Robot (python-dev)
Date: 2011-06-01 00:01
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
Author: Terry J. Reedy (terry.reedy) *
Date: 2013-09-15 05:30
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.
Author: Terry J. Reedy (terry.reedy) *
Date: 2013-09-15 05:47
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.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2013-09-15 07:21
As Victor suggested in 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.
Author: Terry J. Reedy (terry.reedy) *
Date: 2013-09-21 21:19
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.
Author: R. David Murray (r.david.murray) *
Date: 2013-09-21 22:27
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.
Author: Guido van Rossum (gvanrossum) *
Date: 2013-09-21 22:44
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.
Author: Antoine Pitrou (pitrou) *
Date: 2013-09-21 22:49
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.
Author: Terry J. Reedy (terry.reedy) *
Date: 2013-09-21 23:02
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: 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.
Author: Terry J. Reedy (terry.reedy) *
Date: 2013-09-21 23:11
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?
Author: R. David Murray (r.david.murray) *
Date: 2013-09-21 23:31
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.
Author: Oleg Oshmyan (chortos)
Date: 2013-09-21 23:54
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.
Author: Terry J. Reedy (terry.reedy) *
Date: 2013-09-22 00:05
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.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2013-09-23 15:03
Here is a patch. It fixes also other issues with globals nullification (similar to ).
Author: Terry J. Reedy (terry.reedy) *
Date: 2013-09-23 20:05
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.
Author: Roundup Robot (python-dev)
Date: 2014-02-10 17:27
New changeset 734da14489c1 by Serhiy Storchaka in branch '2.7': : 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': : Use more Pythonic way to check _child_created. http://hg.python.org/cpython/rev/79a6300f6421
New changeset a7a62a88380a by Serhiy Storchaka in branch 'default': : Use more Pythonic way to check _child_created. http://hg.python.org/cpython/rev/a7a62a88380a
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2014-02-10 17:28
I excluded from patches caching of SubprocessError and OSError, because it shouldn't be an issue after committing . I also removed caching of _active because it is explicitly checked for None and it is set to None on shutdown with a purpose.
History
Date
User
Action
Args
2022-04-11 14:57:17
admin
set
github: 56294
2014-02-10 17:28:32
serhiy.storchaka
set
status: open -> closed
resolution: fixed
messages: +
stage: resolved
2014-02-10 17:27:52
python-dev
set
messages: +
2014-02-10 13:58:36
serhiy.storchaka
set
assignee: serhiy.storchaka
type: behavior
versions: + Python 3.4, - Python 3.2
2013-09-23 20:05:26
terry.reedy
set
messages: +
2013-09-23 15:03:07
serhiy.storchaka
set
files: + subprocess_del.patch
messages: +
2013-09-22 00:05:16
terry.reedy
set
messages: +
2013-09-21 23:54:41
chortos
set
messages: +
2013-09-21 23:31:00
r.david.murray
set
messages: +
2013-09-21 23:11:15
terry.reedy
set
messages: +
2013-09-21 23:02:07
terry.reedy
set
messages: +
2013-09-21 22:49:31
pitrou
set
nosy: + pitrou
messages: +
2013-09-21 22:44:25
gvanrossum
set
nosy: + gvanrossum
messages: +
2013-09-21 22:30:48
Arfrever
set
nosy: + Arfrever
2013-09-21 22:27:17
r.david.murray
set
nosy: + r.david.murray
messages: +
2013-09-21 21:19:32
terry.reedy
set
messages: +
2013-09-15 07:21:31
serhiy.storchaka
set
status: closed -> open
nosy: + serhiy.storchaka
messages: +
resolution: fixed -> (no value)
stage: resolved -> (no value)
2013-09-15 05:47:37
terry.reedy
set
messages: +
2013-09-15 05:30:39
terry.reedy
set
nosy: + terry.reedy
messages: +
2011-06-01 00:01:07
python-dev
set
messages: +
2011-05-31 23:08:55
python-dev
set
status: open -> closed
nosy: + python-dev
messages: +
resolution: fixed
stage: resolved
2011-05-27 17:59:21
chortos
set
files: + _child_created_3.diff
messages: +
2011-05-27 05:58:23
petri.lehtinen
set
messages: +
2011-05-26 13:57:57
chortos
set
messages: +
2011-05-26 12:39:49
chortos
set
files: + _child_created_2.diff
messages: +
2011-05-26 12:03:06
petri.lehtinen
set
messages: +
2011-05-26 12:01:49
vstinner
set
messages: +
2011-05-26 12:00:01
chortos
set
messages: +
2011-05-26 11:20:27
petri.lehtinen
set
messages: +
2011-05-26 11:11:20
vstinner
set
messages: +
2011-05-26 11:09:14
petri.lehtinen
set
messages: +
2011-05-26 11:07:48
vstinner
set
nosy: + vstinner
messages: +
2011-05-26 11:04:58
petri.lehtinen
set
nosy: + petri.lehtinen
messages: +
2011-05-16 03:08:55
chortos
create