classification
Title: threading.local doesn't free attrs when assigning thread exits
Type: behavior Stage: resolved
Components: Extension Modules, Library (Lib) Versions: Python 3.2, Python 3.1, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: amaury.forgeotdarc, christian.heimes, gregory.p.smith, loewis, ncoghlan, pfein, pitrou, tamino
Priority: high Keywords: patch

Created on 2008-01-18 23:55 by pfein, last changed 2010-08-30 22:01 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
_threading_local.py.patch pfein, 2008-01-18 23:55 patch for threading.local doctests
threading_local.patch amaury.forgeotdarc, 2008-01-19 17:43
test1868.py tamino, 2008-08-28 20:24 Test program
threading_local2.patch christian.heimes, 2008-08-28 21:09
threading_local3.patch tamino, 2008-08-28 22:11 Slightly modified version of threading_local2.patch
threading_local4.patch gregory.p.smith, 2008-09-02 06:42 based on threading_local3, includes the unit test and fixes one code review comment.
threadlocal.patch pitrou, 2010-08-21 22:12
Messages (24)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2008-08-28 04:44
see also #3710
msg72063 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2008-09-02 06:42
this is ready for more review at http://codereview.appspot.com/3641
msg72333 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2008-11-29 08:11
Is any of the patches believed to be ready?
msg76589 - (view) Author: Martin v. Löwis (loewis) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2010-08-28 18:30
Thank you Gregory. I've committed the patch in r84344 (py3k), r84345 (3.1) and r84346 (2.7).
History
Date User Action Args
2010-08-30 22:01:57ncoghlansetnosy: + ncoghlan
2010-08-28 18:30:12pitrousetstatus: open -> closed
resolution: fixed
messages: + msg115165

stage: resolved
2010-08-22 18:28:39gregory.p.smithsetassignee: gregory.p.smith -> pitrou
messages: + msg114689
2010-08-21 22:21:02pitrousetmessages: + msg114577
2010-08-21 22:12:46pitroulinkissue3710 superseder
2010-08-21 22:12:09pitrousetfiles: + threadlocal.patch

messages: + msg114574
versions: - Python 2.6
2010-05-11 20:42:23terry.reedysetversions: + Python 3.1, Python 2.7, Python 3.2, - Python 3.0
2008-12-10 09:16:55loewissetmessages: + msg77520
versions: - Python 2.5, Python 2.5.3
2008-11-29 12:46:59loewissetkeywords: - needs review
messages: + msg76589
2008-11-29 08:11:14loewissetmessages: + msg76584
2008-11-29 04:48:35gregory.p.smithsetversions: + Python 2.5.3
2008-09-10 21:07:45gregory.p.smithsetmessages: + msg72985
2008-09-10 16:17:30loewissetpriority: release blocker -> high
nosy: + loewis
messages: + msg72974
2008-09-09 13:14:43barrysetpriority: deferred blocker -> release blocker
2008-09-08 22:24:35gregory.p.smithsetmessages: + msg72799
2008-09-04 01:16:12benjamin.petersonsetpriority: release blocker -> deferred blocker
2008-09-02 13:21:03christian.heimessetmessages: + msg72336
2008-09-02 12:55:37amaury.forgeotdarcsetmessages: + msg72335
2008-09-02 12:14:42pitrousetnosy: + pitrou
messages: + msg72333
2008-09-02 06:42:52gregory.p.smithsetfiles: + threading_local4.patch
messages: + msg72315
2008-09-02 05:48:31gregory.p.smithsetassignee: gregory.p.smith
2008-08-28 22:11:33taminosetfiles: + threading_local3.patch
messages: + msg72115
2008-08-28 21:09:19christian.heimessetfiles: + threading_local2.patch
messages: + msg72112
2008-08-28 20:24:49taminosetfiles: + test1868.py
nosy: + tamino
messages: + msg72110
2008-08-28 05:29:26gregory.p.smithsetpriority: high -> release blocker
keywords: + needs review
messages: + msg72063
2008-08-28 04:44:36gregory.p.smithsetmessages: + msg72062
2008-08-28 04:37:36gregory.p.smithsetkeywords: + patch
nosy: + gregory.p.smith
2008-01-19 17:43:55amaury.forgeotdarcsetfiles: + threading_local.patch
messages: + msg60203
2008-01-19 15:20:53amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg60183
2008-01-19 14:50:56pfeinsetmessages: + msg60180
2008-01-19 14:05:31christian.heimessetnosy: + christian.heimes
messages: + msg60164
2008-01-19 00:10:14christian.heimessetpriority: high
components: + Extension Modules
versions: + Python 2.6, Python 3.0
2008-01-18 23:55:06pfeincreate