Changes by Serhiy Storchaka storch...@gmail.com:
--
assignee: - serhiy.storchaka
type: - behavior
versions: +Python 3.4 -Python 3.2
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12085
___
Roundup Robot added 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
Serhiy Storchaka added the comment:
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.
Serhiy Storchaka added the comment:
Here is a patch. It fixes also other issues with globals nullification (similar
to issue19021).
--
Added file: http://bugs.python.org/file31849/subprocess_del.patch
___
Python tracker rep...@bugs.python.org
Terry J. Reedy added the comment:
def _handle_exitstatus(self, sts, _WIFSIGNALED=os.WIFSIGNALED,
_WTERMSIG=os.WTERMSIG, _WIFEXITED=os.WIFEXITED,
-_WEXITSTATUS=os.WEXITSTATUS):
+_WEXITSTATUS=os.WEXITSTATUS, _SubprocessError=SubprocessError):
Terry J. Reedy added the comment:
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
R. David Murray added the comment:
__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
Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com:
--
nosy: +Arfrever
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue12085
___
Guido van Rossum added the comment:
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
Antoine Pitrou added the comment:
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__,
Terry J. Reedy added the comment:
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:
Terry J. Reedy added the comment:
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
R. David Murray added the comment:
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.
Oleg Oshmyan added the comment:
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
Terry J. Reedy added the comment:
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
Serhiy Storchaka added the comment:
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
Terry J. Reedy added the comment:
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
Terry J. Reedy added the comment:
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
Roundup Robot devnull@devnull added the comment:
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
Roundup Robot devnull@devnull added the comment:
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
--
___
Python tracker
Oleg Oshmyan chor...@inbox.lv added the comment:
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. :-)
--
Added file:
Petri Lehtinen pe...@digip.org added the comment:
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.
--
nosy: +petri.lehtinen
___
Python tracker
STINNER Victor victor.stin...@haypocalc.com added the comment:
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()
Petri Lehtinen pe...@digip.org added the comment:
STINNER Victor wrote:
We can use a class attribute to set the attribute before calling __init__:
True, this is clever. Will you commit?
--
___
Python tracker rep...@bugs.python.org
STINNER Victor victor.stin...@haypocalc.com added the comment:
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?
--
___
Python tracker
Petri Lehtinen pe...@digip.org added the comment:
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__
Oleg Oshmyan chor...@inbox.lv added the comment:
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__,
STINNER Victor victor.stin...@haypocalc.com added the comment:
Should I attempt to modify my patch to include a comment and a test?
Yes please, you can use support.captured_stderr() tool.
--
___
Python tracker rep...@bugs.python.org
Petri Lehtinen pe...@digip.org added the comment:
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.
--
___
Python tracker rep...@bugs.python.org
Oleg Oshmyan chor...@inbox.lv added the comment:
Here is a new patch; please take a look. Do I understand correctly that I
should now remove the old one?
--
Added file: http://bugs.python.org/file22123/_child_created_2.diff
___
Python tracker
Oleg Oshmyan chor...@inbox.lv added the comment:
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
Petri Lehtinen pe...@digip.org added the comment:
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.
New submission from Oleg Oshmyan chor...@inbox.lv:
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)
33 matches
Mail list logo
