classification
Title: AttributeError in Popen.__del__
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, barry, chortos, doko, haypo, larry, pitrou, sbt, serhiy.storchaka, terry.reedy
Priority: release blocker Keywords: patch

Created on 2013-09-14 23:46 by serhiy.storchaka, last changed 2014-03-07 11:12 by Arfrever. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess_del_getattr.patch serhiy.storchaka, 2013-09-14 23:46 review
subprocess_active_pids.patch serhiy.storchaka, 2014-02-10 13:57 review
Messages (25)
msg197737 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-14 23:46
Sometimes closing IDLE I got such exception:

Exception ignored in: <bound method Popen.__del__ of <subprocess.Popen object at 0xb5b8618c>>
Traceback (most recent call last):
  File "/home/serhiy/py/cpython/Lib/subprocess.py", line 890, in __del__
TypeError: 'NoneType' object is not callable

It is happened because the builtins module was destroyed before running __del__ in process of garbage collecting.

Here is a patch which should fix the bug. Anothe possible solution is get rid from getattr and catch AttributeError instead.

See also issue16650.
msg197750 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-09-15 05:50
The condition was changed from self._child_created to the getattr call in #12085. As explained in msg197748, I think that change should be reverted (and this issue closed). I think elaborating a bad patch only makes it worse.
msg198240 - (view) Author: Oleg Oshmyan (chortos) Date: 2013-09-22 01:34
> Anothe possible solution is get rid from getattr and catch
> AttributeError instead.

Surely this would suffer from the same issue?

Why are the builtins getting deleted anyway? In fact, why is getattr getting deleted from the builtins module? The __builtins__ global is specifically not deleted when deleting modules to let destructors use builtins, and indeed the error message says "is not callable", suggesting the builtins module is still there but getattr is not. Is this perhaps some sort of IDLE bug?
msg198247 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-09-22 07:43
When the patch to #12085 is changed, as has been agreed, I think, this issue should go away.

Moving the deletion of builtins to later in the shutdown process has be discussed and maybe implemented.
msg198264 - (view) Author: Oleg Oshmyan (chortos) Date: 2013-09-22 11:36
But the thing is, builtins are already supposed to be the very last thing destroyed at shutdown.
msg198389 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-25 15:35
I'm wondering too. While the code in Popen.__del__() is almost same, I can't reproduce the issue in 3.3. Perhaps it relates to some 3.4 changes? Import machinery, weak references, the shutdown process?

Before applying the patch which fixes Popen.__del__() I want understand what happens.
msg198392 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2013-09-25 16:06
The clearing of modules at shutdown has been substantially changed in 3.4.  Now a best effort is made to let the module go away purely by gc.  Those modules which survive get purged in random order.

In 3.3 all modules were purged, but builtins was special cased to be purged last.  (See Python/import.c:PyImport_Cleanup().)

I would favour setting a flag before the purging stage which prevents __del__ methods (and weakrefs?) from running.
msg198404 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-25 18:02
Could we instead restore the builtins module to it's initial state before the purging stage? I believe all builtins are immutable and can't create reference loops.
msg198413 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-09-25 19:38
> Before applying the patch which fixes Popen.__del__()

I think your #12085 patch should be applied and that issue closed. It is not specifically about shutdown. Tweaking shutdown further would be a new issue.
msg198861 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-10-02 18:55
There is a regression in 3.4 due to changes in shutdown procedure. This code correctly works in 3.3. There are more than a dozen places in the stdlib which rely upon accessibility of builtins. I wrote patches for all these cases, but third-party code will be broken.

I think we should restore guarantees about builtins.
msg198893 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-03 12:44
> There is a regression in 3.4 due to changes in shutdown procedure. This 
> code correctly works in 3.3. There are more than a dozen places in the 
> stdlib which rely upon accessibility of builtins.

Well, perhaps we can special-case builtins not to be "wiped" at shutdown.
However, there is another problem here in that the Popen object survives until the builtins module is wiped. This should be investigated too.
msg198897 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2013-10-03 14:15
> Well, perhaps we can special-case builtins not to be "wiped" at shutdown.
> However, there is another problem here in that the Popen object survives 
> until the builtins module is wiped. This should be investigated too.

Maybe it is because it uses the evil resuscitate-in-__del__ trick.  I 
presume that if the child process survives during shutdown, then the popen 
object is guaranteed to survive too.

We could get rid of the trick:

* On Windows __del__ is unneeded since we don't need to reap zombie 
  processes.

* On Unix __del__ could just add self._pid (rather than self) to the list 
  _active.  _cleanup() would then use os.waitpid() to check the pids in 
  _active.

The hardest thing about making such a change is that test_subprocess 
currently uses _active.
msg210834 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-10 13:57
Here is a patch which implements Richard's suggestion: _active now contains pid-s instead of Popen instances. But this doesn't fix this issue. Patches for issue19255 and issue12085 fixes it.
msg212359 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-27 15:28
We're seeing this in Ubuntu now that 3.4 is the default.

https://bugs.launchpad.net/python/+bug/1284469
msg212361 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-02-27 15:46
Serhiy fixed the issue #19255 with the changeset 6a1711c96fa6, but this changeset will wait Python 3.4.1.
msg212369 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-27 16:51
On Feb 27, 2014, at 03:46 PM, STINNER Victor wrote:

>Serhiy fixed the issue #19255 with the changeset 6a1711c96fa6, but this
>changeset will wait Python 3.4.1.

Okay, thanks.  I was reviewing and rather liked the less invasive patch, but
if this one is going to make it into 3.4.1, I'll review it and test it
locally.  We can patch Ubuntu's 3.4 for now.
msg212375 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-27 19:17
If this fixes the problem, shouldn't the issue be closed and a NEWS item added?  I'm going to test the patch locally.
msg212376 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-27 19:23
Hmm... if this *already* affects Ubuntu users, shouldn't this be fixed in 3.4 proper? It's extremely likely that someone else will be affected too.
msg212381 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-27 20:05
On Feb 27, 2014, at 07:23 PM, Antoine Pitrou wrote:

>Hmm... if this *already* affects Ubuntu users, shouldn't this be fixed in 3.4
>proper? It's extremely likely that someone else will be affected too.

Agreed!
msg212439 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-28 14:57
I've testing this patch on Ubuntu, and it seems to fix the problem.  My quick testing doesn't show any new problems, but we'll only know for sure once the new Python 3.4 package hits the archive and folks start updating to it.  So far so good though.

Larry, please consider cherry picking this into 3.4.0 final.
msg212441 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-28 15:06
Nosying Doko, since I think he may want to get this fix into Debian, if Larry does not cherry pick it into 3.4.0 final.
msg212475 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-28 21:42
Note that the changeset 6a1711c96fa6 introdused several errors (mostly tests failures), so it needs other changesets to fix it. Related changesets which should be cherry picked: 6a1711c96fa6, fa160c8145e5, efaf12106d68, 7ecee9e0dc58, 10ea3125d7b8, 488ccbee6ee6.
msg212477 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-02-28 21:49
On Feb 28, 2014, at 09:42 PM, Serhiy Storchaka wrote:

>Note that the changeset 6a1711c96fa6 introdused several errors (mostly tests
>failures), so it needs other changesets to fix it. Related changesets which
>should be cherry picked: 6a1711c96fa6, fa160c8145e5, efaf12106d68,
>7ecee9e0dc58, 10ea3125d7b8, 488ccbee6ee6.

Interestingly enough, the test failures don't seem to break the build, but do
seem to break the "DEP 8" tests, which are run on the built package and must
pass before the package is promoted to the primary archive.  It takes quite a
while to build Python for Ubuntu (since we do several builds, e.g. debug
builds, etc. along with all the tests), so I've just noticed this.

Thanks for listing the additional revisions that need to be cherry picked.  I
will investigate for Ubuntu, but it does convince me more that Larry should
attempt to pull these into 3.4.0.
msg212817 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-03-06 17:21
So you're asking that I cherry pick six revisions here?

6a1711c96fa6
fa160c8145e5
efaf12106d68
7ecee9e0dc58
10ea3125d7b8
488ccbee6ee6
msg212875 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-03-07 11:04
Those six revisions have been cherry-picked into 3.4.0.
History
Date User Action Args
2014-03-07 11:12:28Arfreversetstage: needs patch -> resolved
2014-03-07 11:04:31larrysetstatus: open -> closed
resolution: fixed
messages: + msg212875
2014-03-06 17:21:11larrysetmessages: + msg212817
2014-03-04 23:54:58barrysetpriority: normal -> release blocker
2014-02-28 21:49:03barrysetmessages: + msg212477
2014-02-28 21:42:18serhiy.storchakasetmessages: + msg212475
2014-02-28 15:06:26barrysetnosy: + doko
messages: + msg212441
2014-02-28 14:57:12barrysetmessages: + msg212439
2014-02-27 20:05:14barrysetmessages: + msg212381
2014-02-27 19:23:56pitrousetmessages: + msg212376
2014-02-27 19:17:12barrysetmessages: + msg212375
2014-02-27 16:51:38barrysetmessages: + msg212369
2014-02-27 15:46:15hayposetnosy: + haypo
messages: + msg212361
2014-02-27 15:44:29hayposetnosy: + larry
2014-02-27 15:28:45barrysetnosy: + barry
messages: + msg212359
2014-02-10 13:57:15serhiy.storchakasetfiles: + subprocess_active_pids.patch

messages: + msg210834
2013-10-13 21:22:40serhiy.storchakasetstage: patch review -> needs patch
2013-10-03 14:15:20sbtsetmessages: + msg198897
2013-10-03 12:44:34pitrousetmessages: + msg198893
2013-10-02 18:55:27serhiy.storchakasetmessages: + msg198861
versions: - Python 2.7, Python 3.3
2013-09-25 19:38:37terry.reedysetmessages: + msg198413
2013-09-25 18:02:32serhiy.storchakasetmessages: + msg198404
2013-09-25 16:06:38sbtsetnosy: + pitrou, sbt
messages: + msg198392
2013-09-25 15:35:36serhiy.storchakasetmessages: + msg198389
2013-09-22 11:36:23chortossetmessages: + msg198264
2013-09-22 07:43:41terry.reedysetmessages: + msg198247
2013-09-22 01:34:39chortossetnosy: + chortos
messages: + msg198240
2013-09-15 05:50:10terry.reedysetmessages: + msg197750
2013-09-15 04:30:23Arfreversetnosy: + Arfrever
2013-09-14 23:46:46serhiy.storchakacreate