| msg204442 - (view) |
Author: STINNER Victor (vstinner) *  |
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 (vstinner) * |
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 (vstinner) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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) * |
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) * |
Date: 2013-11-28 19:49 |
| See also issue #10517 |
|
|
| msg204703 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
Date: 2013-11-28 21:55 |
| But yes, I'd like to see this behave like normal. |
|
|
| msg204706 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
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 (vstinner) * |
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) * |
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) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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 (vstinner) * |
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 |
|
|