Ayende @ Rahien

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

ayende@ayende.com

+972 52-548-6969

@

Posts: 5,947 | Comments: 44,541

filter by tags archive

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

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

No future posts left, oh my!

RECENT SERIES

  1. RavenDB Sharding (3):
    22 May 2015 - Adding a new shard to an existing cluster, splitting the shard
  2. The RavenDB Comic Strip (2):
    20 May 2015 - Part II – a team in trouble!
  3. Challenge (45):
    28 Apr 2015 - What is the meaning of this change?
  4. Interview question (2):
    30 Mar 2015 - fix the index
  5. Excerpts from the RavenDB Performance team report (20):
    20 Feb 2015 - Optimizing Compare – The circle of life (a post-mortem)
View all series

RECENT COMMENTS

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats