Issue python#19787: PyThread_set_key_value() now always set the value

vstinner · vstinner · commit 590cebe391fb · 2013-12-13T11:08:56.000+01:00
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 python#10915 and python#15751.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
 }

Original file line number	Diff line number	Diff line change
`@@ -168,14 +168,11 @@ set_reentrant(int reentrant)`
`168`	`168`	`assert(reentrant == 0 \|\| reentrant == 1);`
`169`	`169`	`if (reentrant) {`
`170`	`170`	`assert(PyThread_get_key_value(tracemalloc_reentrant_key) == NULL);`
`171`		`- PyThread_set_key_value(tracemalloc_reentrant_key,`
`172`		`- REENTRANT);`
	`171`	`+ PyThread_set_key_value(tracemalloc_reentrant_key, REENTRANT);`
`173`	`172`	`}`
`174`	`173`	`else {`
`175`		`- /* FIXME: PyThread_set_key_value() cannot be used to set the flag`
`176`		`- to zero, because it does nothing if the variable has already`
`177`		`- a value set. */`
`178`		`- PyThread_delete_key_value(tracemalloc_reentrant_key);`
	`174`	`+ assert(PyThread_get_key_value(tracemalloc_reentrant_key) == REENTRANT);`
	`175`	`+ PyThread_set_key_value(tracemalloc_reentrant_key, NULL);`
`179`	`176`	`}`
`180`	`177`	`}`
`181`	`178`
Original file line number	Diff line number	Diff line change
`@@ -627,9 +627,6 @@ int`
`627`	`627`	`PyThread_set_key_value(int key, void *value)`
`628`	`628`	`{`
`629`	`629`	`int fail;`
`630`		`- void *oldValue = pthread_getspecific(key);`
`631`		`- if (oldValue != NULL)`
`632`		`- return 0;`
`633`	`630`	`fail = pthread_setspecific(key, value);`
`634`	`631`	`return fail ? -1 : 0;`
`635`	`632`	`}`