Ayende @ Rahien

Rafal commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 20:04:34 GMT

Some idea for the fix: don't store a reference to ClosableThreadLocal class in the slots dictionary. Instead, in the constructor of ClosableThreadLocal, generate a key (GUID for example) and use that key for storing the value in slots Dictionary. In the finalizer of ClosableThreadLocal remove the key from slots.

configurator commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 14:56:12 GMT

Why not instead of having a ThreadLocal dictionary with this as key, have an instance dictionary with the thread as key? Alternatively, why not use a WeakReference as the key in the dictionary?

hazzik commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 12:28:11 GMT

public class CloseableThreadLocal { [ThreadStatic] private static Dictionary slots; private readonly object key = new object(); private readonly Dictionary local = Slots; public static Dictionary Slots { get { return slots ?? (slots = new Dictionary ()); } } public /*protected internal*/ virtual Object InitialValue() { return null; } public virtual Object Get() { object val; if (local.TryGetValue(key, out val)) return val; val = InitialValue(); Set(val); return val; } public virtual void Set(object val) { local[key] = val; } public virtual void Close() { GC.SuppressFinalize(this); if (local != null) // intentionally using the field here, to avoid creating the instance local.Remove(key); } ~CloseableThreadLocal() { Console.WriteLine("close"); Close(); } }

Daniel Grunwald commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 12:06:10 GMT

Yes, using a WeakReference as key will do the same job (and initially I was also thinking of using one); but would be pretty strange as the dictionary will use reference equality of the WeakReference, and never look at what the weak reference is pointing to. So whether your key is a weak reference or a simple "readonly object key = new object();" doesn't matter.

Koen commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 12:02:11 GMT

I guess a WeakReference might also do the job? (not sure)

Daniel Grunwald commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 12:00:38 GMT

In this wrong answer, the finalizer doesn't run at all; because the object is still referenced by the dictionary. To fix this issue, don't use the CloseableThreadLocal as key; but create a separate key object within the CloseableThreadLocal. This prevents the CloseableThreadLocal from being reachable via the dictionary keys. However it might still be reachable from a value in the dictionary - see my "class Leak" and comments about ConditionalWeakTable on the previous blog post. Once you get the finalizer running, you still have to ensure that you remove the entry from *all* dictionaries (the ThreadLocal might have been used on multiple threads). As it is, the code removes the entry only from the finalizer thread's dictionary. A static List of all dictionaries might help; but then you'll have to think of something to remove the dictionaries from that list when the corresponding thread exits.

Koen commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 11:52:06 GMT

I'd think the finalizer would never be executed in the case that slots holds a reference to the current object.

commenter commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 10:28:36 GMT

Maybe slots dictionaries could be registered in a lookup table by thread ID, for retrievability in the finalizer.

commenter commented on Wrong answer #1: Your own ThreadLocal

Sat, 18 Dec 2010 10:22:30 GMT

The finalizer runs on a different thread, so works on a whole other threadstatic 'slots' than the one you want to clean up. As for a solution, I havent thought of one yet :)