[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Changes by STINNER Victor victor.stin...@gmail.com: -- versions: +Python 3.4 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Antoine Pitrou added the comment: On jeu., 2013-12-12 at 21:59 +, STINNER Victor wrote: So are you ok to apply the bugfix in Python 3.4? I'm ok. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
STINNER Victor added the comment:
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 = 0x0304
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?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19787
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Roundup Robot added the comment: 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 -- nosy: +python-dev resolution: - fixed stage: - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Kristján Valur Jónsson added the comment: +1 Why don't we just fix this and see where the chips fall? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Charles-François Natali added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
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? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
STINNER Victor added the comment: 2013/11/28 Antoine Pitrou rep...@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 andpytools:/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 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Charles-François Natali added the comment: 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? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
STINNER Victor added the comment: 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 gu...@python.org date:Mon May 05 20:56:21 1997 + 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 mhamm...@skippinet.com.au date:Sat Apr 19 15:41:53 2003 + 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 krist...@ccpgames.com date:Mon Sep 20 02:11:49 2010 + 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. --- -- nosy: +kristjan.jonsson ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Kristján Valur Jónsson added the comment: 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 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Kristján Valur Jónsson added the comment: See also issue #10517 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
STINNER Victor added the comment: 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()? -- Added file: http://bugs.python.org/file32883/fix_set_key_value.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Kristján Valur Jónsson added the comment: But yes, I'd like to see this behave like normal. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Kristján Valur Jónsson added the comment: Please see the rather long discussion in http://bugs.python.org/issue10517 There were issues having to do with fork. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
STINNER Victor added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Charles-François Natali added the comment: 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). -- nosy: +grahamd ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Kristján Valur Jónsson added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
STINNER Victor added the comment: Oh, here is in interesting commit: --- changeset: 33705:891042c94aed branch: legacy-trunk user:Tim Peters tim.pet...@gmail.com date:Sat Oct 09 22:33:09 2004 + 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. -- keywords: +patch nosy: +pitrou, tim.peters Added file: http://bugs.python.org/file32879/pythread_set_key_value.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
STINNER Victor added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
New submission from STINNER Victor: 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. -- messages: 204442 nosy: haypo, neologix priority: normal severity: normal status: open title: tracemalloc: set_reentrant() should not have to call PyThread_delete_key() versions: Python 3.4 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
STINNER Victor added the comment:
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);
}
}
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19787
___
___
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Changes by Antoine Pitrou pit...@free.fr: -- type: - enhancement versions: +Python 3.5 -Python 3.4 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19787] tracemalloc: set_reentrant() should not have to call PyThread_delete_key()
Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com: -- nosy: +Arfrever ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19787 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
