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.

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

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.

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

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

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

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.

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());

  }

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?

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

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

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

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.

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

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?

@Nadav

"hello there1" finalized on program exit

and i'm in debug mode

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

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

@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);


    }

}

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

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