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.

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

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Apr 28, 2021

May fix #30056

@ghost
Copy link

ghost commented Apr 28, 2021

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

Issue Details

May fix #51538

Author: jkotas
Assignees: -
Labels:

area-System.IO

Milestone: -

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.

@@ -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

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.

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.

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.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, big thanks for fixing this @jkotas !

@adamsitnik adamsitnik added this to the 6.0.0 milestone Apr 29, 2021
@jkotas jkotas merged commit 964b500 into dotnet:main Apr 29, 2021
@jkotas jkotas deleted the watcher branch April 29, 2021 19:21
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SafeHandle use-after-dispose in FileSystemWatcher on OSX
2 participants