What is the correct way of adding thread-safety to an IDisposable object?

Question

Imagine an implementation of the IDisposable interface, that has some public methods.

If an instance of that type is shared between multiple threads and one of the threads may dispose it, what is the best way to ensure the other threads do not attempt to work with the instance after disposed? In most cases, after the object is disposed, its methods must be aware of it and throw the ObjectDisposedException or maybe InvalidOperationException or at least inform the calling code for doing something wrong. Do I need synchronization for every method - particularly around the check if it is disposed? Do all IDisposable implementations with other public methods need to be thread-safe?

---

Here is an example:

public class DummyDisposable : IDisposable
{
    private bool _disposed = false;

    public void Dispose()
    {
        _disposed = true;
        // actual dispose logic
    }

    public void DoSomething()
    {
        // maybe synchronize around the if block?
        if (_disposed)
        {
            throw new ObjectDisposedException("The current instance has been disposed!");
        }

        // DoSomething logic
    }

    public void DoSomethingElse()
    {
         // Same sync logic as in DoSomething() again?
    }
}

Answer 1

The simplest thing you can do is mark the private disposed variable as volatile and inspect it at the beginning of your methods. You can then throw an ObjectDisposedException if the object has already been disposed.

There are two caveats to this:

1. You shouldn't throw an ObjectDisposedExceptionif the method is an event handler. Instead you should just gracefully exit from the method if that is possible. The reason being is that there exists a race condition where events can be raised after you unsubscribe from them. (See this article by Eric Lippert for more information.)

2. This doesn't stop your class from being disposed while you are in the middle of executing one of your class methods. So if your class has instance members that can't be accessed after disposal, you're going to need to setup some locking behaviour to ensure access to these resources are controlled.

Microsoft's guidance around IDisposable says you should check for disposed on all methods, but I haven't personally found this necessary. The question really is, is something going to throw an exception or cause unintended side effects if you allow a method to execute after the class is disposed. If the answer is yes, you need to do some work to make sure that doesn't happen.

In terms of whether all IDisposable classes should be thread safe: No. Most of the use cases for disposable classes involve them only ever being accessed by a single thread.

That being said, you may want to investigate why you need your disposable class to be thread safe as it adds a lot of additional complexity. There may be an alternate implementation that allows you to not have to worry about thread safety issues in your disposable class.

Answer 2

I tend to use an integer rather than a boolean as your field for storing the disposed status, because then you can use the thread-safe Interlocked class to test if Dispose has already been called.

Something like this:

private int _disposeCount;

public void Dispose()
{
    if (Interlocked.Increment(ref _disposeCount) == 1)
    {
        // disposal code here
    }
}

This ensures that the disposal code is called only once not matter how many times the method is called, and is totally thread safe.

Then each method can quite simply use call this method as a barrier check:

private void ThrowIfDisposed()
{
   if (_disposeCount > 0) throw new ObjectDisposedException(GetType().Name);
}

With regard to synchronising every method - are you saying a simple barrier check won't do - that you want to stop other threads that might be already executing code in the instance. This is a more complex problem. I don't know what your code is doing, but consider if you really need that - will a simple barrier check not do?

If you just meant with regard to the disposed check itself - my example above is fine.

EDIT: to answer the comment "What's the difference between this and a volatile bool flag? It's slightly confusing to have a field named somethingCount and allow it to hold 0 and 1 values only"

Volatile is related to ensuring the read or write operation operation is atomic and safe. It doesn't make the process of assigning and checking a value thread safe. So, for instance, the following is not thread safe despite the volatile:

private volatile bool _disposed;

public void Dispose()
{
    if (!_disposed)
    {
        _disposed = true

        // disposal code here
    }
}

The problem here is that if two threads were close together, the first could check _disposed, read false, enter the code block and get switched out before setting _disposed to true. The second then checks _disposed, sees false and also enters the code block.

Using Interlocked ensures both the assignment and subsequent read are a single atomic operation.

Answer 3

Most BCL implementations of Dispose are not thread-safe. The idea is that it's up to the caller of Dispose to make sure nobody else is using the instance anymore before it is Disposed. In other words, it pushes the synchronization responsibility upwards. This makes sense, as otherwise now all your other consumers need to handle the boundary case where the object was Disposed while they were using it.

That said, if you want a thread-safe Disposable class, you can just create a lock around every public method (including Dispose) with a check for _disposed at the top. This may become more complicated if you have long-running methods where you don't want to hold the lock for the entire method.

Answer 4

I prefer to use integers and Interlocked.Exchange or Interlocked.CompareExchange on an integer-type object "disposed" or "state" variable; I'd use enum if Interlocked.Exchange or Interlocked.CompareExchange could handle such types, but alas they cannot.

One point which most discussions of IDisposable and finalizers fail to mention is that while an object's finalizer shouldn't run while IDisposable.Dispose() is in progress, there's no way for a class to prevent objects of its type from being declared dead and then resurrected. To be sure, if outside code allows that to happen there obviously can't be any requirement that the object "work normally", but the Dispose and finalize methods should be well-enough protected to ensure that they won't corrupt any other objects' state, which will in turn generally require using either locks or Interlocked operations on object state variables.

Answer 5

You have to lock every access to the ressource you are going to dispose. I also added the Dispose pattern I normally use.

public class MyThreadSafeClass : IDisposable
{
    private readonly object lockObj = new object();
    private MyRessource myRessource = new MyRessource();

    public void DoSomething()
    {
        Data data;
        lock (lockObj)
        {
            if (myResource == null) throw new ObjectDisposedException("");
            data = myResource.GetData();
        }
        // Do something with data
    }

    public void DoSomethingElse(Data data)
    {
        // Do something with data
        lock (lockObj)
        {
            if (myRessource == null) throw new ObjectDisposedException("");
            myRessource.SetData(data);
        }
    }

    ~MyThreadSafeClass()
    {
        Dispose(false);
    }
    public void Dispose() 
    { 
        Dispose(true); 
        GC.SuppressFinalize(this);
    }
    protected void Dispose(bool disposing) 
    {
        if (disposing)
        {
            lock (lockObj)
            {
                if (myRessource != null)
                {
                    myRessource.Dispose();
                    myRessource = null;
                }
            }
            //managed ressources
        }
        // unmanaged ressources
    }
}

Answer 6

FWIW, your sample code matches how my co-workers and I typically deal with this issue. We generally define a private CheckDisposed method on the class:

private volatile bool isDisposed = false; // Set to true by Dispose

private void CheckDisposed()
{
    if (this.isDisposed)
    {
        throw new ObjectDisposedException("This instance has already been disposed.");
    }
}

Then we call the CheckDisposed() method at the top of all public methods.

If thread contention over disposal is considered likely, rather than an error condition, I will also add a public IsDisposed() method (Similar to Control.IsDisposed).

---

Update: Based on the comments with respect to the value of making isDisposed volatile, note that the "fence" issue is rather trivial given how I use the CheckDisposed() method. It is essentially a troubleshooting tool for quickly catching the case where code calls a public method on the object after it has already been disposed. Calling CheckDisposed() at the start of a public method in no way guarantees that the object won't be disposed within that method. If I consider that to be a risk inherent in my class's design, as opposed to an error condition I failed to account for, then I use the aforementioned IsDisposed method along with appropriate locking.