Ayende @ Rahien

Refunds available at head office

Wrong Answer #3: Your own Thread Local

Originally posted at 12/15/2010

Last time, we forgot that the slots dictionary is a thread static variable, and that the finalizer is going to run on another thread… Let us fix this, too:

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

    private readonly object holder = new object();
    private Dictionary<object, object> capturedSlots;

    private Dictionary<object, object> Slots
    {
        get
        {
            if (slots == null)
                slots = new Dictionary<object, object>();
            capturedSlots = slots;
            return slots;
        }
    }


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

    public virtual Object Get()
    {
        object val;

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

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

    public virtual void Close()
    {
        GC.SuppressFinalize(this);
        if (capturedSlots != null)
            capturedSlots.Remove(this);
    }

    ~CloseableThreadLocal()
    {
        if (capturedSlots == null)
            return;
        capturedSlots.Remove(holder);
    }
}

And now it works!

Except… Under some very rare scenarios, it will not do so.

What are those scenarios? Why do we care? And how do we fix this?

Comments

Patrick Huizinga
12/20/2010 10:11 AM by
Patrick Huizinga

When you access your CloseableThreadLocal from multiple threads, capturedSlots will only be the slots instance from the last thread. Thus on finalizing, all other threads will not be 'closed'.

Since the whole purpose of a *ThreadLocal is to use it from multiple threads I'd say the scenario isn't that rare.

Rodrigo Guerreiro
12/20/2010 10:15 AM by
Rodrigo Guerreiro

There could be some racing while building the slots and setting the slots more than one time and only the last one will win and it will be that one that is going to be captured.

if (slots == null)

            slots = new Dictionary

<object,> ();

we could do something like this:

if(slots == null)

{

 lock(syncObject)

 {

      if(slots == null)

      {

           slots = new Dictionary

<object,> ();

      }

 }

}

tobi
12/20/2010 10:16 AM by
tobi

And there is a race condition between the finalizer thread and other threads banging on that dictionary.

Patrick Huizinga
12/20/2010 11:01 AM by
Patrick Huizinga

@Rodrigo

__slots is a [ThreadStatic] field. Every thread gets it's own version of that field, which means you won't need locking for setting __slots .

Rodrigo Guerreiro
12/20/2010 11:41 AM by
Rodrigo Guerreiro

@Patrick Huizinga

UPS! You're right. I've missed that

Ori
12/20/2010 12:45 PM by
Ori

What about instancing the threadstatic static dictionary in the CloseableThreadLocal ctor to capture the thread the instance of CloseableThreadLocal was created on?

Patrick Huizinga
12/20/2010 01:46 PM by
Patrick Huizinga

Ori, it's not about which thread CloseableThreadLocal was created on, it's about all the threads that use the same instance ctl.

Chris Wright
12/20/2010 11:44 PM by
Chris Wright

So basically you keep a set of dictionaries on each instance, containing every dictionary they've been inserted into.

Better yet, keep a weak reference to those dictionaries. I assume that [ThreadLocal] is smart enough to prevent the GC from collecting any of the referenced items while the thread is alive, and to stop preventing the GC from collecting those items when the thread dies. If you use a strong reference, a dictionary for a thread that has died will stay in memory as long as at least one ClosableThreadLocal which has been accessed from that thread is alive, just to do unnecessary cleanup.

If you have concurrent collection enabled, you have problems. Your destructors might run while another thread is modifying the dictionary. You need a dictionary that can handle updates from multiple threads correctly without locking. Perhaps it would mediate updates through a dictionary of Thread => Queue, and you automatically drop updates marked with threads other than explicitly registered threads (where registering a thread requires locking).

Ugh, this is spiraling out of control... there has to be another approach that will work better.

tytusse
12/21/2010 12:43 AM by
tytusse

Cant see how would this work without ordinary locking - this scenario always leads to the point, where you have to access some sort of thread-shared resource.

ThreadStatic is cleaned up after thread dies, so it is no problem with it not being cleaned up (there is only one reference to it). When thread dies, all data within it's scope gets cleaned up nicely.

The problem shows up, when you create lots of objects in the scope of single thread, because their values are stuck inside thread-static dictionary until the end of this thread - if it's long enough or object quantity is high enough - you have a problem.

I also don't know how WeakReference can help here, because all it can do is set it's target (stored) to null when GC "has a good mood" for that. So you end up with locking :-(

This is as far as I could get, it's ugly when you create lots of objects, that are dropped immediately, because it slows down finalizer on static locking.... It also keeps all thread-static dictionaries referenced inside instances ... but it at least makes it possible to get to the point, where all is cleaned up:

public class CloseableThreadLocal

{

    [ThreadStatic]

    public static Dictionary

<object,> slots;

    private static object knownSlotsSyncRoot = new object();


    private readonly object holder = new object();


    private readonly List

<dictionary<object,>

knownSlots =

        new List

<dictionary<object,>

();

    private Dictionary

<object,> Slots

    {

        get

        {

            if (slots == null)

            {

                lock (knownSlotsSyncRoot)

                {

                    slots = new Dictionary

<object,> ();

                    knownSlots.Add(slots);

                }

            }


            return slots;

        }

    }



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

    {

        return null;

    }


    public virtual Object Get()

    {

        object val;


        if (Slots.TryGetValue(holder, out val))

        {

            return val;

        }

        val = InitialValue();

        Set(val);

        return val;

    }


    public virtual void Set(object val)

    {

        Slots[holder] = val;

    }


    public virtual void Close()

    {

        GC.SuppressFinalize(this);

        lock (knownSlotsSyncRoot)

        {

            foreach (var slot in knownSlots)

            {

                slot.Remove(holder);

            }


            slots = null;

        }


    }


    ~CloseableThreadLocal()

    {

        lock (knownSlotsSyncRoot)

        {

            foreach (var slot in knownSlots)

            {

                slot.Remove(holder);

            }

        }

    }

}
Comments have been closed on this topic.