Fix NullReferenceException in FileSystemWatcher on Windows during dispose

[martincostello]

Fix NullReferenceException in FileSystemWatcher on Windows if two threads try and dispose the instance at the same time.

Found in a failing test while running the CI in a PR against the OpenTelemetry libraries for net8.0 on Windows: workflow logs

Failed OpenTelemetry.Instrumentation.AspNetCore.Tests.BasicTests.AddAspNetCoreInstrumentation_BadArgs [1 ms]
  Error Message:
   [Test Class Cleanup Failure (OpenTelemetry.Instrumentation.AspNetCore.Tests.BasicTests)]: System.NullReferenceException : Object reference not set to an instance of an object.
  Stack Trace:
     at System.IO.FileSystemWatcher.StopRaisingEvents()
   at System.IO.FileSystemWatcher.Dispose(Boolean disposing)
   at System.ComponentModel.Component.Dispose()
   at Microsoft.Extensions.FileProviders.Physical.PhysicalFilesWatcher.Dispose(Boolean disposing)
   at Microsoft.Extensions.FileProviders.PhysicalFileProvider.Dispose(Boolean disposing)
   at Microsoft.Extensions.FileProviders.PhysicalFileProvider.Dispose()
   at Microsoft.Extensions.Hosting.Internal.Host.<DisposeAsync>g__DisposeAsync|21_0(Object o)
   at Microsoft.Extensions.Hosting.Internal.Host.DisposeAsync()
   at Microsoft.Extensions.Hosting.Internal.Host.Dispose()
   at Microsoft.AspNetCore.Mvc.Testing.DeferredHostBuilder.DeferredHost.Dispose()
   at Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory`1.DisposeAsync()
   at Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory`1.DisposeAsync()
   at Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory`1.Dispose(Boolean disposing)
   at Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory`1.Dispose()

[dotnet-policy-service]

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

[stephentoub]

if two threads try and dispose the instance at the same time

It's not thread-safe. If two threads are calling Dispose concurrently, that's a bug in the consumer.

[martincostello]

Sure, but trying to find where the exception was coming from it looks like both the Linux and macOS implementations of StopRaisingEvents() guard against the state changing to a degree:

runtime/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.OSX.cs

Lines 69 to 76 in 70e3218

	CancellationTokenSource? token = _cancellation;
	if (token != null)
	{
	_cancellation = null;
	token.Cancel();
	token.Dispose();
	}
	}

runtime/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Linux.cs

Lines 98 to 103 in 70e3218

	var cts = _cancellation;
	if (cts != null)
	{
	_cancellation = null;
	cts.Cancel();
	}

Windows seems to be the odd-one-out here, so seemed reasonable to me to add the null guard.

I haven't looked into OTel's tests as to why it's happening - it could be an issue with their tests, it could be something in xunit as it appears to happen during test class cleanup.

[martincostello]

Looks like a standard xunit IClassFixture<T> usage, which itself is copied from some old samples in aspnetcore.

[stephentoub]

both the Linux and macOS implementations of StopRaisingEvents() guard against the state changing to a degree:

If the goal is consistency, then it'd be better to do:

SafeFileHandle? handle = _directoryHandle;
if (handle is not null)
{
    _directoryHandle = null;
    handle.Dispose();
}

That won't address other problems with this instance being erroneously used concurrently, though.

[martincostello]

Sure, I'll refactor that then. I went with ? as it was the smallest change.