Re: [PATCH] fsmonitor: eliminate call to deprecated FSEventStream function

[Victoria Dye <vdye@github.com>]

Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhostetler@github.com>
> 
> Replace the call to `FSEventStreamScheduleWithRunLoop()` function with
> the suggested `FSEventStreamSetDispatchQueue()` function.
> 
> The MacOS version of the builtin FSMonitor feature uses the
> `FSEventStreamScheduleWithRunLoop()` function to drive the event loop
> and process FSEvents from the system.  This routine has now been
> deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
> generates a warning when compiling calls to this function.  In
> DEVELOPER=1 mode, this now causes a compile error.

I just updated to MacOS 13 and have been building without 'DEVELOPER=1' to
work around this error, so thank you so much for fixing it! 

> 
> The `FSEventStreamSetDispatchQueue()` function is conceptually similar
> and is the suggested replacement.  However, there are some subtle
> thread-related differences.
> 
> Previously, the event stream would be processed by the
> `fsm_listen__loop()` thread while it was in the `CFRunLoopRun()`
> method.  (Conceptually, this was a blocking call on the lifetime of
> the event stream where our thread drove the event loop and individual
> events were handled by the `fsevent_callback()`.)
> 
> With the change, a "dispatch queue" is created and FSEvents will be
> processed by a hidden queue-related thread (that calls the
> `fsevent_callback()` on our behalf).  Our `fsm_listen__loop()` thread
> maintains the original blocking model by waiting on a mutex/condition
> variable pair while the hidden thread does all of the work.

It's unfortunate that Apple doesn't seem to have any documentation on how
they'd recommend migrating in either [1] or [2], but your approach sounds
straightforward. Rearranging the patch a bit...

[1] https://developer.apple.com/documentation/coreservices/1447824-fseventstreamschedulewithrunloop
[2] https://developer.apple.com/documentation/coreservices/1444164-fseventstreamsetdispatchqueue

> @@ -490,9 +499,11 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
>  
>  	data = state->listen_data;
>  
> -	data->rl = CFRunLoopGetCurrent();
> +	pthread_mutex_init(&data->dq_lock, NULL);
> +	pthread_cond_init(&data->dq_finished, NULL);
> +	data->dq = dispatch_queue_create("FSMonitor", NULL);
>  
> -	FSEventStreamScheduleWithRunLoop(data->stream, data->rl, kCFRunLoopDefaultMode);
> +	FSEventStreamSetDispatchQueue(data->stream, data->dq);
>  	data->stream_scheduled = 1;
>  
>  	if (!FSEventStreamStart(data->stream)) {

First, you create the dispatch queue and schedule the stream on it. The docs
mention "If there’s a problem scheduling the stream on the queue, an error
will be returned when you try to start the stream.", and that appears to be
handled by the 'if (!FEventStreamStart(data->stream))' that already exists. 

Looks good.

> @@ -501,7 +512,9 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)
>  	}
>  	data->stream_started = 1;
>  
> -	CFRunLoopRun();
> +	pthread_mutex_lock(&data->dq_lock);
> +	pthread_cond_wait(&data->dq_finished, &data->dq_lock);
> +	pthread_mutex_unlock(&data->dq_lock);
>  
>  	switch (data->shutdown_style) {
>  	case FORCE_ERROR_STOP:
> 

Then, you block on the 'dq_finished' condition variable until it indicates a
shutdown (forced or otherwise). The two causes for that are...

> @@ -379,8 +382,11 @@ force_shutdown:
>  	fsmonitor_batch__free_list(batch);
>  	string_list_clear(&cookie_list, 0);
>  
> +	pthread_mutex_lock(&data->dq_lock);
>  	data->shutdown_style = FORCE_SHUTDOWN;
> -	CFRunLoopStop(data->rl);
> +	pthread_cond_broadcast(&data->dq_finished);
> +	pthread_mutex_unlock(&data->dq_lock);
> +
>  	strbuf_release(&tmp);
>  	return;
>  }

...force shutdown due to a major change in the repo detected in
'fsevent_callback()' (e.g. moving the .git dir)...

> @@ -479,9 +486,11 @@ void fsm_listen__stop_async(struct fsmonitor_daemon_state *state)
>  	struct fsm_listen_data *data;
>  
>  	data = state->listen_data;
> -	data->shutdown_style = SHUTDOWN_EVENT;
>  
> -	CFRunLoopStop(data->rl);
> +	pthread_mutex_lock(&data->dq_lock);
> +	data->shutdown_style = SHUTDOWN_EVENT;
> +	pthread_cond_broadcast(&data->dq_finished);
> +	pthread_mutex_unlock(&data->dq_lock);
>  }
>  

...or, shutdown due to an 'fsm_listen__stop_async()' call from the cleanup
of 'fsmonitor_run_daemon_1()'.

Between those two, all of the possible "stop listener" scenarios seem to be
covered (as they were when using 'CFRunLoopStop()').

> @@ -471,6 +473,11 @@ void fsm_listen__dtor(struct fsmonitor_daemon_state *state)
>  		FSEventStreamRelease(data->stream);
>  	}
>  
> +	if (data->dq)
> +		dispatch_release(data->dq);
> +	pthread_cond_destroy(&data->dq_finished);
> +	pthread_mutex_destroy(&data->dq_lock);
> +
>  	FREE_AND_NULL(state->listen_data);
>  }

And finally, cleanup the queue, lock, and condition var in the destructor.

This all looks good to me. Practically, I'd like to see this merged sooner
rather than later to stop (myself and others working on MacOS 13) needing to
hack some way around the deprecation error, but I'll defer to others more
familiar with FSMonitor if there's anything I didn't spot.

Thanks again!