FileSystemWatcher.Linux: handle races while adding child directories. #64906
Conversation
This handles the child directory getting removed or replaced by a file while we are adding a watch for it or enumerating its subdirectories.
ghost
added
area-System.IO
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsThis handles the child directory getting removed or replaced by a file Fixes #57139. @adamsitnik ptal. cc @omajid
|
| @@ -324,9 +324,8 @@ private void AddDirectoryWatch(WatchedDirectory parent, string directoryName) | |||
| // against the handle, so we'd deadlock if we relied on that approach. Instead, we want to follow | |||
| // the approach of removing all watches when we're done, which means we also don't want to | |||
| // add any new watches once the count hits zero. | |||
| if (parent == null || _wdToPathMap.Count > 0) | |||
| if (_wdToPathMap.Count > 0) | |||
There was a problem hiding this comment.
I've removed the null check because the method doesn't get called with parent==null, and the argument is also not nullable (WatchedDirectory parent).
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs
Show resolved
Hide resolved
| { | ||
| if (_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher)) |
There was a problem hiding this comment.
In the reported issue Directory.EnumerateDirectories failed with DirectoryNotFoundException so this particular catch is for this method.
I assume that you added ENOTDIR because you know that it's another error that could potentially occur?
But how about the catch (Exception ex)? You have done that just to be extra safe? I can see that we are doing that in other places here. I am not 100% sure if all our unit tests verify that OnError was not invoked. Can we add Debug.Fail(ex.ToString()); so when we introduce a bug here the CI is going to get red?
| { | |
| if (_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher)) | |
| { | |
| Debug.Fail(ex.ToString()); | |
| if (_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher)) |
There was a problem hiding this comment.
I assume that you added ENOTDIR because you know that it's another error that could potentially occur?
inotify is not recursive. When we get a notification that a new directory is added, we need to recurse it ourselves. By the time we start recursing it, the directory may be gone (DirectoryNotFoundException) or replaced by a file (ENOTDIR). In those two cases, we pretend the directory was never there.
If there are other errors, we report them to the user, similar to what already happens for inotify_add_watch errors.
Can we add Debug.Fail(ex.ToString()); so when we introduce a bug here the CI is going to get red?
It isn't there for inotify_add_watch errors, so I did the same thing here.
This handles the child directory getting removed or replaced by a file
while we are adding a watch for it or enumerating its subdirectories.
Fixes #57139.
@adamsitnik ptal.
cc @omajid