Skip to content

Commit 590cebe

Browse files
vstinnervstinner
committed
Issue python#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 python#10915 and python#15751.
1 parent cb1c4c8 commit 590cebe

6 files changed

Lines changed: 24 additions & 39 deletions

File tree

‎Misc/NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ Release date: 2014-01-05
1010
Core and Builtins
1111
-----------------
1212

13+
- Issue #19787: PyThread_set_key_value() now always set the value. In Python
14+
3.3, the function did nothing if the key already exists (if the current value
15+
is a non-NULL pointer).
16+
1317
- Issue #14432: Remove the thread state field from the frame structure. Fix a
1418
crash when a generator is created in a C thread that is destroyed while the
1519
generator is still used. The issue was that a generator contains a frame, and

‎Modules/_tracemalloc.c‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -168,14 +168,11 @@ set_reentrant(int reentrant)
168168
assert(reentrant == 0 || reentrant == 1);
169169
if (reentrant) {
170170
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);
173172
}
174173
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);
179176
}
180177
}
181178

‎Python/pystate.c‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -723,18 +723,18 @@ _PyGILState_NoteThreadState(PyThreadState* tstate)
723723
724724
The only situation where you can legitimately have more than one
725725
thread state for an OS level thread is when there are multiple
726-
interpreters, when:
726+
interpreters.
727727
728-
a) You shouldn't really be using the PyGILState_ APIs anyway,
729-
and:
728+
You shouldn't really be using the PyGILState_ APIs anyway (see issues
729+
#10915 and #15751).
730730
731-
b) The slightly odd way PyThread_set_key_value works (see
732-
comments by its implementation) means that the first thread
733-
state created for that given OS level thread will "win",
734-
which seems reasonable behaviour.
731+
The first thread state created for that given OS level thread will
732+
"win", which seems reasonable behaviour.
735733
*/
736-
if (PyThread_set_key_value(autoTLSkey, (void *)tstate) < 0)
737-
Py_FatalError("Couldn't create autoTLSkey mapping");
734+
if (PyThread_get_key_value(autoTLSkey) == NULL) {
735+
if (PyThread_set_key_value(autoTLSkey, (void *)tstate) < 0)
736+
Py_FatalError("Couldn't create autoTLSkey mapping");
737+
}
738738

739739
/* PyGILState_Release must not try to delete this thread state. */
740740
tstate->gilstate_counter = 1;

‎Python/thread.c‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ static int nkeys = 0; /* PyThread_create_key() hands out nkeys+1 next */
205205
* segfaults. Now we lock the whole routine.
206206
*/
207207
static struct key *
208-
find_key(int key, void *value)
208+
find_key(int set_value, int key, void *value)
209209
{
210210
struct key *p, *prev_p;
211211
long id = PyThread_get_thread_ident();
@@ -215,8 +215,11 @@ find_key(int key, void *value)
215215
PyThread_acquire_lock(keymutex, 1);
216216
prev_p = NULL;
217217
for (p = keyhead; p != NULL; p = p->next) {
218-
if (p->id == id && p->key == key)
218+
if (p->id == id && p->key == key) {
219+
if (set_value)
220+
p->value = value;
219221
goto Done;
222+
}
220223
/* Sanity check. These states should never happen but if
221224
* they do we must abort. Otherwise we'll end up spinning in
222225
* in a tight loop with the lock held. A similar check is done
@@ -227,7 +230,7 @@ find_key(int key, void *value)
227230
if (p->next == keyhead)
228231
Py_FatalError("tls find_key: circular list(!)");
229232
}
230-
if (value == NULL) {
233+
if (!set_value && value == NULL) {
231234
assert(p == NULL);
232235
goto Done;
233236
}
@@ -279,19 +282,12 @@ PyThread_delete_key(int key)
279282
PyThread_release_lock(keymutex);
280283
}
281284

282-
/* Confusing: If the current thread has an association for key,
283-
* value is ignored, and 0 is returned. Else an attempt is made to create
284-
* an association of key to value for the current thread. 0 is returned
285-
* if that succeeds, but -1 is returned if there's not enough memory
286-
* to create the association. value must not be NULL.
287-
*/
288285
int
289286
PyThread_set_key_value(int key, void *value)
290287
{
291288
struct key *p;
292289

293-
assert(value != NULL);
294-
p = find_key(key, value);
290+
p = find_key(1, key, value);
295291
if (p == NULL)
296292
return -1;
297293
else
@@ -304,7 +300,7 @@ PyThread_set_key_value(int key, void *value)
304300
void *
305301
PyThread_get_key_value(int key)
306302
{
307-
struct key *p = find_key(key, NULL);
303+
struct key *p = find_key(0, key, NULL);
308304

309305
if (p == NULL)
310306
return NULL;

‎Python/thread_nt.h‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -389,20 +389,11 @@ PyThread_delete_key(int key)
389389
TlsFree(key);
390390
}
391391

392-
/* We must be careful to emulate the strange semantics implemented in thread.c,
393-
* where the value is only set if it hasn't been set before.
394-
*/
395392
int
396393
PyThread_set_key_value(int key, void *value)
397394
{
398395
BOOL ok;
399-
void *oldvalue;
400396

401-
assert(value != NULL);
402-
oldvalue = TlsGetValue(key);
403-
if (oldvalue != NULL)
404-
/* ignore value if already set */
405-
return 0;
406397
ok = TlsSetValue(key, value);
407398
if (!ok)
408399
return -1;

‎Python/thread_pthread.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -627,9 +627,6 @@ int
627627
PyThread_set_key_value(int key, void *value)
628628
{
629629
int fail;
630-
void *oldValue = pthread_getspecific(key);
631-
if (oldValue != NULL)
632-
return 0;
633630
fail = pthread_setspecific(key, value);
634631
return fail ? -1 : 0;
635632
}

0 commit comments

Comments
 (0)