classification
Title: threading.local() does not work with C-created threads
Type: enhancement Stage: committed/rejected
Components: Documentation Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: BreamoreBoy, amaury.forgeotdarc, anacrolix, belopolsky, docs@python, eric.araujo, ezio.melotti, georg.brandl, meador.inge, ncoghlan, nikratio, python-dev, ronaldoussoren, swapnil, verigak
Priority: normal Keywords: easy, patch

Created on 2009-08-02 20:02 by nikratio, last changed 2014-01-20 05:12 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
lib.c nikratio, 2010-02-16 15:02
test.py nikratio, 2010-02-16 15:03
issue6627.patch nikratio, 2014-01-08 05:25
Messages (19)
msg91198 - (view) Author: Nikolaus Rath (nikratio) * Date: 2009-08-02 20:02
When threads are created by a C extension loaded with ctypes,
threading.local() objects are always empty. If one uses
_threading_local.local() instead of threading.local(), the problem does
not occur. 

More information and example program showing the behaviour:
http://code.google.com/p/fusepy/issues/detail?id=15
msg99414 - (view) Author: Nikolaus Rath (nikratio) * Date: 2010-02-16 15:02
Here is a simple testcase:

$ gcc -fPIC -g -c -Wall lib.c
$ gcc -shared -Wl -o lib.so lib.o
$ python test.py
In callback: creating thread
Python callback_fn called, setting lc.x = 42
Python callback_fn called, setting lc.x = 42


Expected output:

$ python test.py
In callback: creating thread
Python callback_fn called, setting lc.x = 42
Python callback_fn called, lc.x = 42
msg99415 - (view) Author: Nikolaus Rath (nikratio) * Date: 2010-02-16 15:06
Note also that if the from __future__ import is enabled, the program segfaults instead (separate bug)?
msg116671 - (view) Author: Swapnil Talekar (swapnil) Date: 2010-09-17 14:43
As far as I know, the thread creation done in the file is not correct. While creating threads in C extension, there are certain rules to follow. Firstly, Python should be made thread-aware if it is not already i.e. call PyEval_InitThreads in the C callback function. After its creation, the thread should bootstrap to be able to execute Python code. It should create a new PyThreadState for itself by calling PyThreadState_New. For this, the thread should be passed the InterpreterState* through the entry function. Before executing any Python code, the thread should make sure that the current ThreadState * is corresponding to it by calling PyEval_RestoreThread.
Refer http://docs.python.org/c-api/init.html#thread-state-and-the-global-interpreter-lock
msg116672 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-09-17 15:08
Swapnil's analysis looks correct to me - there are certain rules you have to follow before calling back into the Python interpreter core. If you don't follow them, the behaviour you will get is completely undefined.

If the problem still occurs even when the C thread is correctly initialised for calling into the Python C API then this issue can be reopened.
msg116673 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-09-17 15:28
One more thing:
in the sample code, the thread initializes no PyThreadState. So the ctypes callback creates a temporary PyThreadState just for the duration of the call.

This explains the difference between threading.local and _threading_local:

- threading.local() uses the PyThreadState to store its data: You get a new value on each call.
- _threading_local.local() uses a global dictionary indexed by the thread id: the same value is retained across call.
msg116710 - (view) Author: Nikolaus Rath (nikratio) * Date: 2010-09-17 18:58
@Swapnil: the rules you quote are correct for the C extension, but do not apply when using ctypes, because ctypes is doing the required initializations automatically.

However, if Amaury is correct, ctypes performs the initializations in a way that break the threading.local functionality. 

I think the best way to address this bug would therefore be to add a warning to the ctypes documentation that C created threads will not support threading.local().
msg116720 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-09-17 20:49
> ctypes performs the initializations in a way that break the
> threading.local functionality. 

Not quite. ctypes (or more exactly: the PyGILState_Ensure() and PyGILState_Release() functions, which are the standard API to do this) is in a situation where it must call Python code from a thread which has no PyThreadState.  So it creates a thread state, runs the code, and destroys the thread state; is this wrong?

If you want to keep Python state during your C thread, there are 4 lines to add to your function:

void *async_cb(void *dummy)
{
    PyGILState_STATE gstate = PyGILState_Ensure();
    Py_BEGIN_ALLOW_THREADS
    (*callback_fn)();
    (*callback_fn)();
    Py_END_ALLOW_THREADS
    PyGILState_Release(gstate);
    pthread_exit(NULL);
}
msg116721 - (view) Author: Nikolaus Rath (nikratio) * Date: 2010-09-17 21:10
No, I am not saying that the behaviour of ctypes is wrong. It just happens to have some effects on threading.local that I think should be documented. That's why I reassigned this as a documentation bug. 

Please reconsider closing this bug. I'm also happy to change the type to "Feature request".


As an aside: I think in most cases one uses ctypes to call functions that are already compiled, so changing the source is not an option.
msg116722 - (view) Author: Nikolaus Rath (nikratio) * Date: 2010-09-17 21:16
To be a bit more constructive, why not add something like this in paragraph to http://docs.python.org/library/ctypes.html#callback-functions:

"Note that if the callback function is called in a new thread that has been created outside of Python's control (i.e., by the foreign code that calls the callback), ctypes creates a new dummy Python thread on every invocation. That means that values stored with `threading.local` will *not* survive across different callbacks.
msg116725 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-09-17 22:13
This is not specific to ctypes.
Please read http://docs.python.org/c-api/init.html#thread-state-and-the-global-interpreter-lock
specially the paragraph that says "...when threads are created from C...".
Is it explicit enough? How would you change it?
msg116726 - (view) Author: Nikolaus Rath (nikratio) * Date: 2010-09-17 22:29
One point of ctypes is to save the user the trouble of having to create a full blown C extension, so I don't think it's reasonable to expect a ctypes user to have read the full C API documentation as well. Only a very small subset of the page that you gave is actually relevant for use with ctypes. Why not put this information where a ctypes user can find it easily?
msg116747 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-09-18 02:20
The suggestion in http://bugs.python.org/issue6627#msg116722 is a good one.

This a case where the user may legitimately not realise that threading.local is stored in the *temporary* state created by ctypes rather than in something more persistent inside the interpreter.

Since the ctypes state is per callback, it won't persist across calls, even when they're made from the same thread.

Suggested wording:
"""Note that if the callback function is called in a new thread that has been created outside of Python's control (i.e., by the foreign code that calls the callback), ctypes creates a new dummy Python thread on every invocation, including recreation of the thread local storage area. While this is correct for most purposes, it does mean that values stored with `threading.local` will *not* survive across different callbacks, even when those calls are made from the same C thread."""
msg117029 - (view) Author: Swapnil Talekar (swapnil) Date: 2010-09-21 04:38
Nick, the last statement,
"While this is correct for most purposes, it does mean that..."
can be simplified to, 
"It means...". 
I had to read it several times before I realized, there is no "not" after "does" :) 
BTW, since this particular arrangement of having a temporary thread state during the callback is particularly useful for ctypes (I cannot imagine any other usecase) and also it sort-of breaks things, a potential feature request could be to have consistent thread state during the lifetime of a C thread. I don't have much idea how to do that or whether it is even possible? Would anyone like to give a thought?
msg117048 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-09-21 13:03
On Tue, Sep 21, 2010 at 2:39 PM, Swapnil Talekar <report@bugs.python.org> wrote:
> Swapnil Talekar <swapnil.st@gmail.com> added the comment:
> Nick, the last statement,
> "While this is correct for most purposes, it does mean that..."
> can be simplified to,
> "It means...".
> I had to read it several times before I realized, there is no "not" after "does" :)

The shorter version doesn't mean the same thing though - the ctypes
arrangement *really is* correct for most purposes. The only issue is
that threading.local won't persist, since the storage is blown away as
soon as the callback returns.

> BTW, since this particular arrangement of having a temporary thread state during the callback is particularly useful for ctypes (I cannot imagine any other usecase) and also it sort-of breaks things, a potential feature request could be to have consistent thread state during the lifetime of a C thread. I don't have much idea how to do that or whether it is even possible? Would anyone like to give a thought?

There's no easy way to make the thread state persist between calls, as
ctypes needs to destroy the thread state it creates at some point to
avoid a memory leak. Since it has no way of knowing when the
underlying C thread is no longer in use, it is forced to assume that
every call is going to be the last one from that thread and kill the
thread state.
msg185554 - (view) Author: Mark Lawrence (BreamoreBoy) Date: 2013-03-30 12:50
msg116747 suggests wording for a doc patch.  Could this be applied and this issue closed?  msg117029 talks about a potential feature request.  Was this ever discussed?
msg207669 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-08 05:25
Here's a patch that (I think) incorporates all the comments. If someone could apply it, that would be great :-).
msg208521 - (view) Author: Nikolaus Rath (nikratio) * Date: 2014-01-20 04:44
(adding the documentation and ctypes experts from http://docs.python.org/devguide/experts.html to noisy list in the hope to get this moving again.)
msg208523 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-20 05:12
New changeset f4eade5df217 by Benjamin Peterson in branch '3.3':
document that a new Python thread context is created in ctypes callbacks (closes #6627)
http://hg.python.org/cpython/rev/f4eade5df217

New changeset 9cd2d7a3f9f2 by Benjamin Peterson in branch '2.7':
document that a new Python thread context is created in ctypes callbacks (closes #6627)
http://hg.python.org/cpython/rev/9cd2d7a3f9f2

New changeset fd647825475a by Benjamin Peterson in branch 'default':
merge 3.3 (#6627)
http://hg.python.org/cpython/rev/fd647825475a
History
Date User Action Args
2014-01-20 05:12:33python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg208523

resolution: fixed
stage: needs patch -> committed/rejected
2014-01-20 04:44:54nikratiosetnosy: + georg.brandl, belopolsky, ezio.melotti, eric.araujo, meador.inge

messages: + msg208521
versions: - Python 2.6, Python 3.1, Python 3.2, Python 3.5
2014-01-08 05:25:20nikratiosetfiles: + issue6627.patch
keywords: + patch
messages: + msg207669

versions: + Python 3.1, Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5
2013-07-24 13:08:32ronaldoussorensetnosy: + ronaldoussoren
2013-03-30 12:50:29BreamoreBoysetnosy: + BreamoreBoy
messages: + msg185554
2010-09-21 13:03:21ncoghlansetmessages: + msg117048
2010-09-21 04:39:01swapnilsetmessages: + msg117029
2010-09-18 02:20:36ncoghlansetstatus: closed -> open
type: behavior -> enhancement
messages: + msg116747

keywords: + easy
resolution: works for me -> (no value)
stage: committed/rejected -> needs patch
2010-09-17 22:29:15nikratiosetmessages: + msg116726
2010-09-17 22:13:20amaury.forgeotdarcsetmessages: + msg116725
2010-09-17 21:16:08nikratiosetmessages: + msg116722
2010-09-17 21:10:20nikratiosetmessages: + msg116721
2010-09-17 20:49:23amaury.forgeotdarcsetstatus: open -> closed
resolution: works for me
messages: + msg116720
2010-09-17 18:58:30nikratiosetresolution: not a bug -> (no value)
2010-09-17 18:58:20nikratiosetstatus: closed -> open

nosy: + docs@python
messages: + msg116710

assignee: docs@python
components: + Documentation, - Library (Lib)
2010-09-17 15:28:45amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg116673
2010-09-17 15:08:10ncoghlansetstatus: open -> closed
resolution: not a bug
messages: + msg116672

stage: committed/rejected
2010-09-17 14:43:43swapnilsetnosy: + swapnil
messages: + msg116671
2010-07-18 16:23:43anacrolixsetnosy: + anacrolix
2010-02-16 15:06:18nikratiosetmessages: + msg99415
2010-02-16 15:03:10nikratiosetfiles: + test.py
2010-02-16 15:02:58nikratiosetfiles: + lib.c
2010-02-16 15:02:42nikratiosetmessages: + msg99414
2009-08-16 16:04:57verigaksetnosy: + verigak
2009-08-07 16:20:02ncoghlansetnosy: + ncoghlan
2009-08-02 20:02:43nikratiocreate