DirectoryInfoWrapper throws DirectoryNotFoundException when directory is deleted during enumeration

[ericstj]

Description

Reported here #44626
Discovered here #41426 (comment)

I thought I had rooted out this issue and solved it by reducing the timer period(and added an extra test to try and capture the issue), but now I suspect there is a codepath here that is creating the timer on a background thread even when the PFW is disposed. So the polling timer is still going then we later delete the directory from under it.

The delete is happening between these two lines, and causing the exception:

runtime/src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Abstractions/DirectoryInfoWrapper.cs

Lines 36 to 38 in 5c65d89

	if (_directoryInfo.Exists)
	{
	foreach (FileSystemInfo fileSystemInfo in _directoryInfo.EnumerateFileSystemInfos("*", SearchOption.TopDirectoryOnly))

We could just catch the exception here, since it's already trying to avoid it by checking Exists. I was hoping to find a better fix to identify where to cancel the timer. Perhaps we are canceling the timer but it was already running?

This should fix PollingFileProviderShouldntConsumeINotifyInstances to be less flaky.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Description:	Description Reported here #44626 Discovered here #41426 (comment) I thought I had rooted out this issue and solved it by reducing the timer duration (and added an extra test to try and capture the issue), but now I suspect there is a codepath here that is creating the timer on a background thread even when the PFW is disposed. So the polling timer is still going then we later delete the directory from under it. The delete is happening between these two lines, and causing the exception: runtime/src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Abstractions/DirectoryInfoWrapper.cs Lines 36 to 38 in 5c65d89 if (_directoryInfo.Exists) { foreach (FileSystemInfo fileSystemInfo in _directoryInfo.EnumerateFileSystemInfos("*", SearchOption.TopDirectoryOnly)) We could just catch the exception here, since it's already trying to avoid it by checking Exists. I was hoping to find a better fix to identify where to cancel the timer. Perhaps we are canceling the timer but it was already running?
Author:	ericstj
Assignees:	-
Labels:	area-Extensions-FileSystem, untriaged
Milestone:	-

[ericstj]

I believe this is indeed a race condition. The Timer gets disposed by

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs

Line 261 in 5c65d89

_timer?.Dispose();

but nothing will stop a currently running timer callback when this happens.

The problem can occur if a directory is deleted after the exists check and before enumeration starts here:

runtime/src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Abstractions/DirectoryInfoWrapper.cs

Lines 36 to 38 in 5c65d89

	if (_directoryInfo.Exists)
	{
	foreach (FileSystemInfo fileSystemInfo in _directoryInfo.EnumerateFileSystemInfos("*", SearchOption.TopDirectoryOnly))

If the deletion happens during enumeration the enumeration will halt, but if it happens at the start an exception is thrown. We can address this by handling the exception, since the existence check makes it clear that this should ignore missing directories.

[ericstj]

Here's a customer hit on this same race condition: https://stackoverflow.com/questions/54830949/omnisharp-not-functioning-properly-in-one-project-but-is-in-another

[stephentoub]

but nothing will stop a currently running timer callback when this happens

I've not read the full description to know whether this is relevant, but there are Timer.Dispose{Async} overloads that let you wait until all work associated with the timer has quiesced.

[ericstj]

I can safely say that it's part of the DirectoryInfoWrapper's contract to not throw on a missing directory regardless so I feel this is the right fix.

[ericstj]

there are Timer.Dispose{Async} overloads that let you wait until all work associated with the timer has quiesced.

We'd need to add DisposeAsync to PhysicalFilesWatcher to support this, but that's not a bad idea. I'll open an issue on it.

[stephentoub]

And/or use Timer.Dispose(WaitHandle).

[directhex]

Closing as resolved by #44671