Ayende @ Rahien

Thomas Eyde commented on Wrong Answer #2: Your own ThreadLocal

Mon, 20 Dec 2010 10:18:05 GMT

@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](http://pastebin.com/d9HSepi5)

Alois Kraus commented on Wrong Answer #2: Your own ThreadLocal

Mon, 20 Dec 2010 08:50:03 GMT

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.

Karan Misra commented on Wrong Answer #2: Your own ThreadLocal

Mon, 20 Dec 2010 08:48:50 GMT

How about this? public class CloseableThreadLocal { [ThreadStatic] private static int _counter; [ThreadStatic] internal static Dictionary _slots; public static Dictionary Slots { get { return _slots ?? (_slots = new Dictionary ()); } } private static Func string.Format("{0}{1}", Thread.CurrentThread.ManagedThreadId, _counter).GetHashCode(); private int _threadKey = GenerateThreadKey(); private Dictionary > _threadSlots; private Dictionary > ThreadSlots { get { return _threadSlots ?? (_threadSlots = new Dictionary >()); } } 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(); } } >

Hendry Luk commented on Wrong Answer #2: Your own ThreadLocal

Mon, 20 Dec 2010 06:08:03 GMT

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?

Jared Kells commented on Wrong Answer #2: Your own ThreadLocal

Mon, 20 Dec 2010 01:50:22 GMT

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 _mySlots; private ConcurrentDictionary MySlots { get { return _mySlots ?? (_mySlots = Slots); } } In Get and Set use MySlots in Close() use _mySlots

Karan Misra commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 23:40:14 GMT

How about this guys? Any comments? public class CloseableThreadLocal { private readonly string _threadKey = typeof(CloseableThreadLocal).FullName + Thread.CurrentThread.ManagedThreadId; private Dictionary _slots; [ThreadStatic] internal static Dictionary 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 (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(); } }

Alois Kraus commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 22:11:15 GMT

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 slots = new Dictionary (); public static Dictionary 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.

Ori commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 13:33:15 GMT

ooops :)

Ayende Rahien commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 12:25:10 GMT

Ori, Take a look at the class name

Ori commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 12:15:06 GMT

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

Augi commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 11:06:38 GMT

You could use WeakReference class instead of own "holder": [msdn.microsoft.com/.../system.weakreference.aspx](http://msdn.microsoft.com/en-us/library/system.weakreference.aspx)

Inferis commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 10:58:36 GMT

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.

Ayende Rahien commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 10:21:20 GMT

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

tobi commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 10:18:06 GMT

beautiful technique.

ashic commented on Wrong Answer #2: Your own ThreadLocal

Sun, 19 Dec 2010 10:16:19 GMT

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