Ayende @ Rahien

It's a girl

Wrong answer #1: Your own ThreadLocal

Originally posted at 12/15/2010

Well, one way we can use to solve the problem is by introducing a finalizer, like so:

public class CloseableThreadLocal
{
    [ThreadStatic] private static Dictionary<object, object> slots;

    public static Dictionary<object, object> Slots
    {
        get { return slots ?? (slots = new Dictionary<object, object>()); }
    }

    public /*protected internal*/ virtual Object InitialValue()
    {
        return null;
    }

    public virtual Object Get()
    {
        object val;

        if (Slots.TryGetValue(this, out val))
        {
            return val;
        }
        val = InitialValue();
        Set(val);
        return val;
    }

    public virtual void Set(object val)
    {
        Slots[this] = val;
    }

    public virtual void Close()
    {
        GC.SuppressFinalize(this);
        if (slots != null)// intentionally using the field here, to avoid creating the instance
            slots.Remove(this);
    }

   ~CloseableThreadLocal()
   {
         Close();
   }
}

But this will not actually work, executing this code shows that we still have a memory leak:

internal class Program
{
    private static void Main(string[] args)
    {
        UseThreadLocal();
        GC.Collect(int.MaxValue);
        GC.WaitForPendingFinalizers();
        Console.WriteLine(CloseableThreadLocal.slots.Count);
    }

    private static void UseThreadLocal()
    {
        var tl = new CloseableThreadLocal();
        tl.Set("hello there");
        Console.WriteLine(tl.Get());
    }
}

Why? And how can we fix this?

Comments

commenter
12/18/2010 10:22 AM by
commenter

The finalizer runs on a different thread, so works on a whole other threadstatic 'slots' than the one you want to clean up. As for a solution, I havent thought of one yet :)

commenter
12/18/2010 10:28 AM by
commenter

Maybe slots dictionaries could be registered in a lookup table by thread ID, for retrievability in the finalizer.

Koen
12/18/2010 11:52 AM by
Koen

I'd think the finalizer would never be executed in the case that slots holds a reference to the current object.

Daniel Grunwald
12/18/2010 12:00 PM by
Daniel Grunwald

In this wrong answer, the finalizer doesn't run at all; because the object is still referenced by the dictionary.

To fix this issue, don't use the CloseableThreadLocal as key; but create a separate key object within the CloseableThreadLocal. This prevents the CloseableThreadLocal from being reachable via the dictionary keys.

However it might still be reachable from a value in the dictionary - see my "class Leak" and comments about ConditionalWeakTable on the previous blog post.

Once you get the finalizer running, you still have to ensure that you remove the entry from all dictionaries (the ThreadLocal might have been used on multiple threads). As it is, the code removes the entry only from the finalizer thread's dictionary. A static List of all dictionaries might help; but then you'll have to think of something to remove the dictionaries from that list when the corresponding thread exits.

Koen
12/18/2010 12:02 PM by
Koen

I guess a WeakReference might also do the job? (not sure)

Daniel Grunwald
12/18/2010 12:06 PM by
Daniel Grunwald

Yes, using a WeakReference as key will do the same job (and initially I was also thinking of using one); but would be pretty strange as the dictionary will use reference equality of the WeakReference, and never look at what the weak reference is pointing to.

So whether your key is a weak reference or a simple "readonly object key = new object();" doesn't matter.

hazzik
12/18/2010 12:28 PM by
hazzik
public class CloseableThreadLocal

{

    [ThreadStatic] private static Dictionary

<object,> slots;

    private readonly object key = new object();

    private readonly Dictionary

<object,> local = Slots;

    public static Dictionary

<object,> Slots

    {

        get { return slots ?? (slots = new Dictionary

<object,> ()); }

    }


    public /*protected internal*/ virtual Object InitialValue()

    {

        return null;

    }


    public virtual Object Get()

    {

        object val;

        if (local.TryGetValue(key, out val))

            return val;

        val = InitialValue();

        Set(val);

        return val;

    }


    public virtual void Set(object val)

    {

        local[key] = val;

    }


    public virtual void Close()

    {

        GC.SuppressFinalize(this);

        if (local != null) // intentionally using the field here, to avoid creating the instance

            local.Remove(key);

    }


    ~CloseableThreadLocal()

    {

        Console.WriteLine("close");

        Close();

    }

 }
configurator
12/18/2010 02:56 PM by
configurator

Why not instead of having a ThreadLocal dictionary with this as key, have an instance dictionary with the thread as key?

Alternatively, why not use a WeakReference as the key in the dictionary?

Rafal
12/18/2010 08:04 PM by
Rafal

Some idea for the fix: don't store a reference to ClosableThreadLocal class in the slots dictionary. Instead, in the constructor of ClosableThreadLocal, generate a key (GUID for example) and use that key for storing the value in slots Dictionary. In the finalizer of ClosableThreadLocal remove the key from slots.

Comments have been closed on this topic.