30

When you use a ThreadLocal<T> and T implements IDisposable, how are you supposed to dispose of the members being held inside of the ThreadLocal?

According to ILSpy, the Dispose() and Dispose(bool) methods of ThreadLocal are

public void Dispose()
{
    this.Dispose(true);
    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
    int currentInstanceIndex = this.m_currentInstanceIndex;
    if (currentInstanceIndex > -1 && Interlocked.CompareExchange(ref this.m_currentInstanceIndex, -1, currentInstanceIndex) == currentInstanceIndex)
    {
        ThreadLocal<T>.s_availableIndices.Push(currentInstanceIndex);
    }
    this.m_holder = null;
}

It does not appear that ThreadLocal attempts to call Dispose on its child members. I can't tell how to reference each thread it internally has allocated so I can take care of it.


I ran a test with the following code, the class is never disposed

static class Sandbox
{
    static void Main()
    {
        ThreadLocal<TestClass> test = new ThreadLocal<TestClass>();
        test.Value = new TestClass();

        test.Dispose();
        Console.Read();
    }
}

class TestClass : IDisposable
{
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    protected void Dispose(bool Disposing)
    {
        Console.Write("I was disposed!");
    }
}
Pang
  • 9,564
  • 146
  • 81
  • 122
Scott Chamberlain
  • 124,994
  • 33
  • 282
  • 431
  • Dispose method is just like a normal method. Once you call test.Dispose() in your example it must execute the Dispose method in the TestClass. If not, that's strange. BTW, IDisposable implementation is not correct. Look at here http://msdn.microsoft.com/en-us/library/b1yfkh5e.aspx – CharithJ Oct 06 '11 at 02:53
  • 1
    Note that that pattern is only required if your class has unmanaged resources. Otherwise don't add finalizers as they add overhead on class creation and on finalizing. – Just another metaprogrammer Oct 06 '11 at 05:27
  • In .Net 4.5, you can tell the `ThreadLocal` to track all `Value`s, which makes it easy to dispose them manually: [New in .NET 4.5: ThreadLocal.Values](http://blogs.msdn.com/b/pfxteam/archive/2011/11/11/10236000.aspx) – Sphinxxx Apr 29 '14 at 01:47

6 Answers6

14

I had a look at the code in ThreadLocal<T> to see what the current Dispose is doing and it appears to be a lot of voodoo. Obviously disposing of thread-related stuff.

But it doesn't dispose of the values if T itself is disposable.

Now, I have a solution - a ThreadLocalDisposables<T> class, but before I give the full definition it's worth thinking about what should happen if you wrote this code:

var tl = new ThreadLocalDisposables<IExpensiveDisposableResource>();
tl.Value = myEdr1;
tl.Value = myEdr2;
tl.Dispose();

Should both myEdr1 & myEdr2 both be disposed? Or just myEdr2? Or should myEdr1 be disposed when myEdr2 was assigned?

It's not clear to me what the semantics should be.

It is clear to me, however, that if I wrote this code:

var tl = new ThreadLocalDisposables<IExpensiveDisposableResource>(
    () => new ExpensiveDisposableResource());
tl.Value.DoSomething();
tl.Dispose();

Then I would expect that the resource created by the factory for each thread should be disposed of.

So I'm not going to allow the direct assignment of the disposable value for ThreadLocalDisposables and only allow the factory constructor.

Here's ThreadLocalDisposables:

public class ThreadLocalDisposables<T> : IDisposable
    where T : IDisposable
{
    private ThreadLocal<T> _threadLocal = null;
    private ConcurrentBag<T> _values = new ConcurrentBag<T>();

    public ThreadLocalDisposables(Func<T> valueFactory)
    {
        _threadLocal = new ThreadLocal<T>(() =>
        {
            var value = valueFactory();
            _values.Add(value);
            return value;
        });
    }

    public void Dispose()
    {
        _threadLocal.Dispose();
        Array.ForEach(_values.ToArray(), t => t.Dispose());
    }

    public override string ToString()
    {
        return _threadLocal.ToString();
    }

    public bool IsValueCreated
    {
        get { return _threadLocal.IsValueCreated; }
    }

    public T Value
    {
        get { return _threadLocal.Value; }
    }
}

Does this help?

Enigmativity
  • 113,464
  • 11
  • 89
  • 172
8

In .NET 4.5, the Values property was added to ThreadLocal<> to deal with the problem of manually managing the lifetime of ThreadLocal objects. It returns a list of all current instances bound to that ThreadLocal variable.

An example using a Parallel.For loop accessing a ThreadLocal database connection pool was presented in this MSDN article. The relevant code snippet is below.

var threadDbConn = new ThreadLocal<MyDbConnection>(() => MyDbConnection.Open(), true);
try
{
    Parallel.For(0, 10000, i =>
    {
        var inputData = threadDbConn.Value.GetData(i);
        ...
    });
}
finally
{
    foreach(var dbConn in threadDbConn.Values)
    {
        dbConn.Close();
    }
}
Pang
  • 9,564
  • 146
  • 81
  • 122
Chuu
  • 4,301
  • 2
  • 28
  • 54
2

Normally when you don't explicitly dispose of a class that holds an unmanaged resource, the garbage collector will eventually run and dispose of it. For this to happen, the class has to have a finalizer that disposes of its resource. Your sample class doesn't have a finalizer.

Now, to dispose of a class that's held inside a ThreadLocal<T> where T is IDisposable you also have to do it yourself. ThreadLocal<T> is just a wrapper, it won't attempt to guess what's the correct behavior for its wrapped reference when it is itself disposed. The class could, e.g., survive its thread local storage.

Jordão
  • 55,340
  • 13
  • 112
  • 144
  • 1
    So what should be done when you use a ThreadLocal when the object implements IDisposeable and it is not a simple case like my example, "just let the finailizer take care of it"? That seems wrong to me. Also, if I had a easy way to track the object's lifetime so I could call Dispose by hand I would not be using ThreadLocal, I would just track it directly. – Scott Chamberlain Oct 06 '11 at 03:25
  • 2
    @ScottChamberlain - It's a bad idea to let the finalizer take care of it. There is no guarantee that the finalizer will ever run. And you **must** explicitly dispose of your objects in your code. You probably need to wrap `ThreadLocal` to get what you need. – Enigmativity Oct 06 '11 at 05:36
  • I think this answer is unclear. If a class implements `IDisposable` I most definitively think, when writing proper code, it should be disposed; we should not rely on the finalizer. If we *should* rely on the finalizer of a class the creator of the class *should not* expose a `Disposabe` method. – mortb Nov 02 '18 at 12:44
  • The finalizer is a fallback, since you cannot usually really on consumers writing the _proper_ code. But it's only needed if you actually hold unmanaged resources. – Jordão Nov 02 '18 at 13:51
  • (Edited the text to clarify that a finalizer is only needed for unmanaged resources....) – Jordão Nov 02 '18 at 13:54
1

MSDN reference states that the ThreadLocal values should be disposed by the thread using them once its done. However in some instances such as event threading using a thread pool A thread may use the value and go off to do something else and then come back to the value N number of times.

Specific example is where I want an Entity Framework DBContext to persist across the lifespan of a series of service bus worker threads.

I've written up the following class which I use in these instances: Either DisposeThreadCompletedValues can be called manually every so often by another thread or the internal monitor thread can be activated

Hopefully this helps?

using System.Threading;
public class DisposableThreadLocal<T> : IDisposable
    where T : IDisposable
{
    public DisposableThreadLocal(Func<T> _ValueFactory)
    {
        Initialize(_ValueFactory, false, 1);
    }
    public DisposableThreadLocal(Func<T> _ValueFactory, bool CreateLocalWatcherThread, int _CheckEverySeconds)
    {
        Initialize(_ValueFactory, CreateLocalWatcherThread, _CheckEverySeconds);
    }

    private void Initialize(Func<T> _ValueFactory, bool CreateLocalWatcherThread, int _CheckEverySeconds)
    {
        m_ValueFactory = _ValueFactory;
        m_CheckEverySeconds = _CheckEverySeconds * 1000;
        if (CreateLocalWatcherThread)
        {
            System.Threading.ThreadStart WatcherThreadStart;
            WatcherThreadStart = new ThreadStart(InternalMonitor);
            WatcherThread = new Thread(WatcherThreadStart);
            WatcherThread.Start();
        }
    }

    private object SyncRoot = new object();

    private Func<T> m_ValueFactory;
    public Func<T> ValueFactory
    {
        get
        {
            return m_ValueFactory;
        }
    }

    private Dictionary<Thread, T> m_InternalDict = new Dictionary<Thread, T>();
    private Dictionary<Thread, T> InternalDict
    {
        get
        {
            return m_InternalDict;
        }
    }

    public T Value
    {
        get
        {
            T Result;
            lock(SyncRoot)
            {
                if (!InternalDict.TryGetValue(Thread.CurrentThread,out Result))
                {
                    Result = ValueFactory.Invoke();
                    InternalDict.Add(Thread.CurrentThread, Result);
                }
            }
            return Result;
        }
        set
        {
            lock (SyncRoot)
            {
                if (InternalDict.ContainsKey(Thread.CurrentThread))
                {
                    InternalDict[Thread.CurrentThread] = value;
                }
                else
                {
                    InternalDict.Add(Thread.CurrentThread, value);
                }
            }
        }
    }

    public bool IsValueCreated
    {
        get
        {
            lock (SyncRoot)
            {
                return InternalDict.ContainsKey(Thread.CurrentThread);
            }
        }
    }

    public void DisposeThreadCompletedValues()
    {
        lock (SyncRoot)
        {
            List<Thread> CompletedThreads;
            CompletedThreads = new List<Thread>();
            foreach (Thread ThreadInstance in InternalDict.Keys)
            {
                if (!ThreadInstance.IsAlive)
                {
                    CompletedThreads.Add(ThreadInstance);
                }
            }
            foreach (Thread ThreadInstance in CompletedThreads)
            {
                InternalDict[ThreadInstance].Dispose();
                InternalDict.Remove(ThreadInstance);
            }
        }
    }

    private int m_CheckEverySeconds;
    private int CheckEverySeconds
    {
        get
        {
            return m_CheckEverySeconds;
        }
    }

    private Thread WatcherThread;

    private void InternalMonitor()
    {
        while (!IsDisposed)
        {
            System.Threading.Thread.Sleep(CheckEverySeconds);
            DisposeThreadCompletedValues();
        }
    }

    private bool IsDisposed = false;
    public void Dispose()
    {
        if (!IsDisposed)
        {
            IsDisposed = true;
            DoDispose();
        }
    }
    private void DoDispose()
    {
        if (WatcherThread != null)
        {
            WatcherThread.Abort();
        }
        //InternalDict.Values.ToList().ForEach(Value => Value.Dispose());
        foreach (T Value in InternalDict.Values)
        {
            Value.Dispose();
        }
        InternalDict.Clear();
        m_InternalDict = null;
        m_ValueFactory = null;
        GC.SuppressFinalize(this);
    }
}
tcwicks
  • 495
  • 3
  • 11
1

This is related to ThreadLocal<> and memory leak

My guess is because there is no IDisposable constraint on T, it is assumed that the user of ThreadLocal<T> will dispose of the local object, when appropriate.

Community
  • 1
  • 1
Igor Pashchuk
  • 2,455
  • 2
  • 22
  • 29
1

How is the ThreadLocal.Dispose method itself getting called? I would expect that it would most likely be within something like a "using" block. I would suggest that one wrap the "using" block for the ThreadLocal with a "using" block for the resource that's going to be stored there.

supercat
  • 77,689
  • 9
  • 166
  • 211