OSX FileSystemWatcher needs to root RunningInstance

[carlossanlop]

Fixes #30056

This is an implementation of the hypothesis shared here: #30056 (comment)

FileSystemWatcher is currently causing CI failures, and one potential root cause may be that we are using an event that was generated by an object that is not rooted. The issue might go away if we root the RunningInstance instance and get it freed when the FileSystemWatcher is disposed.

[ghost]

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

Issue Details

---

Proof of concept. Please do not review yet.

I'm quickly testing the hypothesis shared here: #30056 (comment)

I'm currently having trouble building on my Macbook so I need to use the CI.

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	area-System.IO
Milestone:	-

[carlossanlop]

Obviously need a resource string here. But before I add one, do we want to throw here? Should we simply reuse the existing running instance if the user accidentally calls StartRaisingEvents more than once in a row?

The previous behavior was to create a brand new instance every time, which is why I thought it would make sense to continue trying to do the same.

Another option is to free any allocated instances, then set the new instance with Alloc.

[danmoseley]

To today if I call Start twice in a row, do I experience the same behavior as if I called Start once? Ie, it's effectively a no-op? If so then it seems to me that we don't have any good reason to break them. We essentially document that any redundant Start calls are ignored, and preserve that behavior.
If instead today the user gets into a bad state, then starting to throw might be helpful.

[stephentoub]

Should we simply reuse the existing running instance if the user accidentally calls StartRaisingEvents more than once in a row?

Can you elaborate on what someone's code would look like to call StartRaisingEvents twice in a row? The method is private. What path through the code would enable that, other than erroneously using an FSW instance in parallel?

[carlossanlop]

Do we want it freed in StopRaisingEvents, or should it be freed only on FinalizeDispose?

[danmoseley]

If it is held by RunningInstance then it should presumably be freed in the Dispose of RunningInstance. And you call Dispose here unless you want to preserve the RunningInstance across Start/Stop calls. Which depends on the semantics of FSW. I'm guessing the semantics are that we release all native resources when you do Stop, in case it holds a handle to the directory being watched for example. So in that case StartRaisingEVents creates a RI and StopRaisingEvents disposes the RI

[danmoseley]

and of course Dispose should also release it in case StopRaisingEvents never got called.

[danmoseley]

nit, newline after.

[jkotas]

Should this GCHandle rather be in the RunningInstance ? RunningInstance has the callback that the GCHandle protects, it would be best if this GCHandle is close to where the protected callbacks are.

[jkotas]

You can actually simplify the RunningInstance to just use function pointer + GCHandle once you do that. The marshaled delegate that it uses right now is less efficient than function pointer + GCHandle.

[kevincathcart-cas]

There is a lot that is questionable here (in the underlying code, I'm not talking about the proposed changes per-se).

The RuntimeInstance is normally de-facto rooted while the FileSystemWatcher is rooted (at least up until the FileSystemWatcher is disabled or disposed). This happens though a rather roundabout route: One of its methods (CleanupEventStream()) is registered as a callback on the CancellationTokenSource that gets held by the FileSystemWatcher. But this feels like an awfully indirect way to ensure the RuntimeInstance remains rooted.

It is worth noting that CleanupEventStream() is thread-safe, and it needs to be, because it can get called from both the callback, and the from the the FileSystemWatcher's finalizer at pretty much the same time, just by chance, since both the callback and the finalizer will try to clean things up if the watcher is no longer reachable. However, this implies it is both the callback and the FileSystemWatcher's responsibility to stop the RuntimeInstance if the watcher becomes unreachable.

That would almost make sense if leaking RuntimeInstances could happen (i.e. having the CancellationTokenSource not reference a still executing RuntimeInstance), and if the RuntimeInstance also rooted itself, because it would let it eventually clean itself after the watcher has gone away when/if the callback gets triggered, but if this were supposed to be such a weird edge case failsafe, I would have expected a comment saying as much. Instead this just looks a lot like confusion of responsibilities.

It is also very much unclear why CleanupEventStream() bothers calling FSEventStreamStop() against the SafeHandle, when the SafeHandle does this itself in its ReleaseHandle method. Furthermore note the try finally around it. If that were actually needed, then why does the exact same call not need that same logic inside the SafeHandle's ReleaseHandle method?

---

@stephentoub's analysis would basically be that the RuntimeInstance was somehow not rooted via the CTS, so the FSW's finalizer does not trigger CleanupEventStream(). But the callback comes in, and enters CleanupEventStream() while racing the SafeHandle's finalizer. Sure that absolutely does seem like it could cause a crash, and one that having a GCHandle root the RuntimeInstance could avoid. However, I'm not clear on how the RuntimeInstance would end up not rooted by the CancellationTokenSource, while still allowing the callbacks to happen.

It sure looks like that would only happen if:
a) a FSW is invalidly being manipulated by multiple threads at once, such that two StartRaisingEvents() are running at once, or
b) MacOS's FSEventStreamStart function succeeds but returns false.

[jkotas]

I have posted #52019 with a different take on this issue.