Fix PyThread_set_key_value() strange behaviour

[vstinner]

BPO	19787
Nosy	@tim-one, @pitrou, @kristjanvalur, @vstinner
Files	pythread_set_key_value.patch fix_set_key_value.patch pythread_set_key_value-2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-12-13.11:11:50.495>
created_at = <Date 2013-11-26.00:13:57.877>
labels = ['type-feature']
title = 'Fix PyThread_set_key_value() strange behaviour'
updated_at = <Date 2013-12-13.11:11:50.494>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2013-12-13.11:11:50.494>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2013-12-13.11:11:50.495>
closer = 'vstinner'
components = []
creation = <Date 2013-11-26.00:13:57.877>
creator = 'vstinner'
dependencies = []
files = ['32879', '32883', '33110']
hgrepos = []
issue_num = 19787
keywords = ['patch']
message_count = 25.0
messages = ['204442', '204443', '204634', '204636', '204660', '204674', '204676', '204680', '204691', '204692', '204703', '204704', '204705', '204706', '204707', '204708', '205158', '205161', '205216', '205985', '205986', '206014', '206015', '206023', '206043']
nosy_count = 8.0
nosy_names = ['tim.peters', 'pitrou', 'kristjan.jonsson', 'vstinner', 'grahamd', 'Arfrever', 'neologix', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19787'
versions = ['Python 3.4', 'Python 3.5']

[vstinner]

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.

[vstinner]

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

[vstinner]

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.

[vstinner]

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.

[pitrou]

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?

[vstinner]

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

[neologix]

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?

[vstinner]

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:
bpo-9786 Native TLS support for pthreads
PyThread_create_key now has a failure mode that the applicatino can detect.
---

[kristjanvalur]

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

[kristjanvalur]

See also issue bpo-10517

[vstinner]

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()?

[kristjanvalur]

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

[kristjanvalur]

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

[vstinner]

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 bpo-10517.