The following patch adds native TLS implementation for pthreads, avoiding the relatively slow and clunky default tls implemented in thread.c

The patch looks good.
Apart from PyGILState_Ensure(), are there other parts of the code that use these functions?

Just tried to apply the patch:

• test_capi now freezes in auto-thread-state
• test_threading now freezes in test_finalize_runnning_thread

The code may need a little bit of extra work: The pthreads machine that I have to work with is a PS3 :).
And, despite the documentation not saying, I suspect that pthread_getspecific() actually does mess with "errno", making it necessary to preserve "errno" when calling PyThread_get_key_value(). (The reason for this is documented in thread_nt.h)

I'll do some tests and let you know.

"errno" is preserved by PyEval_RestoreThread(), so this isn't the issue.
What you are probably seeing is latent race conditions in the test suite, made apparent by a non-locking TLS implementation. The test suite isn't free from those, see for example bpo-8799

Hm, both the test you mention are using the (non-recursive) lock to synchronize threads. I can't see anything wrong there.

Could you please try to replace the cod in pthread_getspecific() with this:

int err = errno
void *result = pthread_getspecific(key);
errno = err;
return result;

If this fixes those cases, then there is code somewhere that relies on errno being maintained across these calls.

The problem turns out to be in pystate.c (my system returns 0 as a valid key number). See issue bpo-9797.

Ah, good to hear.

The ifdef should go; pthreads always support TLS (since XPG5, 1997).

I left the ifdef in for a quick and easy way to disable this code for those interested, but I'm happy to remove it if it makes for greater synergy.

Added a new patch with #ifdef remvoved, for greater harmony.

It seems we need to clarify the return value of PyThread_create_key. The patch returns 0 in case of failure, which is clearly wrong as 0 is also a valid key.

I think it's safe to return -1: TlsAlloc returns TLS_OUT_OF_INDEXES, which is 0xFFFFFFFF, i.e. -1 on a 32-bit system.

I've changed the function as you suggest, although there are in fact no failure detection semantics defined for PyThread_create_key(). See e.g. thread.c:294

/* Return a new key.  This must be called before any other functions in
 * this family, and callers must arrange to serialize calls to this
 * function.  No violations are detected.
 */
int
PyThread_create_key(void)

Am 13.09.2010 10:00, schrieb Kristján Valur Jónsson:

Kristján Valur Jónsson <kristjan@ccpgames.com> added the comment:

I've changed the function as you suggest, although there are in fact no failure detection semantics defined for PyThread_create_key().

Right. That's why I'm thinking that we should introduce some. TLS
creation *can* fail, and it is unfortunate that the original
introduction of this API forgot to consider that. Your patch did
(de facto) introduce an error return code (originally 0, now -1).
So it would be good if this was documented, and the NT version adjusted.

In principle, this is really difficult to get right. AFAICT,
pthread_key_t doesn't even have to be an integral type, and even
if it is, it may well become -1. However, we can probably worry
about this when a system comes along where this implementation
breaks.

Right. That's why I'm thinking that we should introduce some. TLS
creation *can* fail, and it is unfortunate that the original
introduction of this API forgot to consider that. Your patch did
(de facto) introduce an error return code (originally 0, now -1).

Perhaps we can simply call Py_FatalError() instead? There's not much we
can do when such a failure occurs anyway.

Perhaps we can simply call Py_FatalError() instead? There's not much we
can do when such a failure occurs anyway.

Sounds fine as well. Python's own usage definitely shouldn't fail. If
extension modules use this in an aggressive manner, they may actually
exhaust TLS keys (at least on Windows, where old versions had only
64 slots). Having the interpreter fail then sounds reasonable.

Ok, here is a patch. key creation returns -1 on error, and the caller can detect this and raise a fatal error.

Patch looks good to me.

Committed as revision 84914