Skip to content
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.

Sign up for GitHub

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

Cleanup lifetime management in OSX FileSystemWatcher #52019

Merged
merged 1 commit into from
Apr 29, 2021
Merged

Cleanup lifetime management in OSX FileSystemWatcher #52019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 3 additions & 26 deletions src/libraries/Common/src/Interop/OSX/Interop.EventStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ internal unsafe delegate void FSEventStreamCallback(
/// Internal wrapper to create a new EventStream to listen to events from the core OS (such as File System events).
/// </summary>
/// <param name="allocator">Should be IntPtr.Zero</param>
/// <param name="cb">A callback instance that will be called for every event batch.</param>
/// <param name="callback">A callback instance that will be called for every event batch.</param>
/// <param name="context">Should be IntPtr.Zero</param>
/// <param name="pathsToWatch">A CFArray of the path(s) to watch for events.</param>
/// <param name="sinceWhen">
Expand All @@ -107,38 +107,15 @@ internal unsafe delegate void FSEventStreamCallback(
/// <returns>On success, returns a pointer to an FSEventStream object; otherwise, returns IntPtr.Zero</returns>
/// <remarks>For *nix systems, the CLR maps ANSI to UTF-8, so be explicit about that</remarks>
[DllImport(Interop.Libraries.CoreServicesLibrary, CharSet = CharSet.Ansi)]
private static extern SafeEventStreamHandle FSEventStreamCreate(
internal static extern SafeEventStreamHandle FSEventStreamCreate(
IntPtr allocator,
FSEventStreamCallback cb,
FSEventStreamCallback callback,
IntPtr context,
SafeCreateHandle pathsToWatch,
FSEventStreamEventId sinceWhen,
CFTimeInterval latency,
FSEventStreamCreateFlags flags);

/// <summary>
/// Creates a new EventStream to listen to events from the core OS (such as File System events).
/// </summary>
/// <param name="cb">A callback instance that will be called for every event batch.</param>
/// <param name="pathsToWatch">A CFArray of the path(s) to watch for events.</param>
/// <param name="sinceWhen">
/// The start point to receive events from. This can be to retrieve historical events or only new events.
/// To get historical events, pass in the corresponding ID of the event you want to start from.
/// To get only new events, pass in kFSEventStreamEventIdSinceNow.
/// </param>
/// <param name="latency">Coalescing period to wait before sending events.</param>
/// <param name="flags">Flags to say what kind of events should be sent through this stream.</param>
/// <returns>On success, returns a valid SafeCreateHandle to an FSEventStream object; otherwise, returns an invalid SafeCreateHandle</returns>
internal static SafeEventStreamHandle FSEventStreamCreate(
FSEventStreamCallback cb,
SafeCreateHandle pathsToWatch,
FSEventStreamEventId sinceWhen,
CFTimeInterval latency,
FSEventStreamCreateFlags flags)
{
return FSEventStreamCreate(IntPtr.Zero, cb, IntPtr.Zero, pathsToWatch, sinceWhen, latency, flags);
Copy link
Member Author

Choose a reason for hiding this comment

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

Low value wrapper.

}

/// <summary>
/// Attaches an EventStream to a RunLoop so events can be received. This should usually be the current thread's RunLoop.
/// </summary>
Expand Down
131 changes: 83 additions & 48 deletions src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.OSX.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ private sealed class RunningInstance
// Callback delegate for the EventStream events
private readonly Interop.EventStream.FSEventStreamCallback _callback;
Copy link
Member Author

@jkotas jkotas Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The new GCHandle is a counterpart for this delegate. The RunningInstance has to be kept alive for as long as the marshalled delegate is in use. There was nothing to guarantee that before this change.

I have also looked into replacing the delegate by a function pointer, but that was more involved change so I have abandoned it.


// GC handle to keep this running instance rooted
private GCHandle _gcHandle;

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

Expand Down Expand Up @@ -270,71 +273,103 @@ private void CleanupEventStream()
if (eventStream != null)
{
_cancellationRegistration.Unregister();
try
{
// When we get here, we've requested to stop so cleanup the EventStream and unschedule from the RunLoop
Interop.EventStream.FSEventStreamStop(eventStream);
Copy link
Member Author

Choose a reason for hiding this comment

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

FSEventStreamStop does not throw any exceptions. This try/finally does not look necessary. It may be a left over from some thread abort hardening.

}
finally
{
StaticWatcherRunLoopManager.UnscheduleFromRunLoop(eventStream);
eventStream.Close();
}

// When we get here, we've requested to stop so cleanup the EventStream and unschedule from the RunLoop
Interop.EventStream.FSEventStreamStop(eventStream);

StaticWatcherRunLoopManager.UnscheduleFromRunLoop(eventStream);
eventStream.Dispose();

Debug.Assert(_gcHandle.IsAllocated);
_gcHandle.Free();
}
}

internal void Start(CancellationToken cancellationToken)
{
// Get the path to watch and verify we created the CFStringRef
SafeCreateHandle path = Interop.CoreFoundation.CFStringCreateWithCString(_fullDirectory);
if (path.IsInvalid)
SafeCreateHandle? path = null;
SafeCreateHandle? arrPaths = null;
bool cleanupGCHandle = false;
try
Copy link
Member Author

Choose a reason for hiding this comment

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

This refactoring is to ensure that everything gets disposed when anything fails.

{
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, true);
}
// Get the path to watch and verify we created the CFStringRef
path = Interop.CoreFoundation.CFStringCreateWithCString(_fullDirectory);
if (path.IsInvalid)
{
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, true);
}

// Take the CFStringRef and put it into an array to pass to the EventStream
SafeCreateHandle arrPaths = Interop.CoreFoundation.CFArrayCreate(new CFStringRef[1] { path.DangerousGetHandle() }, (UIntPtr)1);
if (arrPaths.IsInvalid)
{
path.Dispose();
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, true);
}
// Take the CFStringRef and put it into an array to pass to the EventStream
arrPaths = Interop.CoreFoundation.CFArrayCreate(new CFStringRef[1] { path.DangerousGetHandle() }, (UIntPtr)1);
if (arrPaths.IsInvalid)
{
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, true);
}

_context = ExecutionContext.Capture();
_context = ExecutionContext.Capture();

// Make sure the OS file buffer(s) are fully flushed so we don't get events from cached I/O
Interop.Sys.Sync();
// Make sure the OS file buffer(s) are fully flushed so we don't get events from cached I/O
Interop.Sys.Sync();

// Create the event stream for the path and tell the stream to watch for file system events.
_eventStream = Interop.EventStream.FSEventStreamCreate(
_callback,
arrPaths,
Interop.EventStream.kFSEventStreamEventIdSinceNow,
0.0f,
EventStreamFlags);
if (_eventStream.IsInvalid)
Debug.Assert(!_gcHandle.IsAllocated);
_gcHandle = GCHandle.Alloc(this);

cleanupGCHandle = true;

// Create the event stream for the path and tell the stream to watch for file system events.
SafeEventStreamHandle eventStream = Interop.EventStream.FSEventStreamCreate(
IntPtr.Zero,
_callback,
IntPtr.Zero,
arrPaths,
Interop.EventStream.kFSEventStreamEventIdSinceNow,
0.0f,
EventStreamFlags);
if (eventStream.IsInvalid)
{
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, true);
}

cleanupGCHandle = false;

_eventStream = eventStream;
}
finally
{
arrPaths.Dispose();
path.Dispose();
throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo(), _fullDirectory, true);
if (cleanupGCHandle)
_gcHandle.Free();
arrPaths?.Dispose();
path?.Dispose();
}

StaticWatcherRunLoopManager.ScheduleEventStream(_eventStream);

bool started = Interop.EventStream.FSEventStreamStart(_eventStream);
if (started)
bool success = false;
try
{
// Once we've started, register to stop the watcher on cancellation being requested.
_cancellationRegistration = cancellationToken.UnsafeRegister(obj => ((RunningInstance)obj!).CleanupEventStream(), this);
StaticWatcherRunLoopManager.ScheduleEventStream(_eventStream);

if (!Interop.EventStream.FSEventStreamStart(_eventStream))
{
// 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, error)));
}
}
else
{
// Once we've started, register to stop the watcher on cancellation being requested.
_cancellationRegistration = cancellationToken.UnsafeRegister(obj => ((RunningInstance)obj!).CleanupEventStream(), this);

success = true;
}
}
else
finally
{
// 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))
if (!success)
{
// An error occurred while trying to start the run loop so fail out
watcher.OnError(new ErrorEventArgs(new IOException(SR.EventStream_FailedToStart, error)));
CleanupEventStream();
}
}
}
Expand Down