Re: [PATCH v5 27/30] fsmonitor--daemon: use a cookie file to sync with file system
From: Johannes Schindelin <hidden>
Date: 2022-02-24 16:46:06
Hi Jeff, On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:
From: Jeff Hostetler <redacted> 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.
quoted hunk ↗ jump to hunk
@@ -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