Skip to content

Commit

Permalink
Fix ObjectDisposedException in FileSystemWatcher.OSX.cs (dotnet#34589)
Browse files Browse the repository at this point in the history 
Synchronize access to disposing the SafeHandle, and clean up some code distributed oddly between the ctor and Start, so as to make more fields readonly and only register for cancellation once everything has started, to avoid race conditions that could otherwise result between cancellation and starting happening concurrently.
  • Loading branch information
@stephentoub
stephentoub authored Apr 7, 2020
1 parent 29ed7be commit 8aa97da
Showing 1 changed file with 46 additions and 58 deletions.
104 changes: 46 additions & 58 deletions src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.OSX.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,12 @@ private void StartRaisingEvents()

try
{
CancellationTokenSource cancellation = new CancellationTokenSource();
RunningInstance instance = new RunningInstance(this, _directory, _includeSubdirectories, TranslateFlags(_notifyFilters), cancellation.Token);
_enabled = true;
var cancellation = new CancellationTokenSource();
_cancellation = cancellation;
instance.Start();
_enabled = true;

var instance = new RunningInstance(this, _directory, _includeSubdirectories, TranslateFlags(_notifyFilters));
instance.Start(cancellation.Token);
}
catch
{
Expand Down Expand Up @@ -131,47 +132,52 @@ private sealed class RunningInstance

// The user can input relative paths, which can muck with our path comparisons. Save off the
// actual full path so we can use it for comparing
private string _fullDirectory;
private readonly string _fullDirectory;

// Boolean if we allow events from nested folders
private bool _includeChildren;
private readonly bool _includeChildren;

// The bitmask of events that we want to send to the user
private FSEventStreamEventFlags _filterFlags;

// The EventStream to listen for events on
private SafeEventStreamHandle? _eventStream;

private readonly FSEventStreamEventFlags _filterFlags;

// Callback delegate for the EventStream events
private Interop.EventStream.FSEventStreamCallback? _callback;
private readonly Interop.EventStream.FSEventStreamCallback _callback;

// Token to monitor for cancellation requests, upon which processing is stopped and all
// state is cleaned up.
private readonly CancellationToken _cancellationToken;
// The EventStream to listen for events on
private SafeEventStreamHandle? _eventStream;

// Calling RunLoopStop multiple times SegFaults so protect the call to it
private bool _stopping;
// Registration with the cancellation token.
private CancellationTokenRegistration _cancellationRegistration;

private ExecutionContext? _context;

internal RunningInstance(
internal unsafe RunningInstance(
FileSystemWatcher watcher,
string directory,
bool includeChildren,
FSEventStreamEventFlags filter,
CancellationToken cancelToken)
FSEventStreamEventFlags filter)
{
Debug.Assert(string.IsNullOrEmpty(directory) == false);
Debug.Assert(!cancelToken.IsCancellationRequested);
Debug.Assert(!string.IsNullOrEmpty(directory));

_weakWatcher = new WeakReference<FileSystemWatcher>(watcher);
// Make sure _fullPath doesn't contain a link or alias since the OS will give back the actual,
// non link'd or alias'd paths.
_fullDirectory = System.IO.Path.GetFullPath(directory);
_fullDirectory = Interop.Sys.RealPath(_fullDirectory);
if (_fullDirectory is null)
{
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, isDirectory: true);
}

// Also ensure it has a trailing slash.
if (!_fullDirectory.EndsWith('/'))
{
_fullDirectory += "/";
}

_callback = new Interop.EventStream.FSEventStreamCallback(FileSystemEventCallback);
_weakWatcher = new WeakReference<FileSystemWatcher>(watcher);
_includeChildren = includeChildren;
_filterFlags = filter;
_cancellationToken = cancelToken;
_cancellationToken.UnsafeRegister(obj => ((RunningInstance)obj!).CancellationCallback(), this);
_stopping = false;
}

private static class StaticWatcherRunLoopManager
Expand Down Expand Up @@ -260,14 +266,12 @@ private static void WatchForFileSystemEventsThreadStart(ManualResetEventSlim run
}
}

private void CancellationCallback()
private void CleanupEventStream()
{
SafeEventStreamHandle? eventStream = _eventStream;
if (!_stopping && eventStream != null)
SafeEventStreamHandle? eventStream = Interlocked.Exchange(ref _eventStream, null);
if (eventStream != null)
{
_stopping = true;
_eventStream = null;

_cancellationRegistration.Unregister();
try
{
// When we get here, we've requested to stop so cleanup the EventStream and unschedule from the RunLoop
Expand All @@ -281,24 +285,8 @@ private void CancellationCallback()
}
}

internal unsafe void Start()
internal void Start(CancellationToken cancellationToken)
{
// Make sure _fullPath doesn't contain a link or alias
// since the OS will give back the actual, non link'd or alias'd paths
_fullDirectory = Interop.Sys.RealPath(_fullDirectory);
if (_fullDirectory == null)
{
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, true);
}

Debug.Assert(string.IsNullOrEmpty(_fullDirectory) == false, "Watch directory is null or empty");

// Normalize the _fullDirectory path to have a trailing slash
if (_fullDirectory[_fullDirectory.Length - 1] != '/')
{
_fullDirectory += "/";
}

// Get the path to watch and verify we created the CFStringRef
SafeCreateHandle path = Interop.CoreFoundation.CFStringCreateWithCString(_fullDirectory);
if (path.IsInvalid)
Expand All @@ -314,12 +302,6 @@ internal unsafe void Start()
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, true);
}

// Create the callback for the EventStream if it wasn't previously created for this instance.
if (_callback == null)
{
_callback = new Interop.EventStream.FSEventStreamCallback(FileSystemEventCallback);
}

_context = ExecutionContext.Capture();

// Make sure the OS file buffer(s) are fully flushed so we don't get events from cached I/O
Expand All @@ -342,13 +324,19 @@ internal unsafe void Start()
StaticWatcherRunLoopManager.ScheduleEventStream(_eventStream);

bool started = Interop.EventStream.FSEventStreamStart(_eventStream);
if (!started)
if (started)
{
// Once we've started, register to stop the watcher on cancellation being requested.
_cancellationRegistration = cancellationToken.UnsafeRegister(obj => ((RunningInstance)obj!).CleanupEventStream(), this);
}
else
{
// Try to get the Watcher to raise the error event; if we can't do that, just silently exit since the watcher is gone anyway
int error = Marshal.GetLastWin32Error();
if (_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher))
{
// An error occurred while trying to start the run loop so fail out
watcher.OnError(new ErrorEventArgs(new IOException(SR.EventStream_FailedToStart, Marshal.GetLastWin32Error())));
watcher.OnError(new ErrorEventArgs(new IOException(SR.EventStream_FailedToStart, error)));
}
}
}
Expand All @@ -367,7 +355,7 @@ private unsafe void FileSystemEventCallback(
// there's nothing more to do (we can't raise events), so bail.
if (!_weakWatcher.TryGetTarget(out FileSystemWatcher? watcher))
{
CancellationCallback();
CleanupEventStream();
return;
}

Expand Down

0 comments on commit 8aa97da

Please sign in to comment.