| 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). |
|
|