Re: [PATCH v5 27/30] fsmonitor--daemon: use a cookie file to sync with file system

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Jeff,

On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Teach fsmonitor--daemon client threads to create a cookie file
> inside the .git directory and then wait until FS events for the
> cookie are observed by the FS listener thread.
>
> This helps address the racy nature of file system events by
> blocking the client response until the kernel has drained any
> event backlog.
>
> This is especially important on MacOS where kernel events are
> only issued with a limited frequency.  See the `latency` argument
> of `FSeventStreamCreate()`.  The kernel only signals every `latency`
> seconds, but does not guarantee that the kernel queue is completely
> drained, so we may have to wait more than one interval.  If we
> increase the frequency, the system is more likely to drop events.
> We avoid these issues by having each client thread create a unique
> cookie file and then wait until it is seen in the event stream.

It took a couple iterations of this cookie file business to become
robust... ;-)

About these NEEDSWORKs:

> +	/*
> +	 * Create the cookie file on disk and then wait for a notification
> +	 * that the listener thread has seen it.
> +	 */
> +	fd = open(cookie_pathname.buf, O_WRONLY | O_CREAT | O_EXCL, 0600);
> +	if (fd >= 0) {
> +		close(fd);
> +		unlink(cookie_pathname.buf);
> +
> +		/*
> +		 * NEEDSWORK: This is an infinite wait (well, unless another
> +		 * thread sends us an abort).  I'd like to change this to
> +		 * use `pthread_cond_timedwait()` and return an error/timeout
> +		 * and let the caller do the trivial response thing.
> +		 */
> +		while (cookie->result == FCIR_INIT)
> +			pthread_cond_wait(&state->cookies_cond,
> +					  &state->main_lock);

It would probably make sense to do this at some stage, but since we have
code that has seen quite a bit of real-world testing, I am in favor of
postponing this change to a later date.

> @@ -1063,6 +1284,11 @@ done:
>
>  	strbuf_release(&state.path_worktree_watch);
>  	strbuf_release(&state.path_gitdir_watch);
> +	strbuf_release(&state.path_cookie_prefix);
> +
> +	/*
> +	 * NEEDSWORK: Consider "rm -rf <gitdir>/<fsmonitor-dir>"
> +	 */
>
>  	return err;
>  }

In this instance, I think we can just drop the `NEEDSWORK`: it makes sense
to keep around these directories rather than destroying and re-creating
them over and over again.

Ciao,
Dscho