Runtime: FileSystemWatcher: Cannot change Filter safely

[clairernovotny]

In the FileSystemWatcher, there doesn't seem to be a reliable way to change the Filter property once events have started. The issue is that a callback may be in flight at any time and you can get an enumeration exception like this.

Could there be a way to synchronize access to that property to avoid this? The current workaround requires disposing and recreating the FileSystemWatcher instance each time the filter needs to change.

Thoughts?

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumFailedVersion()
   at System.Collections.Generic.List<T>+Enumerator.MoveNextRare()
   at System.Collections.Generic.List<T>+Enumerator.MoveNext()
   at System.IO.FileSystemWatcher.MatchPattern(ReadOnlySpan<T> relativePath)
   at System.IO.FileSystemWatcher.NotifyFileSystemEventArgs(WatcherChangeTypes changeType, ReadOnlySpan<T> name)
   at System.IO.FileSystemWatcher.ParseEventBufferAndNotifyForEach(byte[] buffer)
   at System.IO.FileSystemWatcher.ReadDirectoryChangesCallback(uint errorCode, uint numBytes, NativeOverlapped* overlappedPointer)
   at System.Threading.ThreadPoolBoundHandleOverlapped.CompletionCallback(uint errorCode, uint numBytes, NativeOverlapped* nativeOverlapped)
   at System.Threading._IOCompletionCallback.IOCompletionCallback_Context(object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, object state)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, object state)
   at System.Threading._IOCompletionCallback.PerformIOCompletionCallback(uint errorCode, uint numBytes, NativeOverlapped* pNativeOverlapped)

[stephentoub]

The current workaround requires disposing and recreating the FileSystemWatcher instance each time the filter needs to change.

This would be required in the implementation as well, at least on Linux, where the filters are used in the creation of the native inotify watchers.

[stephentoub]

The issue is that a callback may be in flight at any time and you can get an enumeration exception like this.

That's just a bug arguably, and should be easily fixable. I expect that's only on Windows.

[stephentoub]

This would be required in the implementation as well

Sorry, I misread, I thought you were talking about stopping and starting. The filters should not be changed while running, but when stopped, that should be fine.

[stephentoub]

Hmm, according to the docs, I'm wrong about that:
"The Filter property can be changed after the FileSystemWatcher object has started receiving events"
We may have different bugs in Unix.

[clairernovotny]

One thing to consider as well, is that start/stop via EnableRaisingEvents could still lead to the same race condition as it looks like a callback could still be in-flight?

I'm looking at https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs#L132 but am not seeing where it block until any callbacks were finished? Like what happens if the callback is in the callstack after _currentSession is updated in the Stop, basically after ReadDirectoryChangesCallback, then someone sets Filter, it could still break.

[stephentoub]

but am not seeing where it block until any callbacks were finished?

It doesn't, and adding such blocking could lead to deadlocks, e.g. if the callback itself called Stop.

[clairernovotny]

Agreed, not asking for it to block. Just thinking of any ways it can be made safe without creating a new instance today.

[stephentoub]

So I was reading this issue on my phone and hadn't fully comprehended what property was being used here. At first I thought you were referring to NotifyFilter, and my comments were mainly focused on that. Then I thought you were referring to Filter. Now that I'm at my computer and actually thinking, I see we actually added a .NET Core-only Filters property, and that's actually what you're referring to, and it's that collection that's being mutated and enumerated concurrently.

The design here is unfortunate, exposing Collection<string> but also supporting mutation and being documented to support live updates. I'm not sure why we did that.
https://github.com/dotnet/corefx/pull/29315
cc: @JeremyKuhne, @Anipik

At this point, I see two solutions:

• Make the Collection<string> wrap an immutable-ish IList<string> that creates a new underlying array every time the collection is modified, such that the underlying array instance is never modified, just replaced with another immutable one. That way, we don't have to worry about concurrent updates.
• Lock whenever accessing the collection.

I expect it'll be best to do the former. Changes to Filters should be relatively rare compared to notifications of changes.