msg60127 - (view) |
Author: Peter Fein (pfein) |
Date: 2008-01-18 23:55 |
threading.local doesn't free attributes assigned to a local() instance
when the assigning thread exits. See attached patch for
_threading_local.py doctests.
Per discussion with Crys and arkanes in #python, this may be an issue
with PyThreadState_Clear / PyThreadState_GetDict
This appears to affect both thread._local and _threading_local.py
|
msg60164 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-01-19 14:05 |
I've added a test in r60075.
It shows that threading.local cleans up thread local data except for the
last stopped thread.
|
msg60180 - (view) |
Author: Peter Fein (pfein) |
Date: 2008-01-19 14:50 |
re: r60075 : I'm unclear - is this intentional behavior?
Adding a `del t` at line 24 after the loop doesn't help either (though
it does make the test somewhat clear IMO).
|
msg60183 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2008-01-19 15:20 |
On the opposite, simply evaluating
local.__dict__
just before "deadlist = ...", frees the last value.
Weird? I found that in threadmodule.c, the local object has a
"self->dict" attribute, which contains the last used dictionary. The
dictionaries switch when tp_getattro or tp_setattro are called from a
different thread.
Maybe localobject could be rewritten with no self->dict at all. Or make
self->dict a *borrowed* reference inside the array of thread-local
dictionaries.
|
msg60203 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2008-01-19 17:43 |
Here is a patch that removes the member localobject->dict.
Now the dictionary is always retrieved from the thread state.
|
msg72062 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-08-28 04:44 |
see also #3710
|
msg72063 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-08-28 05:29 |
I like Amaury's patch, it gets rid of what seems like an existing gross
hack of having localobject->dict exist at all.
I believe it may also fix #3710 by getting rid of the unnecessary
localobject->dict member.
Could someone else please review this?
threading_local.patch is available for review here:
http://codereview.appspot.com/3641
|
msg72110 - (view) |
Author: Ben Cottrell (tamino) |
Date: 2008-08-28 20:24 |
I like this patch, too! I think it's a much cleaner way of implementing
the thread._local type. However, when I test it, I have problems with
subclasses of thread._local; using the class itself seems to work.
I've attached a test program that shows the issue.
|
msg72112 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-08-28 21:09 |
Good catch, Ben!
The generic setattr/getattr functions don't work as expected when the
base class doesn't provide a __dict__. They are setting the attributes
in the subclass' __dict__ instead of the thread local dict.
My patch fixes the behavior. However it might be a better idea to make
the threadlocal class non subclass-able.
|
msg72115 - (view) |
Author: Ben Cottrell (tamino) |
Date: 2008-08-28 22:11 |
Christian,
Your patch works for me -- thanks!!
I made a slight modification to your patch to allow "del" to work,
and have attached my modified version.
I agree that allowing subclassing makes thread._local harder to get
right than it would otherwise be. There is code out there that uses
that feature, though -- I'm running into it in the context of django,
which (when using the sqlite database back end) keeps its sqlite
connections in a subclass of thread._local.
|
msg72315 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-09-02 06:42 |
this is ready for more review at http://codereview.appspot.com/3641
|
msg72333 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-09-02 12:14 |
The patch is basically fine with me. While reviewing it I've found out
that threading.local doesn't have cyclic garbage collecting enabled.
I've opened a new issue for it in #3757.
|
msg72335 - (view) |
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * |
Date: 2008-09-02 12:55 |
With the patch, subclasses of threading.local seem to have an empty
__dict__:
import threading
class MyLocal(threading.local):
pass
l = MyLocal()
l.x = 2
l.__dict__ # returns {}
Maybe __dict__ should be special-cased in local_getattro()?
|
msg72336 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-09-02 13:21 |
Amaury Forgeot d'Arc wrote:
> Maybe __dict__ should be special-cased in local_getattro()?
With the patch it's also no longer possible to get a list of attribute
names. +1 for special casing __dict__ in getattro and setattro.
setattr(local, "__dict__", egg) should raise an AttributeError.
Christian
|
msg72799 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-09-08 22:24 |
i won't have time to work on this for several days but i will happily
review updated patches if anyone could contribute fixes for the __dict__
issues described in the most recent comments.
feel free to steal the issue from me.
|
msg72974 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-09-10 16:17 |
If Christian's analysis is right, and memory is released except for the
last thread, I don't think this qualifies as a release blocker.
Also (unless I misinterpret the issue), it seems that we have made many
releases already which had this bug, so it's unclear why it should block
the next release.
Tentatively lowering the priority to high.
|
msg72985 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-09-10 21:07 |
Agreed, this is fine for a bugfix point release.
I was initially concerned about the Modules/threadmodule.c struct
localobject changing but that is private and used only by code within
threadmodule.c so its not part of an API anything else could depend on.
Changing it for 2.6.1/2.5.3/3.0.1 should be fine.
|
msg76584 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-11-29 08:11 |
Is any of the patches believed to be ready?
|
msg76589 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-11-29 12:46 |
Reconsidering: I think the "needs review" keyword should only be applied
if there is a single patch associated with the issue that needs review.
So anybody setting the keyword should remove all patches that don't need
review anymore (as they are superceded). I'm removing the keyword for now.
|
msg77520 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2008-12-10 09:16 |
Since no specific patch was proposed for 2.5.3, I'm untargetting it for 2.5.
|
msg114574 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-08-21 22:12 |
The original patch won't apply anymore, because of changes in the thread._local implementation.
Instead, here is a new patch, which also adds tests for the __dict__ behaviour, and two new private API functions: _PyObject_Generic{Get,Set}AttrWithDict. These are the same as PyObject_Generic{Get,Set}Attr, except that it allows to pass the instance dict instead of letting the function discover it.
|
msg114577 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-08-21 22:21 |
It should be noted that the original reason for this issue is already gone in SVN (as part of the reimplementation alluded to above), but the timing fragility is still there since there can be a thread switch between the moment the "dict" member is set and the moment it is actually used. Hence the patch I've posted, to get rid of the "dict" member and instead handle lookups of the __dict__ attributes ourselves.
|
msg114689 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2010-08-22 18:28 |
your updated patch looks good to me. i've posted it here for easy review if anyone else wants to take a look:
http://codereview.appspot.com/1995049/
|
msg115165 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-08-28 18:30 |
Thank you Gregory. I've committed the patch in r84344 (py3k), r84345 (3.1) and r84346 (2.7).
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:29 | admin | set | github: 46176 |
2010-08-30 22:01:57 | ncoghlan | set | nosy:
+ ncoghlan
|
2010-08-28 18:30:12 | pitrou | set | status: open -> closed resolution: fixed messages:
+ msg115165
stage: resolved |
2010-08-22 18:28:39 | gregory.p.smith | set | assignee: gregory.p.smith -> pitrou messages:
+ msg114689 |
2010-08-21 22:21:02 | pitrou | set | messages:
+ msg114577 |
2010-08-21 22:12:46 | pitrou | link | issue3710 superseder |
2010-08-21 22:12:09 | pitrou | set | files:
+ threadlocal.patch
messages:
+ msg114574 versions:
- Python 2.6 |
2010-05-11 20:42:23 | terry.reedy | set | versions:
+ Python 3.1, Python 2.7, Python 3.2, - Python 3.0 |
2008-12-10 09:16:55 | loewis | set | messages:
+ msg77520 versions:
- Python 2.5, Python 2.5.3 |
2008-11-29 12:46:59 | loewis | set | keywords:
- needs review messages:
+ msg76589 |
2008-11-29 08:11:14 | loewis | set | messages:
+ msg76584 |
2008-11-29 04:48:35 | gregory.p.smith | set | versions:
+ Python 2.5.3 |
2008-09-10 21:07:45 | gregory.p.smith | set | messages:
+ msg72985 |
2008-09-10 16:17:30 | loewis | set | priority: release blocker -> high nosy:
+ loewis messages:
+ msg72974 |
2008-09-09 13:14:43 | barry | set | priority: deferred blocker -> release blocker |
2008-09-08 22:24:35 | gregory.p.smith | set | messages:
+ msg72799 |
2008-09-04 01:16:12 | benjamin.peterson | set | priority: release blocker -> deferred blocker |
2008-09-02 13:21:03 | christian.heimes | set | messages:
+ msg72336 |
2008-09-02 12:55:37 | amaury.forgeotdarc | set | messages:
+ msg72335 |
2008-09-02 12:14:42 | pitrou | set | nosy:
+ pitrou messages:
+ msg72333 |
2008-09-02 06:42:52 | gregory.p.smith | set | files:
+ threading_local4.patch messages:
+ msg72315 |
2008-09-02 05:48:31 | gregory.p.smith | set | assignee: gregory.p.smith |
2008-08-28 22:11:33 | tamino | set | files:
+ threading_local3.patch messages:
+ msg72115 |
2008-08-28 21:09:19 | christian.heimes | set | files:
+ threading_local2.patch messages:
+ msg72112 |
2008-08-28 20:24:49 | tamino | set | files:
+ test1868.py nosy:
+ tamino messages:
+ msg72110 |
2008-08-28 05:29:26 | gregory.p.smith | set | priority: high -> release blocker keywords:
+ needs review messages:
+ msg72063 |
2008-08-28 04:44:36 | gregory.p.smith | set | messages:
+ msg72062 |
2008-08-28 04:37:36 | gregory.p.smith | set | keywords:
+ patch nosy:
+ gregory.p.smith |
2008-01-19 17:43:55 | amaury.forgeotdarc | set | files:
+ threading_local.patch messages:
+ msg60203 |
2008-01-19 15:20:53 | amaury.forgeotdarc | set | nosy:
+ amaury.forgeotdarc messages:
+ msg60183 |
2008-01-19 14:50:56 | pfein | set | messages:
+ msg60180 |
2008-01-19 14:05:31 | christian.heimes | set | nosy:
+ christian.heimes messages:
+ msg60164 |
2008-01-19 00:10:14 | christian.heimes | set | priority: high components:
+ Extension Modules versions:
+ Python 2.6, Python 3.0 |
2008-01-18 23:55:06 | pfein | create | |