How to properly handle a list of IDisposable items in my Dispose()?

Question

I have a class like this:

public class MyFolderRefresher {
    public async Task OnRename(RenamedEventArgs e) {
        // do something
    }
}

public class MyDisposableListOfFileSystemWatchers : IDisposable {
    private List<FileSystemWatcher> _mWatchers;
    private MyFolderRefresher _mFolderRefresher;

    public MyDisposableListOfFileSystemWatchers(List<string> pathsToWatch) {
        _mFolderRefresher = new MyFolderRefresher();
        _mWatchers = new List<FileSystemWatcher>();

        foreach (var path in pathsToWatch) {
            var fileSystemWatcher = new FileSystemWatcher(path)
            {
                NotifyFilter = NotifyFilters.DirectoryName,
                IncludeSubdirectories = true,
                InternalBufferSize = 64 * 1024
            };

            // Listen to events in some different class
            fileSystemWatcher.Renamed += async (sender, e) => await _mFolderRefresher.OnRename(e);
            
            _mWatchers.Add(fileSystemWatcher);
        }
    }

    public void Dispose() {
        Dispose(true);
    }

    private bool _mDisposed = false;
    protected virtual void Dispose(bool disposing) {
        if (_mDisposed) {
            return;
        }

        if (disposing) {
            foreach(var watcher in _mWatchers) {
                watcher.Dispose();
            }

            _mWatchers.Clear();
        }

        _mDisposed = true;
    }
}

The problem is that when calling Dispose() of my MyDisposableListOfFileSystemWatchers class, I sometimes get System.InvalidOperationException. Most of the time it's just a stack trace in the event viewer, but once I caught it in a debugger and was able to see that it was System.InvalidOperationException: Collection was modified; enumeration operation may not execute (not sure if both are the same issue or not).

I'm wondering if I'm doing something wrong in my Dispose(). Specifically:

• Should I be calling GC.SuppressFinalize(this) in my public void Dispose() ?
• Is it ok that I Dispose objects that I'm iterating over?
• Is it ok that I clear the collection, or should I leave that to the Garbage Collector?
• Since I'm Disposing the Publisher (FileSystemWatcher), am I correct in assuming I don't need to manually unsubscribe from the events?

I've read the documentation but wasn't able to find a definitive answer to those questions and I'm at a bit of a loss when it comes to the System.InvalidOperationException as I don't see how it would be possible that both the constructor and Dispose could be running at the same time (those are the only places where _mWatchers` is modified).

Answer 1

I've found the answer in my specific scenario.

In my case, it was true that I was overthinking it too much, a bit lost in my lack of knowledge of C# and managed languages in general. It turned out that my Dispose() was called multiple times and was not guarded agains that.

I've been creating objects that can call Dispose() on my collection when they need to be reloaded. However this reloading could be cancelled in case another reload was requested, and my Dispose() was not guarded in any way against that in case the "cancel reload" came when the Dispose was in progress.

Answer 2

The System.InvalidOperationException: Collection was modified; enumeration operation may not execute exception indicates that your _mWatchers list was modified during iteration. Since the only place you iterate through it and/or modify it is in your Dispose() method, I can see two possible reasons for this:

1. You are disposing of your MyDisposableListOfFileSystemWatchers in multiple threads simultaneously causing reentrancy, or

2. MyDisposableListOfFileSystemWatchers.Dispose() is somehow calling itself recursively (e.g. because it is somehow getting called by FileSystemWatcher.Dispose()), causing reentrancy.

You can protect yourself against both possibilities by adopting an immutable programming style that leaves your class members unchanged after construction -- other than a member to track disposal -- like so:

public class MyDisposableListOfFileSystemWatchers : IDisposable 
{
    private readonly IReadOnlyList<FileSystemWatcher> _mWatchers;
    private readonly MyFolderRefresher _mFolderRefresher;

    public MyDisposableListOfFileSystemWatchers(List<string> pathsToWatch) 
    {
        _mFolderRefresher = new MyFolderRefresher();
        _mWatchers = pathsToWatch
            .Select(path =>
                    {
                        var fileSystemWatcher = new FileSystemWatcher(path)
                        {
                            NotifyFilter = NotifyFilters.DirectoryName,
                            IncludeSubdirectories = true,
                            InternalBufferSize = 64 * 1024
                        };
                        // Listen to events in some different class
                        fileSystemWatcher.Renamed += async (sender, e) => await _mFolderRefresher.OnRename(e);
                        return fileSystemWatcher;
                    })
            .ToList()
            .AsReadOnly();
    }

    public void Dispose() 
    {
        //https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1063#pseudo-code-example
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    private int _mDisposeCount = 0;

    protected virtual void Dispose(bool disposing) 
    {
        if (Interlocked.Exchange(ref _mDisposeCount, 1) == 0)
        {
            if (disposing) 
            {
                // Dispose managed resources
                foreach (var watcher in _mWatchers)
                    watcher?.Dispose();
            }
            // Dispose managed resources
        }
    }
}

As you can see, the only mutable member of the class is _mDisposeCount which we mutate using the thread-safe Interlocked class. By adding the _mDisposeCount check at the beginning of Dispose(bool) we protect against reentrancy of both types above.

Now, with regards to your specific questions:

• Should I be calling GC.SuppressFinalize(this) in my public void Dispose() ?

  This is recommended by the Microsoft documentation which states:

  NOTE: Leave out the finalizer altogether if this class doesn't own unmanaged resources, but leave the other methods exactly as they are.

  I would recommend following the documentation as written without thinking too hard about it.

• Is it ok that I Dispose objects that I'm iterating over?

  Yes as long as you are disposing of managed + unmanaged resources (i.e. disposing is true). When disposing of purely unmanaged resources in a finalizer the question becomes more complex -- but this does not apply to this question.

• Is it ok that I clear the collection, or should I leave that to the Garbage Collector?

  There is no need to do this. Furthermore, if your object may be constructed in one thread and disposed in another you will need to make your Dispose() method thread-safe, so mutating the collection may be actively unhelpful.

  However, if for some reason you do need to do this (e.g. because the class is more complex that is shown in your question and you want to guarantee the list is not used after disposal), I would recommend swapping the _mWatchers list with an empty or null collection rather than mutating the collection itself, like so:

  private IReadOnlyList<FileSystemWatcher> _mWatchers;
  private int _mDisposeCount = 0;
  
  protected virtual void Dispose(bool disposing) 
  {
      if (Interlocked.Exchange(ref _mDisposeCount, 1) == 0)
      {
          if (disposing) 
          {
              // Dispose managed resources
              foreach (var watcher in Interlocked.Exchange(ref _mWatchers, Array.Empty<FileSystemWatcher>()))
                  watcher?.Dispose();
          }
          // Dispose managed resources
      }
  }
  

• Since I'm Disposing the Publisher (FileSystemWatcher), am I correct in assuming I don't need to manually unsubscribe from the events?

  This doesn't seem to be explicitly documented but it is safe to assume.

  For confirmation we can check the source code for FileSystemWatcher.Dispose(bool disposing) which shows that the internal event handlers including _onRenamedHandler are indeed nulled out when the watcher is disposed.

One final note: if your actual OnRename(RenamedEventArgs e) event updates your user interface, be sure to invoke it on the correct thread. See How to make thread safe calls from FileSystemWatcher for details.

Answer 3

Should I be calling GC.SuppressFinalize(this) in my public void Dispose() ?

No, you do not have a finalizer, and without one there is no point.

The question then becomes, should you have a finalizer? This would be needed if someone derives from your class, and also owns some native resource that needs to be cleaned up. If that is not something you plan to do I would recommend just marking your class as sealed. That allow you to simplify your dispose method, and move the responsibility of implementing the full dispose-with-finalizer-pattern (see last example) to whoever unseals your class.

Is it ok that I Dispose objects that I'm iterating over?

Yes, that is totally fine.

Is it ok that I clear the collection, or should I leave that to the Garbage Collector?

It should not really matter much. Clearing it should not hurt, and may help in some cases.

Since I'm Disposing the Publisher (FileSystemWatcher), am I correct in assuming I don't need to manually unsubscribe from the events?

Probably. I would probably do so anyway, but I'm somewhat paranoid when it comes to disposal.

System.InvalidOperationException: Collection was modified; enumeration operation may not execute

I don't see any way this could be caused by the posted code. The documentation for FileSystemWatcher.Dispose also do not mention any possibility of exceptions.

You do not set any SynchronizingObject on your fileSystem watcher. That means the events will be raised on a background thread, making your program multi-threaded. This means you need to be really careful to ensure thread safety. For example, if the rename event handler somehow ends up disposing your object the object might be disposed before its constructor has completed. And this could cause the list to be modified while it is iterated.

If this is the case you may need to remove this possibility somehow. You could also set a bool in the end of the constructor and add a Debug.Assert(IsFullyConstructed) to your dispose method to help confirm or rule out this theory.