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.
Co-authored-by: Kevin Willford <Kevin.Willford@xxxxxxxxxxxxx>
Co-authored-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
---
builtin/fsmonitor--daemon.c | 198 ++++++++++++++++++++++++++++++++++++
fsmonitor--daemon.h | 5 +
2 files changed, 203 insertions(+)
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 0cb09ef0b984..d6b59a98cedd 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -150,6 +150,149 @@ static int do_as_client__send_flush(void)
return 0;
}
+enum fsmonitor_cookie_item_result {
+ FCIR_ERROR = -1, /* could not create cookie file ? */
+ FCIR_INIT = 0,
+ FCIR_SEEN,
+ FCIR_ABORT,
+};
+
+struct fsmonitor_cookie_item {
+ struct hashmap_entry entry;
+ const char *name;
+ enum fsmonitor_cookie_item_result result;
+};
+
+static int cookies_cmp(const void *data, const struct hashmap_entry *he1,
+ const struct hashmap_entry *he2, const void *keydata)
+{
+ const struct fsmonitor_cookie_item *a =
+ container_of(he1, const struct fsmonitor_cookie_item, entry);
+ const struct fsmonitor_cookie_item *b =
+ container_of(he2, const struct fsmonitor_cookie_item, entry);
+
+ return strcmp(a->name, keydata ? keydata : b->name);
+}
+
+static enum fsmonitor_cookie_item_result fsmonitor_wait_for_cookie(
+ struct fsmonitor_daemon_state *state)
+{
+ int fd;
+ struct fsmonitor_cookie_item cookie;
+ struct strbuf cookie_pathname = STRBUF_INIT;
+ struct strbuf cookie_filename = STRBUF_INIT;
+ const char *slash;
+ int my_cookie_seq;
+
+ pthread_mutex_lock(&state->main_lock);
+
+ my_cookie_seq = state->cookie_seq++;
+
+ strbuf_addbuf(&cookie_pathname, &state->path_cookie_prefix);
+ strbuf_addf(&cookie_pathname, "%i-%i", getpid(), my_cookie_seq);
+
+ slash = find_last_dir_sep(cookie_pathname.buf);
+ if (slash)
+ strbuf_addstr(&cookie_filename, slash + 1);
+ else
+ strbuf_addbuf(&cookie_filename, &cookie_pathname);
+ cookie.name = strbuf_detach(&cookie_filename, NULL);
+ cookie.result = FCIR_INIT;
+ // TODO should we have case-insenstive hash (and in cookie_cmp()) ??
+ hashmap_entry_init(&cookie.entry, strhash(cookie.name));
+
+ /*
+ * Warning: we are putting the address of a stack variable into a
+ * global hashmap. This feels dodgy. We must ensure that we remove
+ * it before this thread and stack frame returns.
+ */
+ hashmap_add(&state->cookies, &cookie.entry);
+
+ trace_printf_key(&trace_fsmonitor, "cookie-wait: '%s' '%s'",
+ cookie.name, cookie_pathname.buf);
+
+ /*
+ * 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_or_warn(cookie_pathname.buf);
+
+ while (cookie.result == FCIR_INIT)
+ pthread_cond_wait(&state->cookies_cond,
+ &state->main_lock);
+
+ hashmap_remove(&state->cookies, &cookie.entry, NULL);
+ } else {
+ error_errno(_("could not create fsmonitor cookie '%s'"),
+ cookie.name);
+
+ cookie.result = FCIR_ERROR;
+ hashmap_remove(&state->cookies, &cookie.entry, NULL);
+ }
+
+ pthread_mutex_unlock(&state->main_lock);
+
+ free((char*)cookie.name);
+ strbuf_release(&cookie_pathname);
+ return cookie.result;
+}
+
+/*
+ * Mark these cookies as _SEEN and wake up the corresponding client threads.
+ */
+static void fsmonitor_cookie_mark_seen(struct fsmonitor_daemon_state *state,
+ const struct string_list *cookie_names)
+{
+ /* assert state->main_lock */
+
+ int k;
+ int nr_seen = 0;
+
+ for (k = 0; k < cookie_names->nr; k++) {
+ struct fsmonitor_cookie_item key;
+ struct fsmonitor_cookie_item *cookie;
+
+ key.name = cookie_names->items[k].string;
+ hashmap_entry_init(&key.entry, strhash(key.name));
+
+ cookie = hashmap_get_entry(&state->cookies, &key, entry, NULL);
+ if (cookie) {
+ trace_printf_key(&trace_fsmonitor, "cookie-seen: '%s'",
+ cookie->name);
+ cookie->result = FCIR_SEEN;
+ nr_seen++;
+ }
+ }
+
+ if (nr_seen)
+ pthread_cond_broadcast(&state->cookies_cond);
+}
+
+/*
+ * Set _ABORT on all pending cookies and wake up all client threads.
+ */
+static void fsmonitor_cookie_abort_all(struct fsmonitor_daemon_state *state)
+{
+ /* assert state->main_lock */
+
+ struct hashmap_iter iter;
+ struct fsmonitor_cookie_item *cookie;
+ int nr_aborted = 0;
+
+ hashmap_for_each_entry(&state->cookies, &iter, cookie, entry) {
+ trace_printf_key(&trace_fsmonitor, "cookie-abort: '%s'",
+ cookie->name);
+ cookie->result = FCIR_ABORT;
+ nr_aborted++;
+ }
+
+ if (nr_aborted)
+ pthread_cond_broadcast(&state->cookies_cond);
+}
+
static int lookup_client_test_delay(void)
{
static int delay_ms = -1;
@@ -435,6 +578,9 @@ static void fsmonitor_free_token_data(struct fsmonitor_token_data *token)
* We should create a new token and start fresh (as if we just
* booted up).
*
+ * [2] Some of those lost events may have been for cookie files. We
+ * should assume the worst and abort them rather letting them starve.
+ *
* If there are no readers of the the current token data series, we
* can free it now. Otherwise, let the last reader free it. Either
* way, the old token data series is no longer associated with our
@@ -454,6 +600,8 @@ void fsmonitor_force_resync(struct fsmonitor_daemon_state *state)
state->current_token_data->token_id.buf,
new_one->token_id.buf);
+ fsmonitor_cookie_abort_all(state);
+
if (state->current_token_data->client_ref_count == 0)
free_me = state->current_token_data;
state->current_token_data = new_one;
@@ -526,6 +674,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
kh_str_t *shown;
int hash_ret;
int result;
+ enum fsmonitor_cookie_item_result cookie_result;
/*
* We expect `command` to be of the form:
@@ -654,6 +803,39 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
goto send_trivial_response;
}
+ pthread_mutex_unlock(&state->main_lock);
+
+ /*
+ * Write a cookie file inside the directory being watched in an
+ * effort to flush out existing filesystem events that we actually
+ * care about. Suspend this client thread until we see the filesystem
+ * events for this cookie file.
+ */
+ cookie_result = fsmonitor_wait_for_cookie(state);
+ if (cookie_result != FCIR_SEEN) {
+ error(_("fsmonitor: cookie_result '%d' != SEEN"),
+ cookie_result);
+ result = 0;
+ goto send_trivial_response;
+ }
+
+ pthread_mutex_lock(&state->main_lock);
+
+ if (strcmp(requested_token_id.buf,
+ state->current_token_data->token_id.buf)) {
+ /*
+ * Ack! The listener thread lost sync with the filesystem
+ * and created a new token while we were waiting for the
+ * cookie file to be created! Just give up.
+ */
+ pthread_mutex_unlock(&state->main_lock);
+
+ trace_printf_key(&trace_fsmonitor,
+ "lost filesystem sync");
+ result = 0;
+ goto send_trivial_response;
+ }
+
/*
* We're going to hold onto a pointer to the current
* token-data while we walk the list of batches of files.
@@ -982,6 +1164,9 @@ void fsmonitor_publish(struct fsmonitor_daemon_state *state,
}
}
+ if (cookie_names->nr)
+ fsmonitor_cookie_mark_seen(state, cookie_names);
+
pthread_mutex_unlock(&state->main_lock);
}
@@ -1071,7 +1256,9 @@ static int fsmonitor_run_daemon(void)
memset(&state, 0, sizeof(state));
+ hashmap_init(&state.cookies, cookies_cmp, NULL, 0);
pthread_mutex_init(&state.main_lock, NULL);
+ pthread_cond_init(&state.cookies_cond, NULL);
state.error_code = 0;
state.current_token_data = fsmonitor_new_token_data();
state.test_client_delay_ms = lookup_client_test_delay();
@@ -1094,6 +1281,15 @@ static int fsmonitor_run_daemon(void)
state.nr_paths_watching = 2;
}
+ /*
+ * We will write filesystem syncing cookie files into
+ * <gitdir>/<cookie-prefix><pid>-<seq>.
+ */
+ strbuf_init(&state.path_cookie_prefix, 0);
+ strbuf_addbuf(&state.path_cookie_prefix, &state.path_gitdir_watch);
+ strbuf_addch(&state.path_cookie_prefix, '/');
+ strbuf_addstr(&state.path_cookie_prefix, FSMONITOR_COOKIE_PREFIX);
+
/*
* Confirm that we can create platform-specific resources for the
* filesystem listener before we bother starting all the threads.
@@ -1106,6 +1302,7 @@ static int fsmonitor_run_daemon(void)
err = fsmonitor_run_daemon_1(&state);
done:
+ pthread_cond_destroy(&state.cookies_cond);
pthread_mutex_destroy(&state.main_lock);
fsmonitor_fs_listen__dtor(&state);
@@ -1113,6 +1310,7 @@ static int fsmonitor_run_daemon(void)
strbuf_release(&state.path_worktree_watch);
strbuf_release(&state.path_gitdir_watch);
+ strbuf_release(&state.path_cookie_prefix);
return err;
}
diff --git a/fsmonitor--daemon.h b/fsmonitor--daemon.h
index 06563b6ed56c..4e580e285ed6 100644
--- a/fsmonitor--daemon.h
+++ b/fsmonitor--daemon.h
@@ -45,6 +45,11 @@ struct fsmonitor_daemon_state {
struct fsmonitor_token_data *current_token_data;
+ struct strbuf path_cookie_prefix;
+ pthread_cond_t cookies_cond;
+ int cookie_seq;
+ struct hashmap cookies;
+
int error_code;
struct fsmonitor_daemon_backend_data *backend_data;
--
gitgitgadget
