The tracemalloc module uses PyThread_set_key_value() to store an flag in the Thread Local Storage. The problem is that it is not possible to call the function twice with two different values. If PyThread_set_key_value() is called with a non-NULL pointer, the next calls do nothing.

Python should expose a new function which would always call TlsSetValue() / pthread_setspecific() with the input value with no extra check on the input value.

Extract of Modules/_tracemalloc.c:

static void
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);
    }
    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);
    }
}

Oh, here is in interesting commit:
---
changeset:   33705:891042c94aed
branch:      legacy-trunk
user:        Tim Peters <tim.peters@gmail.com>
date:        Sat Oct 09 22:33:09 2004 +0000
files:       Python/thread.c
description:
Document the results of painful reverse-engineering of the "portable TLS"
code.

PyThread_set_key_value():  It's clear that this code assumes the passed-in
value isn't NULL, so document that it must not be, and assert that it
isn't.  It remains unclear whether existing callers want the odd semantics
actually implemented by this function.
---

Here is a patch adding a _PyThread_set_key_value() function which behaves as I expected.

It looks like PyThread_set_key_value() got its strange behaviour from the find_key() of Python/thread.c.

Don't hesistate if you have a better suggestion for the name if the new function :-)

Another option is to modify directly the existing function, but it might break applications relying on the current (weird) behaviour.

I ran test_tracemalloc on Linux and Windows and the test passed successfully.

It should probably be better to split the patch in two parts if the idea of changing Python/thread* files is accepted. (But initially, the issue comes from the tracemalloc module.)

set_reentrant() of tracemalloc.c:

+        assert(PyThread_get_key_value(tracemalloc_reentrant_key) == REENTRANT);

I also added a new assertion to ensure that set_reentrant(0) was not called twice. It was a suggesed of Jim Jewett.

This is unrelated to this issue and should be commited separatly.

Calling it _PyThread_set_key_value is prone to confusion.
Ideally we would fix PyThread_set_key_value's behaviour. Are there users of the function outside from CPython?

2013/11/28 Antoine Pitrou <report@bugs.python.org>:
> Calling it _PyThread_set_key_value is prone to confusion.
> Ideally we would fix PyThread_set_key_value's behaviour. Are there users of the function outside from CPython?

PyThread_set_key_value() is defined and used in CPython, and defined
in the cpyext module of PyPy.

I found two usages of the function:
"pygobject2:/pygobject-2.28.6/glib/pygmainloop.c"
and"pytools:/Servicing/1.1/Release/Product/Python/PyDebugAttach/PyDebugAttach.cpp"
http://searchcode.com/codesearch/view/21308375
http://searchcode.com/codesearch/view/10353970

PyThread_delete_key_value() is always called before
PyThread_set_key_value() (with the same key). So these projects would
not be impacted by the change. I guess that the key is deleted to
workaround the current limitation ("strange behaviour") of
PyThread_set_key_value().

I cannot find pygmainloop.c in the latest development version:
https://git.gnome.org/browse/pygobject/tree/gi/_glib

I searched for usage at:

http://code.ohloh.net/
https://github.com/search/
http://searchcode.com/
http://stackoverflow.com/search

It's hard to find usage of PyThread_set_key_value() before they are a
lot of copies of CPython in the search engines, and I don't know how
to filter.

Victor

> Antoine Pitrou added the comment:
>
> Calling it _PyThread_set_key_value is prone to confusion.
> Ideally we would fix PyThread_set_key_value's behaviour. Are there users of the function outside from CPython?

The question is why does it behave this way in the first place?
Maybe for sub-interpreters?

> The question is why does it behave this way in the first place?
> Maybe for sub-interpreters?

The code is old. I don't think that it's related to subinterpreter.

PyThread_set_key_value() is used in _PyGILState_Init() and PyGILState_Ensure(), but its behaviour when the key already exists doesn't matter because these functions only call PyThread_set_key_value() once pr thread. It was probably simpler to implement PyThread_set_key_value() like it is nowadays. When the native TLS support was added, the new functions just mimic the old behaviour.


Code history.

find_key() function has been added at the same time than the PyThreadState structure. set_key_value() was already doing nothing when the key already exists. It looks like set_key_value() was not used.
---
changeset:   5405:b7871ca930ad
branch:      legacy-trunk
user:        Guido van Rossum <guido@python.org>
date:        Mon May 05 20:56:21 1997 +0000
files:       Include/Python.h Include/frameobject.h Include/pystate.h Include/pythread.h Include/thread.h Modules/threadmodule.c Objects/frameobject
description:
Massive changes for separate thread state management.
All per-thread globals are moved into a struct which is manipulated
separately.
---

TLS only started to be used in CPython since the following major change:
---
changeset:   28694:a4154dd5939a
branch:      legacy-trunk
user:        Mark Hammond <mhammond@skippinet.com.au>
date:        Sat Apr 19 15:41:53 2003 +0000
files:       Include/pystate.h Include/pythread.h Lib/test/test_capi.py Modules/_testcapimodule.c Modules/posixmodule.c Python/ceval.c Python/pystat
description:
New PyGILState_ API - implements pep 311, from patch 684256.
---

Native TLS support is very recent (compared to the first commit):
---
changeset:   64844:8e428b8e7d81
user:        Kristján Valur Jónsson <kristjan@ccpgames.com>
date:        Mon Sep 20 02:11:49 2010 +0000
files:       Python/pystate.c Python/thread_nt.h Python/thread_pthread.h
description:
issue 9786 Native TLS support for pthreads
PyThread_create_key now has a failure mode that the applicatino can detect.
---

Deja vu, this has come up before.  I wanted to change this because native TLS implementation become awkward.

https://mail.python.org/pipermail/python-dev/2008-August/081847.html

See also issue #10517

Attached patch fix PyThread_set_key_value() instead of adding a new function.

I prefer fix_set_key_value.patch instead of pythread_set_key_value.patch because I feel bad of keeping a buggy function and having to decide between a correct and a buggy version of the same function.

> Deja vu, this has come up before.  I wanted to change this because native TLS implementation become awkward.
> https://mail.python.org/pipermail/python-dev/2008-August/081847.html

You didn't get answer to this old email :-( I suppose that you still want to fix PyThread_set_key_value()?

Please see the rather long discussion in http://bugs.python.org/issue10517
There were issues having to do with fork.

But yes, I'd like to see this behave like normal.

> Please see the rather long discussion in http://bugs.python.org/issue10517
> There were issues having to do with fork.

Python has now a _PyGILState_Reinit() function.

I don't see how fix_set_key_value.patch would make the situation worse. What is the link between this issue and the issue #10517.

AFAICT, there's no link (FWIW I wrote the patch for #10517, then the fix
for threads created outside of Python calling into a subinterpreter -
issue #13156).
Actually, this "fix" would have avoided the regression in issue #13156.
But since it's tricky, I'd really like if Graham could test it (his module
does a lot of nasty things that could not show up in our test suite).

Only that issue #10517 mentions reasons to keep the old behavior, specifically http://bugs.python.org/issue10517#msg134573
I don't know if any of the old arguments are still valid, but I suggested changing this years ago and there was always some objection or other.

Kristján> Only that issue #10517 mentions reasons to keep the old behavior, specifically http://bugs.python.org/issue10517#msg134573 (...)

@Kristján: The behaviour of PyThread_set_key() discussed in this issue is unrelated to the pthread bug related to fork() fixed in #10517.

When it comes to threads and fork, I trust neologix because he knows them better than me and he wrote "AFAICT, there's no link".

> STINNER Victor added the comment:
>
> Kristján> Only that issue #10517 mentions reasons to keep the old behavior, specifically http://bugs.python.org/issue10517#msg134573 (...)
>
> @Kristján: The behaviour of PyThread_set_key() discussed in this issue is unrelated to the pthread bug related to fork() fixed in #10517.
>
> When it comes to threads and fork, I trust neologix because he knows them better than me and he wrote "AFAICT, there's no link".

It's unrelated, but I don't know if changing the current semantics
could break some code, especially involving subinterpreters: that's
why i'd like to have it tested.

But the current behavior is *really* a pain: not having
PyThread_set_key(key, value)
assert(PyThread_get_key(key) == value)

sounds really wrong.

+1
Why don't we just fix this and see where the chips fall?

If I understand correctly, there is probably only one application (mod_python) which might be broken by fix_set_key_value.patch.

If an application is broken by fix_set_key_value.patch, it can get the old behaviour using:

int
PyThread_set_key_value33(int key, void *value)
{
#if PY_VERSION_HEX >= 0x03040000
    void *oldValue = PyThread_get_key_value(key);
    if (oldValue != NULL)
        return 0;
#endif
    return PyThread_set_key_value(key, value);
}

So are you ok to apply the bugfix in Python 3.4?

On jeu., 2013-12-12 at 21:59 +0000, STINNER Victor wrote:
> So are you ok to apply the bugfix in Python 3.4?

I'm ok.

New changeset 46393019b650 by Victor Stinner in branch 'default':
Close #19787: PyThread_set_key_value() now always set the value. In Python 3.3,
http://hg.python.org/cpython/rev/46393019b650

Oh, my change on PyThread_set_key_value() has an unexpected effect on _testcapi.run_in_subinterp(): it now fixes the Python thread state. Py_NewInterpreter() creates a second Python thread state for the current thread, but PyThread_set_key_value() ignored the second call setting the new thread state.

It's a little bit strange that nobody noticed this bug before. It doesn't fix issue #10915: test_threading still hangs when tracemalloc is enabled (I modified manually test.support.run_in_subinterp() for a manual test).

This issue should now be fixed.

My commit broke test_capi, so I revert it. Here is the commit as a patch.

The impact on subinterpreters is more complex than what I expected. A decision should be take on what to do: mimic behaviour of Python 3.3 for subinterpreters (don't replace the Python thread state when a new subinterpreter is created), or fix code using subinterpreters.

New changeset 5d078b0bae75 by Victor Stinner in branch 'default':
Issue #19787: PyThread_set_key_value() now always set the value
http://hg.python.org/cpython/rev/5d078b0bae75