Re: [PATCH v5 27/30] fsmonitor--daemon: use a cookie file to sync with file system
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Feb 24, 2022, 16:45 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2202241742500.11118@tvgsbejvaqbjf.bet>
- In-Reply-To
- <f47a763dc260c57c6f411cde7b4b8826732a8c7a.1644612979.git.gitgitgadget@gmail.com>
Hi Jeff,
On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:
Show 18 quoted lines
> 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:
Show 18 quoted lines
> + /*
> + * 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.
Show 12 quoted lines
> @@ -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