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

From: Victoria Dye <vdye@github.com>
Date: Dec 2, 2022, 17:42 UTC
Message-ID: <43769297-8e31-9420-5d2e-dd1d400b5358@github.com>
In-Reply-To: <pull.1436.git.1669991072393.gitgitgadget@gmail.com>

Jeff Hostetler via GitGitGadget wrote:

Show 11 quoted lines

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

Show 16 quoted lines

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

Show 14 quoted lines

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

Show 12 quoted lines

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

Show 13 quoted lines

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

Show 13 quoted lines

> @@ -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()').

Show 11 quoted lines

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