[Python-Dev] pystate.c changes for Python 2.4.2

[Gabriel Becedillas]

Michael Hudson wrote:
> Gabriel Becedillas <gabriel.becedillas at corest.com> writes:
> 
> 
>>Hi,
>>At the company I work for, we've embedded Python in C++ application we 
>>develop. Since our upgrade to Python 2.4.2 from 2.4.1 we started hitting 
>>Py_FatalError("Invalid thread state for this thread") when using debug 
>>builds.
>>We use both multiple interpreters and thread states.
> 
> 
> I know you've said this to me in email already, but I have no idea how
> you managed to get this to work with 2.4.1 :)
> 
> 
>>I think the problem is that PyThreadState_Delete (which is used when I
>>destroy thread states and interpreters) is not making the same clean up
>>that PyThreadState_DeleteCurrent is doing (which is used when we create 
>>threads from python code). If a thread id is reused (obviously not 
>>between two running threads), and the previous thread used 
>>PyThreadState_Delete instead of PyThreadState_DeleteCurrent, then an old 
>>and invalid value for autoTLSkey is still stored, and that is
>>triggering the Py_FatalError when a call to PyThreadState_Swap is made.
>>If I add this code at the end of PyThreadState_Delete:
>>
>>if (autoTLSkey && PyThread_get_key_value(autoTLSkey) == tstate)
>>	PyThread_delete_key_value(autoTLSkey);
>>
>>then everything works fine.
> 
> 
> I think I begin to understand the problem... but this is still
> fragile: it relies on the fact that you delete a thread state from the
> OS level thread that created it, but while a thread belonging to a
> different *interpreter state* has the GIL (or at least: the
> interpreter state of the thread state being deleted *doesn't* hold the
> GIL).  Can you guarantee that?

Mmm... it doesn't have to do with a race condition or something. Let me 
try to show you with an example (keep in mind that this relies on the 
fact that at some moment the operating system gives you a thread with 
the same id of another thread that allready finished executing):

// Initialize python.
Py_Initialize();
PyEval_InitThreads();
PyThreadState* p_main =  PyThreadState_Swap(NULL);
PyEval_ReleaseLock();

/*
Asume this block is executed in a separate thread, and that has been 
asigned thread id 10.
It doesn't matter what the main thread (the thread that initialized 
Python) is doing. Lets assume its waiting for this one to finish.
*/
{
	PyEval_AcquireLock();
	PyThreadState_Swap(p_main);
	PyThreadState* p_child = PyThreadState_New(p_main->interp);

	PyThreadState_Swap(p_child);
	PyRun_SimpleString("print 'mi vieja mula ya no es lo que era'");

	PyThreadState_Clear(p_child);
	PyThreadState_Swap(NULL);
	PyThreadState_Delete(p_child);
	PyEval_ReleaseLock();
	// The os level thread exits here.
}
/*
When this thread state gets deleted (using PyThreadState_Delete), the 
autoTLSkey stored for thread id number 10 is not removed, because 
PyThreadState_Delete doesn't have the necesary cleanup code (check 
pystate.c):

if (autoTLSkey && PyThread_get_key_value(autoTLSkey) == tstate)
	PyThread_delete_key_value(autoTLSkey);

PyThreadState_DeleteCurrent behaves correctly because it does have this 
code.
I think that this code should be added to tstate_delete_common (I've 
done this and everything worked just fine).
If a new thread executes the same code, and that thread is assigned the 
same thread id, you get the Py_FatalError I'm talking about.
A workaround for this would be to use PyThreadState_DeleteCurrent 
instead of PyThreadState_Delete.

If you use the following code instead of the one above, the you have no 
way out to the Py_FatalError:
*/
{
	PyEval_AcquireLock();
	PyThreadState* ret = Py_NewInterpreter();

	PyRun_SimpleString("print 'mi vieja mula ya no es lo que era'");

	Py_EndInterpreter(ret);
	PyThreadState_Swap(NULL);
	PyEval_ReleaseLock();
}
/*
When this interpreter gets deleted (using Py_EndInterpreter), its thread 
state gets deleted using PyThreadState_Delete, and you are back to the 
beginning of the problem.
*/

I hope this helps to clarify the problem.
Thanks a lot and have a nice day.

> It seems to me that to be robust, you need a way of saying "delete the
> thread local value of autoTLSKey from thread $FOO".  But this is all
> very confusing...
> 
> 
>>Could you please confirm if this is a bug ?
> 
> 
> Yes, I think it's a bug.
> 
> Cheers,
> mwh
> 


-- 


Gabriel Becedillas
Developer
CORE SECURITY TECHNOLOGIES

Florida 141 - 2º cuerpo - 7º piso
C1005AAC Buenos Aires - Argentina
Tel/Fax: (54 11) 5032-CORE (2673)
http://www.corest.com