Hi Jeff,
On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>
> 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
