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

filter by tags archive

AnswerYour own ThreadLocal

time to read 4 min | 616 words

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?

More posts in "Answer" series:

  1. (16 Aug 2011) Modifying execution approaches
  2. (30 Apr 2011) Stopping the leaks
  3. (24 Dec 2010) This code should never hit production
  4. (21 Dec 2010) Your own ThreadLocal
  5. (11 Feb 2010) Debugging a resource leak
  6. (03 Sep 2009) The lazy loaded inheritance many to one association OR/M conundrum
  7. (04 Sep 2008) Don't stop with the first DSL abstraction
  8. (12 Jun 2008) How many tests?

Comments

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

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

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

"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

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

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

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

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

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

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

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

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

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

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

@Nadav

"hello there1" finalized on program exit

and i'm in debug mode

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

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

@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

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

hazzik

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

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

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

And 4 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):
    31 Aug 2015 - The case of the memory eater and high load
  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