From 590cebe391fb2e199afe9b20ff67e360116a1266 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Fri, 13 Dec 2013 11:08:56 +0100 Subject: [PATCH] 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. --- Misc/NEWS | 4 ++++ Modules/_tracemalloc.c | 9 +++------ Python/pystate.c | 18 +++++++++--------- Python/thread.c | 20 ++++++++------------ Python/thread_nt.h | 9 --------- Python/thread_pthread.h | 3 --- 6 files changed, 24 insertions(+), 39 deletions(-) diff --git a/Misc/NEWS b/Misc/NEWS index 11e417a6dbb..eca89cc7a54 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -10,6 +10,10 @@ Release date: 2014-01-05 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 --git a/Modules/_tracemalloc.c b/Modules/_tracemalloc.c index b39e950c273..95b05d61c55 100644 --- a/Modules/_tracemalloc.c +++ b/Modules/_tracemalloc.c @@ -168,14 +168,11 @@ set_reentrant(int reentrant) 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 --git a/Python/pystate.c b/Python/pystate.c index a56e3089694..19fceb7127a 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -723,18 +723,18 @@ _PyGILState_NoteThreadState(PyThreadState* tstate) 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 --git a/Python/thread.c b/Python/thread.c index 8540942e28a..d1cb0e6f9d5 100644 --- a/Python/thread.c +++ b/Python/thread.c @@ -205,7 +205,7 @@ static int nkeys = 0; /* PyThread_create_key() hands out nkeys+1 next */ * 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 @@ find_key(int key, void *value) 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 @@ find_key(int key, void *value) 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_delete_key(int key) 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 @@ PyThread_set_key_value(int key, void *value) 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 --git a/Python/thread_nt.h b/Python/thread_nt.h index ab5a08168f7..ee2079fc613 100644 --- a/Python/thread_nt.h +++ b/Python/thread_nt.h @@ -389,20 +389,11 @@ PyThread_delete_key(int key) 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 --git a/Python/thread_pthread.h b/Python/thread_pthread.h index 20f85358968..d9f7c76f2ab 100644 --- a/Python/thread_pthread.h +++ b/Python/thread_pthread.h @@ -627,9 +627,6 @@ int 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; }