(original) (raw)

changeset: 87931:5d078b0bae75 parent: 87929:e78743e03c8f user: Victor Stinner victor.stinner@gmail.com date: Fri Dec 13 11:08:56 2013 +0100 files: Misc/NEWS Modules/_tracemalloc.c Python/pystate.c Python/thread.c Python/thread_nt.h Python/thread_pthread.h description: Issue #19787: PyThread_set_key_value() now always set the value In Python 3.3, PyThread_set_key_value() did nothing if the key already exists (if the current value is a non-NULL pointer). When _PyGILState_NoteThreadState() is called twice on the same thread with a different Python thread state, it still keeps the old Python thread state to keep the old behaviour. Replacing the Python thread state with the new state introduces new bugs: see issues #10915 and #15751. diff -r e78743e03c8f -r 5d078b0bae75 Misc/NEWS --- a/Misc/NEWS Fri Dec 13 12:08:55 2013 +0200 +++ b/Misc/NEWS Fri Dec 13 11:08:56 2013 +0100 @@ -10,6 +10,10 @@ Core and Builtins ----------------- +- Issue #19787: PyThread_set_key_value() now always set the value. In Python + 3.3, the function did nothing if the key already exists (if the current value + is a non-NULL pointer). + - Issue #14432: Remove the thread state field from the frame structure. Fix a crash when a generator is created in a C thread that is destroyed while the generator is still used. The issue was that a generator contains a frame, and diff -r e78743e03c8f -r 5d078b0bae75 Modules/_tracemalloc.c --- a/Modules/_tracemalloc.c Fri Dec 13 12:08:55 2013 +0200 +++ b/Modules/_tracemalloc.c Fri Dec 13 11:08:56 2013 +0100 @@ -168,14 +168,11 @@ assert(reentrant == 0 || reentrant == 1); if (reentrant) { assert(PyThread_get_key_value(tracemalloc_reentrant_key) == NULL); - PyThread_set_key_value(tracemalloc_reentrant_key, - REENTRANT); + PyThread_set_key_value(tracemalloc_reentrant_key, REENTRANT); } else { - /* FIXME: PyThread_set_key_value() cannot be used to set the flag - to zero, because it does nothing if the variable has already - a value set. */ - PyThread_delete_key_value(tracemalloc_reentrant_key); + assert(PyThread_get_key_value(tracemalloc_reentrant_key) == REENTRANT); + PyThread_set_key_value(tracemalloc_reentrant_key, NULL); } } diff -r e78743e03c8f -r 5d078b0bae75 Python/pystate.c --- a/Python/pystate.c Fri Dec 13 12:08:55 2013 +0200 +++ b/Python/pystate.c Fri Dec 13 11:08:56 2013 +0100 @@ -723,18 +723,18 @@ The only situation where you can legitimately have more than one thread state for an OS level thread is when there are multiple - interpreters, when: + interpreters. - a) You shouldn't really be using the PyGILState_ APIs anyway, - and: + You shouldn't really be using the PyGILState_ APIs anyway (see issues + #10915 and #15751). - b) The slightly odd way PyThread_set_key_value works (see - comments by its implementation) means that the first thread - state created for that given OS level thread will "win", - which seems reasonable behaviour. + The first thread state created for that given OS level thread will + "win", which seems reasonable behaviour. */ - if (PyThread_set_key_value(autoTLSkey, (void *)tstate) < 0) - Py_FatalError("Couldn't create autoTLSkey mapping"); + if (PyThread_get_key_value(autoTLSkey) == NULL) { + if (PyThread_set_key_value(autoTLSkey, (void *)tstate) < 0) + Py_FatalError("Couldn't create autoTLSkey mapping"); + } /* PyGILState_Release must not try to delete this thread state. */ tstate->gilstate_counter = 1; diff -r e78743e03c8f -r 5d078b0bae75 Python/thread.c --- a/Python/thread.c Fri Dec 13 12:08:55 2013 +0200 +++ b/Python/thread.c Fri Dec 13 11:08:56 2013 +0100 @@ -205,7 +205,7 @@ * segfaults. Now we lock the whole routine. */ static struct key * -find_key(int key, void *value) +find_key(int set_value, int key, void *value) { struct key *p, *prev_p; long id = PyThread_get_thread_ident(); @@ -215,8 +215,11 @@ PyThread_acquire_lock(keymutex, 1); prev_p = NULL; for (p = keyhead; p != NULL; p = p->next) { - if (p->id == id && p->key == key) + if (p->id == id && p->key == key) { + if (set_value) + p->value = value; goto Done; + } /* Sanity check. These states should never happen but if * they do we must abort. Otherwise we'll end up spinning in * in a tight loop with the lock held. A similar check is done @@ -227,7 +230,7 @@ if (p->next == keyhead) Py_FatalError("tls find_key: circular list(!)"); } - if (value == NULL) { + if (!set_value && value == NULL) { assert(p == NULL); goto Done; } @@ -279,19 +282,12 @@ PyThread_release_lock(keymutex); } -/* Confusing: If the current thread has an association for key, - * value is ignored, and 0 is returned. Else an attempt is made to create - * an association of key to value for the current thread. 0 is returned - * if that succeeds, but -1 is returned if there's not enough memory - * to create the association. value must not be NULL. - */ int PyThread_set_key_value(int key, void *value) { struct key *p; - assert(value != NULL); - p = find_key(key, value); + p = find_key(1, key, value); if (p == NULL) return -1; else @@ -304,7 +300,7 @@ void * PyThread_get_key_value(int key) { - struct key *p = find_key(key, NULL); + struct key *p = find_key(0, key, NULL); if (p == NULL) return NULL; diff -r e78743e03c8f -r 5d078b0bae75 Python/thread_nt.h --- a/Python/thread_nt.h Fri Dec 13 12:08:55 2013 +0200 +++ b/Python/thread_nt.h Fri Dec 13 11:08:56 2013 +0100 @@ -389,20 +389,11 @@ TlsFree(key); } -/* We must be careful to emulate the strange semantics implemented in thread.c, - * where the value is only set if it hasn't been set before. - */ int PyThread_set_key_value(int key, void *value) { BOOL ok; - void *oldvalue; - assert(value != NULL); - oldvalue = TlsGetValue(key); - if (oldvalue != NULL) - /* ignore value if already set */ - return 0; ok = TlsSetValue(key, value); if (!ok) return -1; diff -r e78743e03c8f -r 5d078b0bae75 Python/thread_pthread.h --- a/Python/thread_pthread.h Fri Dec 13 12:08:55 2013 +0200 +++ b/Python/thread_pthread.h Fri Dec 13 11:08:56 2013 +0100 @@ -627,9 +627,6 @@ PyThread_set_key_value(int key, void *value) { int fail; - void *oldValue = pthread_getspecific(key); - if (oldValue != NULL) - return 0; fail = pthread_setspecific(key, value); return fail ? -1 : 0; } /victor.stinner@gmail.com