Skip to content

Conversation

@tmds
Copy link
Member

@tmds tmds commented Feb 7, 2022

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

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 ghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Feb 7, 2022
@ghost
Copy link

ghost commented Feb 7, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

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

Author: tmds
Assignees: -
Labels:

area-System.IO

Milestone: -

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

Comment on lines +462 to +463
{
if (_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher))
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.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you Tom!

@adamsitnik adamsitnik merged commit 89e5469 into dotnet:main Feb 18, 2022
@adamsitnik adamsitnik added this to the 7.0.0 milestone Feb 18, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure in CreateDefaultBuilder_ConfigJsonDoesReload on Fedora 34 arm64

3 participants