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

I've added a test in r60075.

It shows that threading.local cleans up thread local data except for the
last stopped thread.

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

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.

Here is a patch that removes the member localobject->dict.
Now the dictionary is always retrieved from the thread state.

see also bpo-3710

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 bpo-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

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.

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.

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.

this is ready for more review at http://codereview.appspot.com/3641

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 bpo-3757.

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()?

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

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.

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.

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.

Is any of the patches believed to be ready?

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.

Since no specific patch was proposed for 2.5.3, I'm untargetting it for 2.5.

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.

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.

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/

Thank you Gregory. I've committed the patch in r84344 (py3k), r84345 (3.1) and r84346 (2.7).