Michael Hudson wrote:

Gabriel Becedillas <gabriel.becedillas@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

[Gabriel Becedillas]

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);

It needs a little work, but I think it should be fine, and people tend to listen to me on issues like this ;-) The reason it can't work as-is is shallow: autoTLSkey doesn't exist unless Python is compiled with WITH_THREAD defined. So you need to wrap that inside an "#ifdef WITH_THREAD" block; or add it similarly to tstate_delete_common(), and remove it from PyThreadState_DeleteCurrent(). The primary reason people find this so confusing is because it is :-). In the dim mists of history, neither multiple interpreters nor PyThreadState_DeleteCurrent() existed. While multiple interpreters do exist now, they're poorly understood, and the PEP that introduced the GIL state API explicitly disavowed any responsibility for the new API working at all with multiple interpreters: http://www.python.org/peps/pep-0311.html This proposal identifies a solution for extension authors with complex multi-threaded requirements, but that only require a single "PyInterpreterState". There is no attempt to cater for extensions that require multiple interpreter states. At the time of writing, no extension has been identified that requires multiple PyInterpreterStates, and indeed it is not clear if that facility works correctly in Python itself. In a parallel development, thread races during Python shutdown were discovered that could lead to segfaults, and PyThreadState_DeleteCurrent() was introduced to repair those. As a result, it so happens that core Python never uses the original PyThreadState_Delete() anymore, except when Py_NewInterpreter() has to throw away the brand new thread state it created because it turns out it can't create a new interpreter. Since core Python never calls Py_NewInterpreter() or PyThreadState_Delete(), you're in an intersection of areas no current Python developer even thinks about, let alone tests. But by the same token, _because_ it's unused, there's nothing you can do to PyThreadState_Delete() that can hurt core Python either. That's why people should be more encouraging here ;-) It certainly makes sense to me that if a thread state is going away, any record of it in the auto-GIL-state machinery must also go away.

Thank you very much.

Thank you! If you put a patch on SourceForge, I'll probably be happy to check it in.

[Tim]

...

... As a result, it so happens that core Python never uses the original PyThreadState_Delete() anymore, except when Py_NewInterpreter() has to throw away the brand new thread state it created because it turns out it can't create a new interpreter.

[Michael]

Um, PyThreadState_Delete() is called from zapthreads() is called from PyInterpreterState_Delete() is called from Py_Finalize()... so I don't entirely believe you here :)

That's only because I was wrong ;-) Thanks! That use was in fact half the cause of Python's segfaulting shutdown races, so it was exceptionally stupid of me to miss that one.

...

Since core Python never calls Py_NewInterpreter() or PyThreadState_Delete(), you're in an intersection of areas no current Python developer even thinks about, let alone tests. But by the same token, _because_ it's unused, there's nothing you can do to PyThreadState_Delete() that can hurt core Python either. That's why people should be more encouraging here ;-) It certainly makes sense to me that if a thread state is going away, any record of it in the auto-GIL-state machinery must also go away.

I guess if the patch fixes the original problem, it should go in -- my uneasiness stems not from worrying that it might do harm, but rather from the fact that I think it's incomplete. Maybe I just haven't thought it through enough.

Oh, I don't know whether it's "complete". I don't really understand multiple interpreters, have no idea why the OP is calling PyThreadState_Delete(), and fully expect that nobody is going to add tests to Python to check any of this. But Gabriel showed real skill at tracking down problems in this area, so if he creates new problems for himself by doing this (who else plays in this intersection? nobody I know of), I'm sure he'll figure them out ;-) For example, it seems possible that he'll hit the same kinds of segfaulting shutdown races the Python core used to suffer. If a thread T releases the GIL, and goes on to delete its own thread state via PyThreadState_Delete(T's_thread_state), that _is_ a race with Python shutdown's zapthreads trying to delete the same thing. That's why PyThreadState_DeleteCurrent(void) was introduced, and any call to PyThreadState_Delete() is suspect on this count.

Also, every time I look at pystate.c, I think of things that could be done differently or better -- for example, I think it would be sane and possibly even sensible to only call PyThread_set_key_value() in _PyGILState_NoteThreadState() if "tstate->interp == autoInterpreterState".

I joined Mark Hammond in ignoring the possibility of multiple interpreters when this stuff went in. The best thing to do with untested gimmicks without an audience willing to work on them is to remove them. If you want to "rehabilitate" multiple interpreters here, the best first step would be to write an addendum to PEP 311 documenting an intended design. The GIL state API is documented well enough on its own, but the _intended_ semantics when mixing threads with multiple interpreters is clear as mud. PEP 311 asked "but who cares?", and nobody answered.