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:
GC.SuppressFinalize(this) in my public void Dispose() ?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).
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:
You are disposing of your MyDisposableListOfFileSystemWatchers in multiple threads simultaneously causing reentrancy, or
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.