Fix stop sequence for FSEvents monitor

[t3hk0d3]

If you stop monitor right after starting it in a separate thread - it might result in segfault and warnings like these:

2017-11-23 23:42 ruby[9437] (FSEvents.framework) streamRef->cfRunLoopSourceRef != NULL || streamRef->event_source != NULL(): failed assertion: Must call FSEventStreamScheduleWithRunLoop() before calling FSEventStreamInvalidate()

2017-11-23 23:42 ruby[9437] (FSEvents.framework) streamRef->cfRunLoopSourceRef == NULL && streamRef->event_source == NULL(): failed assertion: FSEventStream was released (causing it to be deallocated) without calling FSEventStreamInvalidate()

Code to reproduce (slightly modified test/src/fswatch_test.c) https://gist.github.com/t3hk0d3/a37fa9ca515ad27bbb8b756120828587

Idea is to have FSEvents deinitialization logic on same thread as initialization, while on_stop should just interrupt (stop) runloop.

It doesn't change deinitialization itself, so it should be backward-compatible.

This seems to be proper solution for me from all aspects, so you won't need to put any sleep timers before stopping monitor :)

[emcrisostomo]

Hi @t3hk0d3,

Doesn't moving run_loop_lock.unlock(); after the call to FSEventStreamScheduleWithRunLoop guarantee that the call to FSEventStreamInvalidate always happens after FSEventStreamScheduleWithRunLoop? Then on_stop() would acquire the lock to run_loop_lock and stop the monitor without the need of moving those calls to run(). Or am I missing something else? As far as the sleep timers you were mentioning: I don't see a need for any of that in this case.

[t3hk0d3]

@emcrisostomo
Thats true, but there is another thing - having deinitialization in same thread (context) as initialization is always good idea. For example if for some reason CFRunLoopRun would return without on_stop being called - FSEvents would not get deinitialized properly.
Imho its more bulletproof solution.

[emcrisostomo]

I agree on that argument. I was focused on the locking stuff, to be sure I wasn't missing anything, since my memory is blurred. Ok, merged.

I'll review the run/stop protocol in that monitor thoroughly because I'm actually thinking there's a race condition there: run_loop may be accessed by on_stop() without an happens-before relationship with run_loop = CFRunLoopGetCurrent();. I'm not sure, I have to take time and review it.

Thanks for the PR.

[t3hk0d3]

@emcrisostomo You are welcome. Thanks for merging :D

[t3hk0d3]

@emcrisostomo
There is little risk that CFRunLoopStop would happen right before CFRunLoopRun is still possible. I was thinking about checking that run_loop is actually running using CFRunLoopIsWaiting, but that is outside of topic of this PR.

[emcrisostomo]

Hi @t3hk0d3, I'm aware of that race condition since I've written the monitor. IIRC CFRunLoopIsWaiting is not sufficient though, because it may answer that the loop is not waiting when the loop it's processing a source. However, since we cannot execute anything after CFRunLoopRun (because it's a blocking call), we may have little choice other than some spin waiting.