Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

{
Debug.Assert(parent != null || _wdToPathMap.Count == 0);
AddDirectoryWatchUnlocked(parent, directoryName);
}
}
Expand Down Expand Up @@ -361,6 +360,15 @@ private void AddDirectoryWatchUnlocked(WatchedDirectory? parent, string director
// raise the Error event with the exception and let the user decide how to handle it.

Interop.ErrorInfo error = Interop.Sys.GetLastErrorInfo();

// Don't report an error when we can't add a watch because the child directory
// was removed or replaced by a file.
if (hasParent && (error.Error == Interop.Error.ENOENT ||
error.Error == Interop.Error.ENOTDIR))
{
return;
}

Exception exc;
if (error.Error == Interop.Error.ENOSPC)
{
Expand Down Expand Up @@ -432,16 +440,30 @@ private void AddDirectoryWatchUnlocked(WatchedDirectory? parent, string director
// asked for subdirectories to be included.
if (isNewDirectory && _includeSubdirectories)
{
// This method is recursive. If we expect to see hierarchies
// so deep that it would cause us to overflow the stack, we could
// consider using an explicit stack object rather than recursion.
// This is unlikely, however, given typical directory names
// and max path limits.
foreach (string subDir in Directory.EnumerateDirectories(fullPath))
try
{
AddDirectoryWatchUnlocked(directoryEntry, System.IO.Path.GetFileName(subDir));
// AddDirectoryWatchUnlocked will add the new directory to
// this.Children, so we don't have to / shouldn't also do it here.
// This method is recursive. If we expect to see hierarchies
// so deep that it would cause us to overflow the stack, we could
// consider using an explicit stack object rather than recursion.
// This is unlikely, however, given typical directory names
// and max path limits.
foreach (string subDir in Directory.EnumerateDirectories(fullPath))
{
AddDirectoryWatchUnlocked(directoryEntry, System.IO.Path.GetFileName(subDir));
// AddDirectoryWatchUnlocked will add the new directory to
// this.Children, so we don't have to / shouldn't also do it here.
}
}
catch (DirectoryNotFoundException)
{ } // The child directory was removed.
catch (IOException ex) when (ex.HResult == Interop.Error.ENOTDIR.Info().RawErrno)
{ } // The child directory was replaced by a file.
catch (Exception ex)
{
if (_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher))
Comment on lines +462 to +463
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Suggested change
{
if (_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher))
{
Debug.Fail(ex.ToString());
if (_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
watcher.OnError(new ErrorEventArgs(ex));
}
}
}
}
Expand Down