Ayende @ Rahien

Refunds available at head office

Answer: Your own ThreadLocal

Originally posted at 12/15/2010

Well, the problem on our last answer was that we didn’t protect ourselves from multi threaded access to the slots variable. Here is the code with this fixed:

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;

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

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

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

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

Is this it? Are there still issues that we need to handle?

Comments

configurator
12/21/2010 10:41 AM by
configurator

I don't get it... The capturedSlots doesn't seem to help you at all here. It would just return the last dictionary used i.e. the one for the last thread. How does this solve your problem at all? This doesn't seem to handle any multi threading scenarios.

Unless I'm missing something big, this isn't much help at all.

krlm
12/21/2010 10:54 AM by
krlm

I don't get it. If destructor is processed in the finalizer thread then when capturedSlots could be different from null? Should capturedSlots be static? Regards

Daniel Grunwald
12/21/2010 11:02 AM by
Daniel Grunwald

This answer is worse than the previous one.

Your lock is completely useless. No other thread could access the same dictionary (because it's thread-local!), so you don't need any lock there.

All the actual issues with the previous answer still exist in this version.

Caveat: the GC might collect the CloseableThreadLocal early, leading to concurrent access; but a call to GC.KeepAlive(this); (at the end of the Get/Set methods) would be a way better solution than locking.

Dennis
12/21/2010 11:20 AM by
Dennis

"Your lock is completely useless. No other thread could access the same dictionary (because it's thread-local!), so you don't need any lock there."

It is still shared among all CloseableThreadLocal instances on that particular thread.

Drazen Dotlic
12/21/2010 11:22 AM by
Drazen Dotlic

Shouldn't the last line in the "Close" method read: 'capturedSlots.Remove(holder);'?

(not 'capturedSlots.Remove(this);')

Dennis
12/21/2010 11:22 AM by
Dennis

This is not good enough. If you share an instance of CloseableThreadLocal between many threads, it will only remember the last thread it was accessing Slots on. You need a list or hashset or something that can remember ALL threads it was accessing.

Dennis
12/21/2010 11:33 AM by
Dennis

Here an example of a usecase that still leaks with the above code:

  static void Main(string[] args)

  {

     var tl = new CloseableThreadLocal();

     Test(tl);

     var thread = new Thread(() => Test(tl));

     thread.Start();

     thread.Join();

     tl = null;

     GC.Collect();

     GC.WaitForPendingFinalizers();


     Console.WriteLine(CloseableThreadLocal.slots.Count);

  }


  private static void Test(CloseableThreadLocal tl)

  {

     tl.Set("hello there");

     Console.WriteLine(tl.Get());

  }
Thomas Eyde
12/21/2010 11:41 AM by
Thomas Eyde

If you create an instance, but never call Set(), then capturedSlots will be null.

The instance could also be passed to different threads. If the finalizer is guaranteed to run only once, then a lock there should not be necessary.

Also, in the multi thread usage, we will only track the very first dictionary and not the following ones. This will lead to a memory leak, but this is clearly not the expected usage, so should we care?

Then there is the lock in Get() and Set(). I don't see the need for that, either. But then again, I needed multiple iterations and several hints before I managed to get a working solution. Will someone explain, please?

Adam Straughan
12/21/2010 03:07 PM by
Adam Straughan

I don't see the point of the lock, there can be many instances on a thread but obviously only one can access the slot at a time.

The bigger issue is lack of clarity on the requirements regarding cross thread usage. Writing my own implementation showed me that without a clear requirement on which thread a slot entry should be stored (creation, first set, second set, etc.) it is not possible to know if it is fully solved, though my test scenarios all worked with no leak [main thread only, background thread only, create on main set on background 2 (get ok on background 2 but obviously fails on main thread)]

Note that when running through the debugger the last leak count shows because the last finalizer has not run, through the console you get the full picture

tobi
12/21/2010 04:40 PM by
tobi
The finalizer only clears the dict of only one thread. Other threads still leak. Unit tests can easily miss this when that particular scenario is not covered.
  
I also wonder about the overhead of locking although the lock will mostly be on a thread local object and uncontended making it relatively cheap.
  
The final solution would need to track a set of all dictionaries where values have ever been stored in. My tip: Just use the LocalDataStoreSlot class. It seems to do fine performancewise.
  
The ThreadLocal
<t in the BCL uses a threadstatic field on a generic typ Holder
<a,b,c>
. A, B and C are filled by reflection with one of 16 dummy classes so there are 4096 slots available that use the fast threadstatic attribute. When that limit is exceeded, LocalDataStoreSlot is used.
>
Alois Kraus
12/21/2010 04:56 PM by
Alois Kraus

If you want to make your Threadlocal non thread local because the finalizer thread should take care to clean up you end up with a global non thread local list. Any solution that tries to work around this will either leak data (like two previous approaches) or have to use a global list that is shared between all threads.

I fear that a working solution which does not rely on other threads (e.g. Finalizer) to clean up needs to use WeakReferences to make it clean.

Yours,

Alois Kraus

Nadav
12/21/2010 05:24 PM by
Nadav

The lock is useless because the slots is thread static, and therefor, even if it is shared among more than one instances of the class, they cannot access the same slots var, as it exists in one thread! so no concurrent access is possible, isn't it?

there is a problem with the capturedSlots, because when an instance X is used by a few threads, it will only contain the slots instance of the last thread that used it, which means that the slots variable might still contain instances from threads that are not the last thread that used that instance.

what you can do is this:

private List <dictionary<object,>

capturedSlots

then in the d'tor do this:

foreach (Dictionary <object,object> captured in capturedSlots)

{

captured .Remove(holder);

}

and when the slots var is accessed, if it isn't contained in the list of captured slots, add it. Then in the d'tor you can be sure that all the slots variable remove the instance that is currently being finalized.

hazzik
12/21/2010 09:28 PM by
hazzik

that answer was presented twice

ayende.com/.../challenge-your-own-threadlocal.aspx
ayende.com/.../...swer-1-your-own-threadlocal.aspx

and there was an issue:

internal class MyClass

{

    private readonly string value;


    public MyClass(string value)

    {

        this.value = value;

    }


    ~MyClass()

    {

        Console.WriteLine("finalized {0}", value);

    }

    public override string ToString()

    {

        return value;

    }

}

internal class Program

{

    private static void Main(string[] args)

    {

        UseThreadLocal();

        GC.Collect();

        GC.WaitForPendingFinalizers();

        Console.WriteLine(CloseableThreadLocal.slots.Count);

        Console.ReadKey();

    }


    private static void UseThreadLocal()

    {

        var local = new CloseableThreadLocal();

        UseThreadLocal(new MyClass("hello there1"), local);

        new Thread(() => UseThreadLocal(new MyClass("hello there2"), local)).Start();

        new Thread(() => UseThreadLocal(new MyClass("hello there3"), local)).Start();

        new Thread(() => UseThreadLocal(new MyClass("hello there4"), local)).Start();

        Thread.Sleep(1000);

    }


    private static void UseThreadLocal(object helloThere, CloseableThreadLocal tl)

    {

        tl.Set(helloThere);

    }

}

last object is not disposed and presented in dictionary

Nadav
12/21/2010 10:09 PM by
Nadav

Don't know about you but with my code and you're example, everything gets disposed, the dictionary has 0 data at the end, and there's no reason why it wouldn't.

something to notice is that the instace of MyClass(hello there1)

gets disposed in the next cycle of the GC:

UseThreadLocal();

GC.Collect();

GC.WaitForPendingFinalizers();

Console.WriteLine(CloseableThreadLocal.slots.Count);

generates these console writes:

finalized hello there4

finalized hello there3

finalized hello there2

0

finalized hello there1

but as you can see, everything is disposed and the slots in the main thread are 0.

do you have a different use case where it fails?

hazzik
12/23/2010 05:03 AM by
hazzik

@Nadav

"hello there1" finalized on program exit

and i'm in debug mode

hazzik
12/23/2010 05:11 AM by
hazzik

code:

UseThreadLocal();

GC.Collect(int.MaxValue);

GC.WaitForPendingFinalizers();

GC.Collect(int.MaxValue);

GC.WaitForPendingFinalizers();

Console.WriteLine(CloseableThreadLocal.slots.Count);

Console.WriteLine("Press any key");

Console.ReadKey();

my output in debug and release mode:

finalized hello there4

finalized hello there3

finalized hello there2

1

Press any key

finalized hello there1

hazzik
12/23/2010 05:19 AM by
hazzik

I've tried with .net4, .net3.5 and .net2.0 - nothing changed

I've tried to run this code snippet in different AppDomain - nothing changed

Nadav
12/23/2010 05:49 AM by
Nadav

@hazzik

my output is:

finalized hello there4

finalized hello there3

finalized hello there2

finalized hello there1

0

when i collect and wait for pending finalizers twice.

did you remember to lock the capturedSlots when you add a new slots?

anyway, it does collect it in the next cycle:

public class CloseableThreadLocal

{

    [ThreadStatic]

    public static Dictionary

<object,> slots;

    private readonly object holder = new object();

    private List

<dictionary<object,>

capturedSlots;

    private Dictionary

<object,> Slots

    {

        get

        {

            if (capturedSlots == null)

            {

                lock (this)

                {

                    if (capturedSlots == null)

                    {

                        capturedSlots = new List

<dictionary<object,>

();

                    }

                }

            }


            if (slots == null)

            {

                slots = new Dictionary

<object,> ();

                lock (capturedSlots)

                {

                    capturedSlots.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);

        if (capturedSlots != null)

        {

            foreach (Dictionary

<object,> dic in capturedSlots)

            {

                lock (dic)

                {

                    dic.Remove(holder);

                }

            }


        }

    }


    ~CloseableThreadLocal()

    {

        if (capturedSlots == null)

            return;


        foreach (Dictionary

<object,> dic in capturedSlots)

        {

            lock (dic)

            {

                dic.Remove(holder);

            }

        }

    }

}



internal class MyClass

{

    private readonly string value;


    public MyClass(string value)

    {

        this.value = value;

    }


    ~MyClass()

    {

        Console.WriteLine("finalized {0}", value);

    }

    public override string ToString()

    {

        return value;

    }

}

internal class Program

{

    private static void Main(string[] args)

    {

        UseThreadLocal();

        GC.Collect();

        GC.WaitForPendingFinalizers();

        GC.Collect();

        GC.WaitForPendingFinalizers();

        Console.WriteLine(CloseableThreadLocal.slots.Count);

        Console.ReadLine();

    }


    private static void UseThreadLocal()

    {

        var local = new CloseableThreadLocal();


        UseThreadLocal(new MyClass("hello there1"), local);

        new Thread(() => UseThreadLocal(new MyClass("hello there2"), local)).Start();

        new Thread(() => UseThreadLocal(new MyClass("hello there3"), local)).Start();

        new Thread(() => UseThreadLocal(new MyClass("hello there4"), local)).Start();



        Thread.Sleep(1000);

    }


    private static void UseThreadLocal(object helloThere, CloseableThreadLocal tl)

    {

        tl.Set(helloThere);


    }

}
Nadav
12/23/2010 05:50 AM by
Nadav

it is also important to notice that the double check if capturedSlots is null is critical here

hazzik
12/23/2010 06:25 AM by
hazzik

Nadav i've checked Ayende's example, not yours

Comments have been closed on this topic.