Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 18 | Comments: 66

filter by tags archive

Wrong answer #1: Your own ThreadLocal

time to read 3 min | 597 words

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

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

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

Koen

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

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

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

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

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

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.

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. RavenDB 3.0 New Stable Release - one day from now
  2. Production postmortem: The industry at large - about one day from now
  3. The insidious cost of allocations - 3 days from now
  4. Buffer allocation strategies: A possible solution - 6 days from now
  5. Buffer allocation strategies: Explaining the solution - 7 days from now

And 3 more posts are pending...

There are posts all the way to Sep 11, 2015

RECENT SERIES

  1. Find the bug (5):
    20 Apr 2011 - Why do I get a Null Reference Exception?
  2. Production postmortem (10):
    01 Sep 2015 - The case of the lying configuration file
  3. What is new in RavenDB 3.5 (7):
    12 Aug 2015 - Monitoring support
  4. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats