Share CFRunLoop for FSEventStreams in FileSystemWatcher on OSX by MaximLipnin · Pull Request #33865 · dotnet/corefx

MaximLipnin · 2018-12-06T15:08:48Z

This is an attempt to solve the problem https://github.com/dotnet/corefx/issues/30600 using the idea of sharing CFRunLoop for FSEventStreams.

I added StaticWatcherRunLoopManager class to share RunLoop among FSEventStreams and updated Start and CancellationCallback methods.

I applied these changes to the latest Mono and used Visual Studio Community 2017 for Mac Version 7.7 (build 1868) to run a sample with 200 FileSystemWatcher instances. mono/mono#11945

It showed reduced number of used threads.

MaximLipnin · 2018-12-06T17:35:05Z

/cc @marek-safar

MaximLipnin · 2018-12-07T07:53:16Z

@dotnet-bot test Windows x64 Debug Build

MaximLipnin · 2018-12-07T07:53:57Z

@dotnet-bot test Windows x86 Release Build

stephentoub

Thanks for working on this.

stephentoub · 2018-12-07T10:55:27Z

            {
-                lock (StopLock)
+                // A reference to the RunLoop that we can use to start or stop a Watcher
+                private static CFRunLoopRef _watcherRunLoop = IntPtr.Zero;


Nit: s_watcherRunLoop

stephentoub · 2018-12-07T10:55:38Z

+                // A reference to the RunLoop that we can use to start or stop a Watcher
+                private static CFRunLoopRef _watcherRunLoop = IntPtr.Zero;
+
+                private static int scheduledStreamsCount = 0;


Nit: s_scheduledStreamsCount

stephentoub · 2018-12-07T10:55:47Z

+
+                private static int scheduledStreamsCount = 0;
+
+                private static object lockObject = new object();


Nit: readonly
Nit: s_lockObject

stephentoub · 2018-12-07T10:56:19Z

+
+                public static void ScheduleEventStream(SafeEventStreamHandle eventStream)
+                {
+                    if(_watcherRunLoop == IntPtr.Zero)


Nit: space before open paren

stephentoub · 2018-12-07T10:58:22Z

+                                new Thread(WatchForFileSystemEventsThreadStart) { IsBackground = true }.Start(new object[] { runLoopStarted, eventStream });
+                                runLoopStarted.Wait();
+
+                                Interlocked.Increment(ref scheduledStreamsCount);


This increment needs to be done before the thread is started to avoid a race condition.

Is starting/stopping FileSystemWatcher generally a hotly contended path? I wouldn't think so. In which case, I'd prefer we avoid trying to be lock-free and instead just use a lock, where we have a better chance of getting the synchronization correct.

stephentoub · 2018-12-07T11:00:54Z

+                        Interop.EventStream.FSEventStreamUnscheduleFromRunLoop(eventStream, _watcherRunLoop, Interop.RunLoop.kCFRunLoopDefaultMode);
+                        Interlocked.Decrement(ref scheduledStreamsCount);
+
+                        if (scheduledStreamsCount == 0)


This isn't thread-safe. It should be using the return value of Decrement.

Also what happens if this thread decrements to 0, goes to stop the loop, and then another thread increments and assumes it's already been started?

stephentoub · 2018-12-07T11:03:47Z

+
+
+                    // Schedule the EventStream to run on the thread's RunLoop
+                    Interop.EventStream.FSEventStreamScheduleWithRunLoop(_eventStream, _watcherRunLoop, Interop.RunLoop.kCFRunLoopDefaultMode);


Why is the first call / the one spinning up the thread different from all of the others? i.e. why does this call need to be part of WatchForFilesystemEventsThreadStart rather than just being in ScheduleEventStream?

RunLoop won't run without any scheduled event stream.

stephentoub · 2018-12-07T11:05:31Z

+
+            private void CancellationCallback()
+            {
+                lock (StopLock) {


Shouldn't this be the same lock object that's used to coordinate other activities within StaticWatcherRunLoopManager?

stephentoub · 2018-12-07T11:06:57Z

+                bool started = Interop.EventStream.FSEventStreamStart(_eventStream);
+                if (!started)
+                {  
+                    // Try to get the Watcher to raise the error event; if we can't do that, just silently exist since the watcher is gone anyway


Nit: preexisting your change, but exist => exit

stephentoub · 2018-12-07T11:14:45Z

cc: @JeremyKuhne

stephentoub · 2018-12-09T22:56:35Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

stephentoub · 2018-12-10T02:38:00Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

stephentoub · 2018-12-10T10:39:02Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-11T13:53:51Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-11T14:35:38Z

@dotnet-bot test OSX x64 Debug Build please

danmoseley · 2018-12-12T00:26:07Z

@stephentoub good to merge?

stephentoub

Thanks for working on this. Looking pretty good. A few more comments.

stephentoub · 2018-12-12T13:44:23Z

+
+                public static void UnscheduleFromRunLoop(SafeEventStreamHandle eventStream)
+                {
+                    if (s_watcherRunLoop != IntPtr.Zero)


In what situation would UnscheduleFromRunLoop be called with s_watcherRunLoop == IntPtr.Zero? And do we really need the double-checked locking here, or could this outer check be removed?

Can you remove the outer check and add an assert that s_watcherRunLoop isn't zero?

stephentoub · 2018-12-12T14:01:00Z

+                        lock(s_lockObject)
+                        {                    
+                            Interop.CoreFoundation.CFRelease(runLoop);                                  
+                            s_watcherRunLoop = IntPtr.Zero;


This looks like a bug / race condition. What happens if the last watcher gets removed and then immediately after that a new one is created? We could end up stomping here over the new value set by the latter.

Is this write even necessary? Aren't we already setting this to zero when unscheduling the last watcher?

This should be removed, right?

MaximLipnin · 2018-12-14T11:35:02Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-14T15:00:57Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-14T19:22:39Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-16T20:43:56Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-17T05:52:59Z

@dotnet-bot test OSX x64 Debug Build please

MaximLipnin · 2018-12-17T05:53:09Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-17T05:53:33Z

@dotnet-bot test Windows x86 Release Build please

# Conflicts: # src/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Create.cs

MaximLipnin · 2018-12-18T13:49:12Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-18T16:02:38Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-18T20:47:49Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

stephentoub · 2018-12-19T01:55:01Z

-        [OuterLoop]
        [Fact]
-        public void FileSystemWatcher_File_Create_MultipleWatchers_ExecutionContextFlowed()
+        public void FileSystemWatcher_File_Create_EnablingDisablingNotAffectRaisingEvent()


Could this use the ExpectEvent helper? Or if not, use ExecuteWithRetry around the whole body? We've had too many CI failures due to FSW flakiness, so we have retries in pretty much all of our FSW tests that actually expect events. The ExpectEvent helper has a built-in retry.

wrapped in ExecuteWithRetry helper method

stephentoub · 2018-12-19T01:55:52Z

-        [OuterLoop]
        [Fact]
-        public void FileSystemWatcher_File_Create_MultipleWatchers_ExecutionContextFlowed()
+        public void FileSystemWatcher_File_Create_EnablingDisablingNotAffectRaisingEvent()


We should probably make this Outerloop as well, since it's going to end up creating 100 threads, and that could in theory cause issues for other concurrently running tests if the CI machine was resource starved.

stephentoub · 2018-12-19T01:56:33Z

+        [OuterLoop]
+        [Fact]
+        public void FileSystemWatcher_File_Create_WatchOwnPath()
+        {


Same ExecuteWithRetry comment/question.

stephentoub · 2018-12-19T01:57:22Z

+            {
+                string fileName = Path.Combine(TestDirectory, "file");
+                AutoResetEvent[] autoResetEvents = new AutoResetEvent[64];
+                FileSystemWatcher[] watchers = new FileSystemWatcher[64];


We should probably dispose all of these in a finally: that way we'll promptly tear them down when the test completes, even in the case of an exception or assert failure.

stephentoub

Thanks!

…vent test in ExecuteWithRetry helper method and mark it with Outerloop

…try helper method

…leSystemWatcher_File_Create_ForceLoopRestart test

MaximLipnin · 2018-12-19T08:35:10Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

MaximLipnin · 2018-12-19T11:47:21Z

@dotnet-bot test Outerloop OSX x64 Debug Build please

stephentoub · 2018-12-19T11:51:20Z

@dotnet-bot test Outerloop Linux x64 Debug Build please
@dotnet-bot test Outerloop Windows x64 Debug Build please

MaximLipnin · 2018-12-19T16:54:30Z

@dotnet-bot test Outerloop Linux x64 Debug Build please

stephentoub · 2018-12-20T01:59:26Z

Thanks!

MaximLipnin · 2018-12-20T06:25:41Z

Thank you for review and help!

…t/corefx#33865) * Share CFRunLoop for FSEventStreams in FileSystemWatcher on OSX * Address review comments * Add execution context capturing * Fix nits * Add some tests to check multiple watchers * Address review comments * Update FileSystemWatcher tests * Add timeout for non-expected events * Wrap FileSystemWatcher_File_Create_EnablingDisablingNotAffectRaisingEvent test in ExecuteWithRetry helper method and mark it with Outerloop * Wrap FileSystemWatcher_File_Create_WatchOwnPath test in ExecuteWithRetry helper method * Move disposing FileSystemWatcher instances to a finally section in FileSystemWatcher_File_Create_ForceLoopRestart test * Use null-conditional operator before disposing Commit migrated from dotnet/corefx@8a06434

Share CFRunLoop for FSEventStreams in FileSystemWatcher on OSX

635858a

stephentoub reviewed Dec 7, 2018

View reviewed changes

MaximLipnin changed the title ~~Share CFRunLoop for FSEventStreams in FileSystemWatcher on OSX~~ [WIP] Share CFRunLoop for FSEventStreams in FileSystemWatcher on OSX Dec 10, 2018

Address review comments

4374be6

MaximLipnin added 2 commits December 11, 2018 15:22

Merge branch 'upstream-master' into fsw

c5f6d4e

Add execution context capturing

158ba9a

MaximLipnin changed the title ~~[WIP] Share CFRunLoop for FSEventStreams in FileSystemWatcher on OSX~~ Share CFRunLoop for FSEventStreams in FileSystemWatcher on OSX Dec 11, 2018

stephentoub reviewed Dec 12, 2018

View reviewed changes

MaximLipnin added 2 commits December 14, 2018 14:24

Fix nits

53af698

Add some tests to check multiple watchers

68ec81d

stephentoub reviewed Dec 14, 2018

View reviewed changes

Comment thread src/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.OSX.cs Outdated

Merge branch 'upstream-master' into fsw

7c1a8bc

# Conflicts: # src/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.File.Create.cs

MaximLipnin force-pushed the fsw branch from 5ace897 to 7c1a8bc Compare December 17, 2018 10:02

Update FileSystemWatcher tests

248083e

Add timeout for non-expected events

2cba0ce

stephentoub reviewed Dec 19, 2018

View reviewed changes

stephentoub approved these changes Dec 19, 2018

View reviewed changes

MaximLipnin added 3 commits December 19, 2018 10:41

Wrap FileSystemWatcher_File_Create_EnablingDisablingNotAffectRaisingE…

5e9ffaa

…vent test in ExecuteWithRetry helper method and mark it with Outerloop

Wrap FileSystemWatcher_File_Create_WatchOwnPath test in ExecuteWithRe…

eed6532

…try helper method

Move disposing FileSystemWatcher instances to a finally section in Fi…

90bca12

…leSystemWatcher_File_Create_ForceLoopRestart test

Use null-conditional operator before disposing

081985c

stephentoub approved these changes Dec 19, 2018

View reviewed changes

karelz added the area-System.IO label Dec 19, 2018

karelz assigned MaximLipnin and JeremyKuhne Dec 19, 2018

stephentoub merged commit 8a06434 into dotnet:master Dec 20, 2018

MaximLipnin deleted the fsw branch December 20, 2018 06:25

MaximLipnin mentioned this pull request Dec 21, 2018

Bring changes from corefx 33865 mono/corefx#225

Merged

karelz added this to the 3.0 milestone Dec 21, 2018

MaximLipnin mentioned this pull request Dec 27, 2018

Avoid a crash in FileSystemWatcher when UnscheduleFromRunLoop is called from the finalizer on shutdown mono/corefx#228

Merged


		private static int scheduledStreamsCount = 0;

		private static object lockObject = new object();



		// Schedule the EventStream to run on the thread's RunLoop
		Interop.EventStream.FSEventStreamScheduleWithRunLoop(_eventStream, _watcherRunLoop, Interop.RunLoop.kCFRunLoopDefaultMode);

Conversation

MaximLipnin commented Dec 6, 2018

Uh oh!

MaximLipnin commented Dec 6, 2018

Uh oh!

MaximLipnin commented Dec 7, 2018

Uh oh!

MaximLipnin commented Dec 7, 2018

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentoub commented Dec 7, 2018

Uh oh!

stephentoub commented Dec 9, 2018

Uh oh!

stephentoub commented Dec 10, 2018

Uh oh!

stephentoub commented Dec 10, 2018

Uh oh!

MaximLipnin commented Dec 11, 2018

Uh oh!

MaximLipnin commented Dec 11, 2018

Uh oh!

danmoseley commented Dec 12, 2018

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaximLipnin commented Dec 14, 2018

Uh oh!

MaximLipnin commented Dec 14, 2018

Uh oh!

Uh oh!

MaximLipnin commented Dec 14, 2018

Uh oh!

MaximLipnin commented Dec 16, 2018

Uh oh!

MaximLipnin commented Dec 17, 2018

Uh oh!

MaximLipnin commented Dec 17, 2018