New thread death in test_bsddb3
test_bsddb3.py fails quickly today under a debug build, with a thread state error, on Win2K, every time. Linux?
I assume this is a bad interaction between Mark Hammond's new auto-thread-state code and _bsddb.c's custom thread-manipulation macros:
C:\Code\python\PCbuild>python_d ../lib/test/test_bsddb3.py -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= Sleepycat Software: Berkeley DB 4.1.25: (December 19, 2002) bsddb.db.version(): (4, 1, 25) bsddb.db.__version__: 4.1.5 bsddb.db.cvsid: $Id: _bsddb.c,v 1.11 2003/03/31 19:51:29 bwarsaw Exp $ python version: 2.3a2+ (#39, Apr 22 2003, 10:48:23) [MSC v.1200 32 bit (I ntel)] -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= Fatal Python error: Invalid thread state for this thread
C:\Code\python\PCbuild>
It's dying in _db_associateCallback, here:
static int _db_associateCallback(DB* db, const DBT* priKey, const DBT* priData, DBT* secKey) { int retval = DB_DONOTINDEX; DBObject* secondaryDB = (DBObject*)db->app_private; PyObject* callback = secondaryDB->associateCallback; int type = secondaryDB->primaryDBType; PyObject* key; PyObject* data; PyObject* args; PyObject* result;
if (callback != NULL) { MYDB_BEGIN_BLOCK_THREADS; ************ HERE *************
The macro is defined like so:
#define MYDB_BEGIN_BLOCK_THREADS { \ PyThreadState* prevState; \ PyThreadState* newState; \ PyEval_AcquireLock(); \ newState = PyThreadState_New(_db_interpreterState); \ prevState = PyThreadState_Swap(newState);
PyThreadState_Swap is complaining here:
#if defined(Py_DEBUG) if (new) { PyThreadState *check = PyGILState_GetThisThreadState(); if (check && check != new) Py_FatalError("Invalid thread state for this thread"); } #endif
This is a new check, I believe it's an intentional check, and I doubt _bsddb.c *should* pass it as-is.
test_bsddb3.py fails quickly today under a debug build, with a thread state error, on Win2K, every time. Linux?
I assume this is a bad interaction between Mark Hammond's new auto-thread-state code and _bsddb.c's custom thread-manipulation macros:
Yes, this is my fault. The assertion is detecting the fact that bsddb is creating and using its own interpreter/thread states than using the thread-state already seen for that thread.
As Tim says, the assertion is new, but the check it makes is valid. I believe that removing the assertion would allow it to work, but the right thing to do is fix bsddb to use the new PyGILState_ API, and therefore share the threadstate with the rest of Python.
I will do this very shortly (ie, within a couple of hours)
Mark.
On Tue, 2003-04-22 at 18:27, Mark Hammond wrote:
Yes, this is my fault. The assertion is detecting the fact that bsddb is creating and using its own interpreter/thread states than using the thread-state already seen for that thread.
As Tim says, the assertion is new, but the check it makes is valid. I believe that removing the assertion would allow it to work, but the right thing to do is fix bsddb to use the new PyGILState_ API, and therefore share the threadstate with the rest of Python.
I will do this very shortly (ie, within a couple of hours)
Thanks for taking care of this Mark! Yes, as PEP 291 states, bsddb.c has to be compatible with Python 2.1. At some point we may want to re-evaluate that, but for now, if it's easy to do, we should keep compatibility.
-Barry
test_bsddb3.py fails quickly today under a debug build, with a thread state error, on Win2K, every time. Linux?
Actually, some guidance would be nice here.
Is this code (_bsddb.c) ever expected to again build under pre-trunk versions of Python, or can I remove the old thread-state management code?
ie, should my changes be or the style:
#if defined(NEW_PYGILSTATE_API_EXISTS) // new 1 line of code #else // existing many lines of code #endif
Or just stick with the new code?
Nothing-is-finished-until-there-is-nothing-left-to-remove ly,
Mark.
[Mark Hammond]
Actually, some guidance would be nice here.
It's easy this time. BTW, I agree your new check is the right thing to do! If another case like this pops up, though, we/you should probably add a section to the PEP explaining what to do about it.
Is this code (_bsddb.c) ever expected to again build under pre-trunk versions of Python, or can I remove the old thread-state management code?
The former: the pybsddb project still exists and is used with older versions of Python. Barry mumbled something today at the office about wanting to keep the C code in synch.
ie, should my changes be or the style:
#if defined(NEW_PYGILSTATE_API_EXISTS) // new 1 line of code #else // existing many lines of code #endif
Yes, that would be great.
Tim Peters tim.one@comcast.net writes:
[Mark Hammond]
Actually, some guidance would be nice here.
It's easy this time. BTW, I agree your new check is the right thing to do! If another case like this pops up, though, we/you should probably add a section to the PEP explaining what to do about it.
ctypes ;-) is another case (and more cases will pop up as soon as the beta is released, and people try their extensions under it).
I agree it is easy to fix, but usually when Python crashes with an invalid thread state I'm very anxious at first.
So is the policy now that it is no longer *allowed* to create another thread state, while in previous versions there wasn't any choice, because there existed no way to get the existing one?
IMO a fatal error is very harsh, especially there's no problem to continue execution - excactly what happens in a release build.
Not that I am misunderstood: I very much appreciate the work Mark has done, and look forward to use it to it's fullest extent.
Thomas
So is the policy now that it is no longer *allowed* to create another thread state, while in previous versions there wasn't any choice, because there existed no way to get the existing one?
Only not allowed under debug builds <wink>.
I would be more than happy to have this code print a warning, or take some alternative action - but I would hate to see the message dropped.
Would a PyErr_Warning call be more appropriate? The only issue here is that literally *thousands* may be generated.
Mark.
[Thomas Heller]
... So is the policy now that it is no longer *allowed* to create another thread state, while in previous versions there wasn't any choice, because there existed no way to get the existing one?
You can still create all the thread states you like; the new check is in PyThreadState_Swap(), not in PyThreadState_New().
There was always a choice, but previously Python provided no *help* in keeping track of whether a thread already had a thread state associated with it. That didn't stop careful apps from providing their own mechanisms to do so.
About policy, yes, it appears to be so now, else Mark wouldn't be raising a fatal error <wink>. I view it as having always been the policy (from a good-faith reading of the previous code), just a policy that was too expensive for Python to enforce. There are many policies like that, such as not passing goofy arguments to macros, and not letting references leak. Python doesn't currently enforce them because it's currently too expensive to enforce them. Over time that can change.
IMO a fatal error is very harsh, especially there's no problem to continue execution - excactly what happens in a release build.
There may or may not be a problem with continued execution -- if you've associated more than one living thread state with a thread, your app may very well be fatally confused in a way that's very difficult to diagnose without this error.
Clearly, I like having fatal errors for dubious things in debug builds. Debug builds are supposed to help you debug. If the fatal error here drives you insane, and you don't want to repair the app code, you're welcome to change
#if defined(Py_DEBUG)
to
#if 0
in your debug build.
Not that I am misunderstood: I very much appreciate the work Mark has done, and look forward to use it to it's fullest extent.
In what way is this error a genuine burden to you? The only time I've seen it trigger is in the Berkeley database wrapper, where it pointed out a fine opportunity to simplify some obscure hand-rolled callback tomfoolery -- and pointed out that the thread in question did in fact already have a thread state. Whether that was correct in all cases is something I don't know -- and don't have to worry about anymore, since the new code reuses the thread state the thread already had. The lack of errors in a debug run now assures me that's in good shape now.
"Tim Peters" tim_one@email.msn.com writes:
[Thomas Heller]
... So is the policy now that it is no longer *allowed* to create another thread state, while in previous versions there wasn't any choice, because there existed no way to get the existing one?
You can still create all the thread states you like; the new check is in PyThreadState_Swap(), not in PyThreadState_New().
So you can create them, but are not allowed to use them? (Should there be a smiley here, or not, I'm not sure)
There was always a choice, but previously Python provided no *help* in keeping track of whether a thread already had a thread state associated with it. That didn't stop careful apps from providing their own mechanisms to do so.
About policy, yes, it appears to be so now, else Mark wouldn't be raising a fatal error <wink>. I view it as having always been the policy (from a good-faith reading of the previous code), just a policy that was too expensive for Python to enforce. There are many policies like that, such as not passing goofy arguments to macros, and not letting references leak. Python doesn't currently enforce them because it's currently too expensive to enforce them. Over time that can change.
I'm confused: what *is* the policy now? And: Has the policy *changed*, or was it simply not checked before?
Since I don't know the policy, I can only guess if the fatal error is appropriate or not.
If it is, there should be a 'recipe' what to do (even if it is 'use the approach outlined in PEP311').
If it is not, the error should be removed (IMO).
Clearly, I like having fatal errors for dubious things in debug builds. Debug builds are supposed to help you debug. If the fatal error here drives you insane, and you don't want to repair the app code,
No, not at all.
Thanks,
Thomas
[Thomas Heller]
... So is the policy now that it is no longer *allowed* to create another thread state, while in previous versions there wasn't any choice, because there existed no way to get the existing one?
[Tim]
You can still create all the thread states you like; the new check is in PyThreadState_Swap(), not in PyThreadState_New().
[Thomas]
So you can create them,
Yes.
but are not allowed to use them?
Currently, no more than one at a time per thread. The API doesn't appear to preclude using multiple thread states with a single thread if the right dances are performed. Offhand I don't know why someone would want to, but people want to do a lot of silly things <wink>.
(Should there be a smiley here, or not, I'm not sure)
No.
... I'm confused: what *is* the policy now? And: Has the policy *changed*, or was it simply not checked before?
I already gave you my best guesses about those (no, yes).
Since I don't know the policy, I can only guess if the fatal error is appropriate or not.
Ditto (yes).
If it is, there should be a 'recipe' what to do (even if it is 'use the approach outlined in PEP311').
Additions to NEWS and the PEP would be fine by me.
If it is not, the error should be removed (IMO).
Sure.
Tim Peters wrote:
Currently, no more than one at a time per thread. The API doesn't appear to preclude using multiple thread states with a single thread if the right dances are performed. Offhand I don't know why someone would want to, but people want to do a lot of silly things <wink>.
There are many good reasons; here is one scenario:
Application A calls embedded Python. It creates thread state T1 to do so. Python calls library L1, which releases GIL. L1 calls L2. L2 calls back into Python. To do so, it allocates a new thread state, and acquires the GIL. All in one thread.
L2 has no idea that A has already allocated a thread state for this thread. With the new API, L2 does not need any longer to create a thread state. However, in older Python releases, this was necessary, so libraries do such things. It is unfortunate that these libraries now break, and I wish the new API would not be enforced so strictly yet.
I already gave you my best guesses about those (no, yes).
I think your guess is wrong: In the past, it was often *necessary* to have multiple thread states allocated for a single thread. There was simply no other option. So it can't be that this was not allowed.
Regards, Martin
[Martin v. Löwis]
There are many good reasons; here is one scenario:
Application A calls embedded Python. It creates thread state T1 to do so. Python calls library L1, which releases GIL. L1 calls L2. L2 calls back into Python. To do so, it allocates a new thread state, and acquires the GIL. All in one thread.
L2 has no idea that A has already allocated a thread state for this thread. With the new API, L2 does not need any longer to create a thread state. However, in older Python releases, this was necessary, so libraries do such things.
I understand that some people did this (we've bumped into two so far, right?), but don't agree it was necessary: the thrust of Mark's new code is to make this easy to do in a uniform way, but people could (and did) build their own layers of TLS-based Python wrappers before this (Mark is one of them; a former employer of mine is another). AFAIK, though, these were cases where multiple libraries agreed to cooperate. I don't really care anymore, since there's a standard way to do this now.
It is unfortunate that these libraries now break, and I wish the new API would not be enforced so strictly yet.
If it were enforced in a release build I'd agree, but it isn't -- a release build enforces nothing new here, and I want to be punched in the groin when a debug build spots dubious practice.
I already gave you my best guesses about those (no, yes).
I think your guess is wrong: In the past, it was often *necessary* to have multiple thread states allocated for a single thread. There was simply no other option. So it can't be that this was not allowed.
It's a new world now -- let's get on with it. Fighting for the right to retain lame code (judged by current stds, whether or not it was lame before) isn't a cause I'll sign up for, and especially not when it's in an extremely error-prone area of the C API, and certainly not when it's so easy to repair too. But if you're determined to let slop slide in the debug build, check in a change to stop the warning -- it's not important enough to me to keep arguing about it. I don't think you'd be doing anyone a real favor, and I'll leave it at that.
participants (7)
-
"Martin v. Löwis" -
Barry Warsaw -
Mark Hammond -
Thomas Heller -
Tim Peters -
Tim Peters -
Tim Peters