classification
Title: Fix PyThread_set_key_value() strange behaviour
Type: enhancement Stage: resolved
Components: Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, grahamd, haypo, kristjan.jonsson, neologix, pitrou, python-dev, tim.peters
Priority: normal Keywords: patch

Created on 2013-11-26 00:13 by haypo, last changed 2013-12-13 11:11 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
pythread_set_key_value.patch haypo, 2013-11-28 01:24 review
fix_set_key_value.patch haypo, 2013-11-28 21:37 review
pythread_set_key_value-2.patch haypo, 2013-12-13 03:17 review
Messages (25)
msg204442 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-26 00:13
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.
msg204443 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-26 00:14
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);
    }
}
msg204634 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 01:24
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.
msg204636 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 01:29
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.
msg204660 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-11-28 14:35
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?
msg204674 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 16:17
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
msg204676 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-11-28 16:51
> 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?
msg204680 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 17:20
> 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.
---
msg204691 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-11-28 19:43
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
msg204692 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-11-28 19:49
See also issue #10517
msg204703 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 21:37
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()?
msg204704 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-11-28 21:55
Please see the rather long discussion in http://bugs.python.org/issue10517
There were issues having to do with fork.
msg204705 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-11-28 21:55
But yes, I'd like to see this behave like normal.
msg204706 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-11-28 22:01
> 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.
msg204707 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-11-28 22:17
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).
msg204708 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-11-28 22:24
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.
msg205158 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-03 21:30
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".
msg205161 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-12-03 21:52
> 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.
msg205216 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2013-12-04 11:27
+1
Why don't we just fix this and see where the chips fall?
msg205985 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-12 21:59
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?
msg205986 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-12-12 22:00
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.
msg206014 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-13 02:32
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
msg206015 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 02:35
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.
msg206023 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2013-12-13 03:17
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.
msg206043 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-13 10:14
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
History
Date User Action Args
2013-12-13 11:11:50hayposetstatus: open -> closed
resolution: fixed
2013-12-13 10:14:21python-devsetmessages: + msg206043
2013-12-13 03:17:45hayposetstatus: closed -> open
files: + pythread_set_key_value-2.patch
resolution: fixed -> (no value)
messages: + msg206023
2013-12-13 02:35:29hayposetmessages: + msg206015
2013-12-13 02:32:23hayposettitle: tracemalloc: set_reentrant() should not have to call PyThread_delete_key() -> Fix PyThread_set_key_value() strange behaviour
2013-12-13 02:32:05python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg206014

resolution: fixed
stage: resolved
2013-12-12 22:00:04pitrousetmessages: + msg205986
2013-12-12 21:59:31hayposetversions: + Python 3.4
2013-12-12 21:59:23hayposetmessages: + msg205985
2013-12-04 11:27:38kristjan.jonssonsetmessages: + msg205216
2013-12-03 21:52:17neologixsetmessages: + msg205161
2013-12-03 21:30:23hayposetmessages: + msg205158
2013-11-28 22:24:21kristjan.jonssonsetmessages: + msg204708
2013-11-28 22:17:38neologixsetnosy: + grahamd
messages: + msg204707
2013-11-28 22:01:55hayposetmessages: + msg204706
2013-11-28 21:55:56kristjan.jonssonsetmessages: + msg204705
2013-11-28 21:55:22kristjan.jonssonsetmessages: + msg204704
2013-11-28 21:37:27hayposetfiles: + fix_set_key_value.patch

messages: + msg204703
2013-11-28 19:49:13kristjan.jonssonsetmessages: + msg204692
2013-11-28 19:43:33kristjan.jonssonsetmessages: + msg204691
2013-11-28 17:20:45hayposetnosy: + kristjan.jonsson
messages: + msg204680
2013-11-28 16:51:16neologixsetmessages: + msg204676
2013-11-28 16:17:52hayposetmessages: + msg204674
2013-11-28 14:35:53pitrousetmessages: + msg204660
2013-11-28 01:29:49hayposetmessages: + msg204636
2013-11-28 01:25:04hayposetfiles: + pythread_set_key_value.patch

nosy: + tim.peters, pitrou
messages: + msg204634

keywords: + patch
2013-11-26 02:27:57Arfreversetnosy: + Arfrever
2013-11-26 01:45:33pitrousettype: enhancement
versions: + Python 3.5, - Python 3.4
2013-11-26 00:14:30hayposetmessages: + msg204443
2013-11-26 00:13:57haypocreate