-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make PhysicalFilesWatcher exclusively use polling when pollForChanges=true #41426
Conversation
Tagging subscribers to this area: @maryamariyan |
The PR is giving a repro of the error.
Stack trace
cc: @pranavkm |
src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.cs
Outdated
Show resolved
Hide resolved
@pranavkm how likely do you imagine folks are to be regressed by this change? Since we aren't enabling the watcher it means that the only events will be raised by polling. This could result in latency or different events entirely if polling doesn't capture all types of changes that a FileSystemWatcher would. What do you think about the likelihood of this and overall general risk of making this change (at this point in the release)? |
src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs
Outdated
Show resolved
Hide resolved
@ericstj we've advertised the flag that enables polling as a resort for apps that are deployed to environments that do not trigger the .NET file watcher (e.g. mounted drives, network shares etc). For such users, this change would make no visible difference. |
This is less breaking that ignoring the FileSystemWatcher when polling is set as it still lets the PhysicalFileWatcher work in FSW + Polling mode. Now when a FileSystemWatcher is not passed to the PhysicalFileWatcher we'll permit construction (if polling is enabled) and have it behave as exclusively polling. The PhysicalFileProvider will use this mode when both UsePollingFileWatcher and UseActivePolling are set which is what happens when the DOTNET_USE_POLLING_FILE_WATCHER environment variable is set.
src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the existing comment on guard check placement, LGTM.
# Conflicts: # src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs
72658cd
to
6b177fd
Compare
Test failures as follows:
This indicates a problem with the polling file watcher if the directory it is watching is deleted underneath it. This is a reliability issue and one we should probably address. Without it the tests I added will be flaky. |
We have to avoid exclusively using FileSystemWatcher since it looks like FSW misses file deletes that happen as a result of directory delete on OSX.
Ok, the flakiness in this test should be fixed now. Can I get another review? |
src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had one tiny nit. LGTM.
Fixes #37664