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

[Gabriel Becedillas]

Gabriel Becedillas wrote:
> 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
>>
> 
> 

Can anybody tell me if the patch I suggested is ok ?
That will be to add the following code at the end of PyThreadState_Delete:

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

Thank you very much.

-- 


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