Ayende @ Rahien

It's a girl

Wrong Answer #2: Your own ThreadLocal

Originally posted at 12/15/2010

Well, last time we tried to introduce a finalizer, but we forgot that we were using our own instance as the key to the slots dictionary, which prevented us from being collected, hence we never run the finalizer.

All problems can be solved by adding an additional level of indirection… instead of using our own instance, let us use a separate instance:

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

    object holder = new object();
    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(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 (slots != null)// intentionally using the field here, to avoid creating the instance
            slots.Remove(holder);
    }

    ~CloseableThreadLocal()
    {
        Close();
    }
}

Now it should work, right?

Expect that it doesn’t… We still see that the data isn’t cleaned up properly, even though the finalizer is run.

Why?

Comments

ashic
12/19/2010 10:16 AM by
ashic

We seem to be setting it with the key as "holder" but when removing, we're doing slots.Remove(this). Could that be it?

tobi
12/19/2010 10:18 AM by
tobi

beautiful technique.

Ayende Rahien
12/19/2010 10:21 AM by
Ayende Rahien

Ashic,

Nope, that is a bug in the wrong example, fixed now

Inferis
12/19/2010 10:58 AM by
Inferis

The finalizer runs on another thread, so the slots field is null when the finalizer runs. The slots field on the original thread is never touched in the finalizer.

Ori
12/19/2010 12:15 PM by
Ori

but why a threadstatic static backing field? seems that the dictionary was suppose to id instances by key but the threadstatic attribute allocated a new backing field per thread..

Ayende Rahien
12/19/2010 12:25 PM by
Ayende Rahien

Ori,

Take a look at the class name

Ori
12/19/2010 01:33 PM by
Ori

ooops :)

Alois Kraus
12/19/2010 10:11 PM by
Alois Kraus

The idea is nice but he forgot to pass the context to make the finalizer work. This one would do the trick:

public class CloseableThreadLocal

{

    public class Closer

    {

        int Tid;

        public Closer(int id)

        {

            Tid = id;

        }


        ~Closer()

        {

            lock(CloseableThreadLocal.Slots)

            {

                if (CloseableThreadLocal.Slots.ContainsKey(this))

                    CloseableThreadLocal.Slots.Remove(this);

            }

        }


        public override bool Equals(object obj)

        {

            Closer c = obj as Closer;

            if (c == null)

                return false;

            return c.Tid == this.Tid;

        }


        public override int GetHashCode()

        {

            return Tid;

        }

    }


    internal static Dictionary

<closer,> slots = new Dictionary <closer,> ();

    public static Dictionary

<closer,> Slots

    {

        get { return slots; }

    }


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

    {

        return null;

    }


    public virtual Object Get()

    {

        object val;


        lock (Slots)

        {

            if (Slots.TryGetValue(new Closer(Thread.CurrentThread.ManagedThreadId), out val))

            {

                return val;

            }

        }

        val = InitialValue();

        Set(val);

        return val;

    }


    public virtual void Set(object val)

    {

        lock (Slots)

        {

            Slots[new Closer(Thread.CurrentThread.ManagedThreadId)] = val;

        }

    }


    public virtual void Close()

    {

        lock (Slots)

        {

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

            {

                slots.Remove(new Closer(Thread.CurrentThread.ManagedThreadId));

            }

        }

    }

}

Yes it does not scale that well because it uses a global dictionary but until you show me the requirement that thread thrashing is a problem it is out of scope.

Karan Misra
12/19/2010 11:40 PM by
Karan Misra

How about this guys? Any comments?

public class CloseableThreadLocal {

    private readonly string _threadKey = typeof(CloseableThreadLocal).FullName + Thread.CurrentThread.ManagedThreadId;

    private Dictionary

<object,> _slots;

    [ThreadStatic]

    internal static Dictionary

<object,> 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 (Slots.TryGetValue(_threadKey, out val)) {

            return val;

        }

        val = InitialValue();

        Set(val);

        return val;

    }


    public virtual void Set(object val) {

        Slots[_threadKey] = val;

        if (_slots == null) _slots = slots;

    }


    public virtual void Close() {

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

            _slots.Remove(_threadKey);

    }


    ~CloseableThreadLocal() {

        Close();

    }

}
Jared Kells
12/20/2010 01:50 AM by
Jared Kells

The finalizer runs in the GC thread.

You could change the dictionary to a concurrent dictionary and then keep a local reference to the one you are using in CloseableThreadLocal.

Something like this:

    private ConcurrentDictionary

<object,> _mySlots;

    private ConcurrentDictionary

<object,> MySlots

    {

        get { return _mySlots ?? (_mySlots = Slots); }

    }

In Get and Set use MySlots in Close() use _mySlots

Hendry Luk
12/20/2010 06:08 AM by
Hendry Luk

The CloseableThreadLocal will get collected, but the key (and the value) will still be referenced by the slots, and will therefore stay uncollected.

Why not use WeakReference? Isn't this the exact scenario WeakReference is intended for?

Karan Misra
12/20/2010 08:48 AM by
Karan Misra

How about this?

public class CloseableThreadLocal {

    [ThreadStatic]

    private static int _counter;


    [ThreadStatic]

    internal static Dictionary

<int,> _slots;

    public static Dictionary

<int,> Slots {

        get { return _slots ?? (_slots = new Dictionary

<int,> ()); }

    }


    private static Func

<int GenerateThreadKey = () => string.Format("{0}{1}", Thread.CurrentThread.ManagedThreadId, _counter).GetHashCode();

    private int _threadKey = GenerateThreadKey();


    private Dictionary

<int,>

_threadSlots;

    private Dictionary

<int,>

ThreadSlots {

        get {

            return _threadSlots ?? (_threadSlots = new Dictionary

<int,>

());

        }

    }


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

        return null;

    }


    public virtual Object Get() {

        object val;


        if (Slots.TryGetValue(_threadKey, out val)) {

            return val;

        }

        val = InitialValue();

        Set(val);

        return val;

    }


    public virtual void Set(object val) {

        Slots[_threadKey] = val;


        lock (this) {

            ThreadSlots[_threadKey] = _slots;

            _counter++;

        }

    }


    public virtual void Close() {

        if (_threadSlots != null) {

            foreach (var values in _threadSlots)

                values.Value.Remove(values.Key);

            _threadSlots = null;

        }

    }


    ~CloseableThreadLocal() {

        Close();

    }

}
Alois Kraus
12/20/2010 08:50 AM by
Alois Kraus

My Closable class needs a flag in the ctor to prevent the finalizer from beeing run when the thing is queried. Apart from this minor issue it should work.

Thomas Eyde
12/20/2010 10:18 AM by
Thomas Eyde

@Hendry, the Close() method removes the key and value, doesn't this mean they will be collected after all.

My first working solution collected all thread slots. That doesn't seem to be necessary, so I modified it. Just keeping a local reference makes the code simpler and removes the need for additional cleanup.

A fun exercise, by the way. Too bad I need so many iterations and hints before I get it right ;)

http://pastebin.com/d9HSepi5

Comments have been closed on this topic.