classification
Title: Interpreter cleanup: order of _PyGILState_Fini and PyInterpreterState_Clear
Type: crash Stage:
Components: Interpreter Core Versions: Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, gvanrossum, ronaldoussoren
Priority: normal Keywords:

Created on 2007-11-07 22:05 by ronaldoussoren, last changed 2007-11-29 23:36 by amaury.forgeotdarc. This issue is now closed.

Files
File name Uploaded Description Edit
threads.py ronaldoussoren, 2007-11-07 22:05
pygilstate.patch ronaldoussoren, 2007-11-22 10:52
test_gilstate.patch amaury.forgeotdarc, 2007-11-23 23:34
Messages (10)
msg57225 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2007-11-07 22:05
Py_Finalize cleans up the thread state by first calling _PyGILState_Fini 
and then calling PyInterpreterState_Clear. The latter can cause user 
code to run, which could use the GILState API and this then causes a 
crash.

The attached file 'threads.py' causes a crash on OSX leopard because of 
this issue. The script causes an exception to be set that has an 
attribute that uses the GILState API in its dealloc slot.
msg57285 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-09 00:05
Do you have a patch?  Then we could consider fixing this in 2.5.2.
msg57297 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2007-11-09 05:23
I don't have a patch. I don't even know enough of the threading 
infrastructure to know if this really a bug or if it is a bug in my 
code.

I'll work on a patch this weekend, if changing the order of calls to 
PyGILState_Fini and PyInterpreterState_Clear doesn't break unittests and 
fixes the problem I ran into I'll post about this issue on python-dev.
msg57754 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2007-11-22 10:52
The attached patch fixes the crash, but I haven't verified if the patch 
is actually correct. 

With this patch some PyThread API's are called after PyInterpreterState_Clear and I don't know if it is valid to do so. All 
unittests pass fine on OSX though.
msg57795 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2007-11-23 23:34
I managed to reproduce the problem consistently with the following code:

import ctypes, sys, time, thread

# Module globals are cleared before __del__ is run
# So save the functions in class dict
class C:
    ensure = ctypes.pythonapi.PyGILState_Ensure
    release = ctypes.pythonapi.PyGILState_Release
    def __del__(self):
        state = self.ensure()
        self.release(state)

def waitingThread():
    x = C()
    time.sleep(100)

thread.start_new_thread(waitingThread, ())
time.sleep(1)
sys.exit(42)


On exit, PyInterpreterState_Clear stops the sleeping thread and free its
local variables. But at this time _PyGILState_Fini has already been
called...

The initial patch also corrects this problem. I join another patch,
which adds the above code in a testcase.

Ready to check-in, but can someone have a look?
msg57856 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-26 22:03
So is this a Mac-only issue?

And couldn't the GIL state cleanup also invoke user code, which might be
abused to create more threads, wreaking havoc that way?  I'm kind of
worried about putting this into 2.5.2 and breaking somebody's working
code (that's always worse than fixing somebody's broken code... :-).
msg57865 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2007-11-27 05:38
This is not a mac-specific issue, the script happens to be mac-specific 
because that's how I found the issue.

Amaury's patch adds a unittest that reproduces the problem in a 
platform-indepenent way using ctypes.

The _PyGILState_Fini function might cause user code to run as well, it 
removes the thread-local variable that contains the PyThreadState for 
threads, and that contains some Python objects that might contain 
arbitrary values (such as the last exception value). I guess that means 
that a variation on Amaury's patch would cause a patched interpreter to 
crash (create on instance of 'C' as an attribute on an exception, e.g. 
raise ValueError(C()) in the second thread). 

BTW. I would be very uncomfortable if my patch were applied without 
review from someone that knows his way around the threading code.  

BTW2. I have a workaround for my initial issue (the crash in 
threads.py): if I add an atexit handler that performs some cleanup in 
PyObjC the problem goes away.
msg57866 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2007-11-27 08:20
> The _PyGILState_Fini function might cause user code to run as well, 
> it removes the thread-local variable that contains the PyThreadState 
> for threads, and that contains some Python objects that might contain 
> arbitrary values (such as the last exception value). 

No, _PyGILState_Fini does not invoke any python code; it only clears C
variables. The last exception value is deleted during the call to 
PyInterpreterState_Clear() (inside PyThreadState_Clear).
msg57877 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-11-27 18:39
> No, _PyGILState_Fini does not invoke any python code

You're right. It calls PyThread_delete_key().  I thought this would
delete entries from a dictionary (thereby potentially invoking Python
code via Py_DECREF()), but it doesn't -- it just frees some memory.

So I'm okay with swapping the order in 2.6 and 3.0.  I'm still not 100%
comfortable with doing this in 2.5, especially since Ronald found a
work-around for his immediate issue.
msg57966 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2007-11-29 23:36
Committed revision 59231 in trunk.
History
Date User Action Args
2007-11-29 23:36:29amaury.forgeotdarcsetstatus: open -> closed
resolution: fixed
messages: + msg57966
2007-11-27 18:39:41gvanrossumsetmessages: + msg57877
2007-11-27 08:20:27amaury.forgeotdarcsetmessages: + msg57866
2007-11-27 05:38:44ronaldoussorensetmessages: + msg57865
2007-11-26 22:03:10gvanrossumsetmessages: + msg57856
2007-11-23 23:34:30amaury.forgeotdarcsetfiles: + test_gilstate.patch
nosy: + amaury.forgeotdarc
messages: + msg57795
2007-11-22 10:52:09ronaldoussorensetfiles: + pygilstate.patch
messages: + msg57754
2007-11-09 05:23:27ronaldoussorensetmessages: + msg57297
2007-11-09 00:05:09gvanrossumsetnosy: + gvanrossum
messages: + msg57285
2007-11-07 22:05:52ronaldoussorencreate