classification
Title: subprocess.Popen.__del__ causes AttributeError (os module == None)
Type: crash Stage: patch review
Components: Extension Modules, Library (Lib) Versions: Python 3.2, Python 3.1, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: LambertDW, akuchling, brett.cannon, brian.curtin, eric.araujo, flox, ggenellina, gregory.p.smith, haypo, hozn, marystern, pitrou, tormen
Priority: high Keywords: needs review, patch

Created on 2009-01-29 16:00 by marystern, last changed 2010-05-14 21:57 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess.diff ggenellina, 2009-01-31 03:58
test_subprocess.diff ggenellina, 2009-01-31 04:14 Test case
subprocess_shutdown.diff brett.cannon, 2010-04-25 20:56
Messages (22)
msg80771 - (view) Author: Mary Stern (marystern) Date: 2009-01-29 16:00
I was getting this error (while running my unit tests):

Exception exceptions.AttributeError: "'NoneType' object has no attribute
'error'" in <bound method Popen.__del__ of <subprocess.Popen object at
0x8a2596c>> ignored

which I tracked down to the os module being set to None (yes really!) in
POpen._internal_poll() when called from Popen.__del__, so this line:

                    except os.error:

was generating the error.

I guess that the module is getting unloaded earlier somehow (maybe a
race condition)?
msg80855 - (view) Author: Gabriel Genellina (ggenellina) Date: 2009-01-31 03:58
At interpreter shutdown, the module's global variables are set to None 
before the module itself is released. __del__ methods may be called in 
those precaries circumstances, and should not rely on any global state.

A temporary fix would be to make Popen._internal_poll and 
Popen._handle_exitstatus keep a reference to the os module (just add a 
default argument os=os, like sys=sys in __del__). But this is just a 
hack; the real fix would be to avoid defining __del__ at all.

A patch for subprocess.py is attached.
msg80856 - (view) Author: Gabriel Genellina (ggenellina) Date: 2009-01-31 04:14
Patch and test case against trunk
msg85988 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-15 10:23
There should be a try/finally in test_issue5099 to ensure that
os.remove(fname) always gets called. Otherwise, looks good.
msg99713 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2010-02-22 04:23
Gabriel: could you please update the patch to take Antoine's comment into account?
msg100690 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-09 01:44
By the way, stderr will contain "[XXX refs]" as the last line with Python compiled in debug mode. The remove_stderr_debug_decorations() function will help you ignore this line.
msg100954 - (view) Author: (tormen) Date: 2010-03-12 18:56
I ran into the same problem today :(
The patch resolved it :)

BUT:
Could anyone comment on when this patch will or will not live?!

...
As the bug is fairly old and already has duplicates and everyone seems to agree on the issue.
...
Plus it seems easy enough to edit the patch to include the try: / finally: and to incorporate the remove_stderr_debug_decorations() suggestion.
msg103434 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-04-17 21:39
Attached is a patch I wrote entirely independently of this issue (Mercurial's test suite trigger it for me). I go further in terms of caching items than the original patch as I still got failures even when I stored just a reference to os (might be an OS X thing with the os module).

I just need someone to verify my patch works under Windows. If someone can do that I will commit it and Gabriel's test (with Antoine's suggested fixes).
msg103464 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-04-18 06:32
i don't see your attachment brett.
msg103511 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-04-18 18:26
Let's try this again...
msg104162 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-04-25 20:56
New patch updated to at least r80476.
msg104239 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-04-26 15:20
Hello


Brett’s patch contains strange-looking docstrings. Since they are for private methods, they could be comments instead.

The patch looks otherwise good to me.

Does anyone have an idead on how to not write a __del__ method at all, according to Gabriel’s first comment?


Regards
msg104270 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-04-26 20:11
Did you run the patch on a Windows machine, Eric?

As for Gabriel's comment about not using a __del__ method, it's a general rule of thumb, not something you have to do. __del__ methods exist for those times when you REALLY need them, but otherwise should be avoided when possible.
msg104271 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-04-26 20:22
I'm -1 on Brett's patch: keep a reference to the required functions in the function arguments is ugly to me. The process should be destroyed before unloading the modules. I don't know how (or even if it's possible or not :-)), but I think that Brett's patch is the wrong solution to the problem.

Eg. Can't we use atexit.register() to destroy the processes at exit?
msg104273 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-04-26 20:31
I was just following the style already set in __del__ for storing a reference to sys (that and I didn't feel like having to explicitly store all of those references in the __init__ or at the class level just above the methods).

As for using atexit, it's possible that could be the proper solution, it just requires someone coding it up before we hit 2.7rc1.
msg104277 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-04-26 21:15
I don’t own a Windows machine. I just read the code, sorry if that was
not helpful.

Regarding quick fix vs. right fix, nothing prevents us from using
Brett’s fix now and take some time to try the atexit way later.

acute-accent-ly yours, Éric
msg104280 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-04-26 21:42
Code reviews are always helpful, Éric. I just wanted since that is what is preventing me from committing.

And you are right that I can commit my change now and we can fix it after the fact.
msg105325 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-05-08 18:48
I think your patch looks good Brett.
msg105677 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-05-14 01:30
OK, my patch is committed:

 2.6 81158
 2.7 81154
 3.1 81159
 3.2 81155

I didn't apply your test, Gabriel, as it passed without the fixes. Thanks to the work you did on it, though.
msg105725 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-05-14 17:39
Python3 compilation fails on Windows:
....
  File "...\3.x.bolen-windows\build\lib\subprocess.py", line 911, in Popen
    _WaitForSingleObject=WaitForSingleObject,
NameError: name 'WaitForSingleObject' is not defined

http://www.python.org/dev/buildbot/3.x.stable/builders/x86%20XP-4%203.x/builds/2121/steps/compile/logs/stdio
msg105738 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-05-14 18:37
pakal wrote a patch fixing Windows regression: see issue #8717.
msg105771 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-05-14 21:57
Fix _internal_poll() on Windows: r81179 (trunk), r81181 (2.6), r81181 (3.x), r81182 (3.1).
History
Date User Action Args
2010-05-14 21:57:37hayposetstatus: open -> closed
resolution: fixed
messages: + msg105771
2010-05-14 18:37:15hayposetmessages: + msg105738
2010-05-14 17:39:05hayposetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg105725
2010-05-14 01:30:14brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg105677
2010-05-08 18:48:11gregory.p.smithsetmessages: + msg105325
2010-04-26 21:43:13brett.cannonsetpriority: normal -> high
2010-04-26 21:43:02brett.cannonsetassignee: brett.cannon
2010-04-26 21:42:41brett.cannonsetmessages: + msg104280
2010-04-26 21:15:06eric.araujosetmessages: + msg104277
2010-04-26 20:31:09brett.cannonsetmessages: + msg104273
2010-04-26 20:22:21hayposetnosy: + haypo
messages: + msg104271
2010-04-26 20:11:14brett.cannonsetmessages: + msg104270
2010-04-26 15:20:17eric.araujosetnosy: + eric.araujo

messages: + msg104239
title: subprocess.POpen.__del__() AttributeError (os module == None!) -> subprocess.Popen.__del__ causes AttributeError (os module == None)
2010-04-25 20:57:11brett.cannonsetfiles: - subprocess__del__.diff
2010-04-25 20:57:01brett.cannonsetfiles: + subprocess_shutdown.diff

messages: + msg104162
2010-04-18 18:26:41brett.cannonsetfiles: + subprocess__del__.diff

messages: + msg103511
2010-04-18 06:32:37gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg103464
2010-04-17 23:26:25r.david.murraysetnosy: + brian.curtin
2010-04-17 21:39:57brett.cannonsetnosy: + brett.cannon
messages: + msg103434
2010-03-12 18:56:08tormensetnosy: + tormen
messages: + msg100954
2010-03-09 10:35:44floxsetnosy: + flox

versions: + Python 3.2, - Python 3.0
2010-03-09 10:33:13floxlinkissue8091 superseder
2010-03-09 01:44:06pitrousetmessages: + msg100690
2010-02-22 04:23:25akuchlingsetnosy: + akuchling
messages: + msg99713
2010-02-02 16:48:18brian.curtinsetpriority: normal
keywords: + needs review
stage: patch review
2009-04-15 10:23:44pitrousetnosy: + pitrou
messages: + msg85988
2009-04-07 20:16:44hoznsetnosy: + hozn
2009-01-31 04:14:55ggenellinasetfiles: + test_subprocess.diff
messages: + msg80856
versions: + Python 3.0, Python 3.1, Python 2.7
2009-01-31 03:58:24ggenellinasetfiles: + subprocess.diff
nosy: + ggenellina
keywords: + patch
messages: + msg80855
components: + Library (Lib)
2009-01-29 16:31:53LambertDWsetnosy: + LambertDW
2009-01-29 16:01:33marysternsettitle: subprocess.POpen.__del__() AttribuetError (os module == None!) -> subprocess.POpen.__del__() AttributeError (os module == None!)
2009-01-29 16:00:06marysterncreate