Skip to content

Commit a511ade

Browse files
jeffhostetlerkewillforddscho
committed
fsmonitor--daemon: use a cookie file to sync with file system
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 <[email protected]> Co-authored-by: Johannes Schindelin <[email protected]> Signed-off-by: Jeff Hostetler <[email protected]>
1 parent 4c1167f commit a511ade

File tree

2 files changed

+232
-1
lines changed

2 files changed

+232
-1
lines changed

builtin/fsmonitor--daemon.c

Lines changed: 227 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,149 @@ static int do_as_client__status(void)
9494
}
9595
}
9696

97+
enum fsmonitor_cookie_item_result {
98+
FCIR_ERROR = -1, /* could not create cookie file ? */
99+
FCIR_INIT = 0,
100+
FCIR_SEEN,
101+
FCIR_ABORT,
102+
};
103+
104+
struct fsmonitor_cookie_item {
105+
struct hashmap_entry entry;
106+
const char *name;
107+
enum fsmonitor_cookie_item_result result;
108+
};
109+
110+
static int cookies_cmp(const void *data, const struct hashmap_entry *he1,
111+
const struct hashmap_entry *he2, const void *keydata)
112+
{
113+
const struct fsmonitor_cookie_item *a =
114+
container_of(he1, const struct fsmonitor_cookie_item, entry);
115+
const struct fsmonitor_cookie_item *b =
116+
container_of(he2, const struct fsmonitor_cookie_item, entry);
117+
118+
return strcmp(a->name, keydata ? keydata : b->name);
119+
}
120+
121+
static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(
122+
struct fsmonitor_daemon_state *state)
123+
{
124+
/* assert current thread holding state->main_lock */
125+
126+
int fd;
127+
struct fsmonitor_cookie_item *cookie;
128+
struct strbuf cookie_pathname = STRBUF_INIT;
129+
struct strbuf cookie_filename = STRBUF_INIT;
130+
enum fsmonitor_cookie_item_result result;
131+
int my_cookie_seq;
132+
133+
CALLOC_ARRAY(cookie, 1);
134+
135+
my_cookie_seq = state->cookie_seq++;
136+
137+
strbuf_addf(&cookie_filename, "%i-%i", getpid(), my_cookie_seq);
138+
139+
strbuf_addbuf(&cookie_pathname, &state->path_cookie_prefix);
140+
strbuf_addbuf(&cookie_pathname, &cookie_filename);
141+
142+
cookie->name = strbuf_detach(&cookie_filename, NULL);
143+
cookie->result = FCIR_INIT;
144+
hashmap_entry_init(&cookie->entry, strhash(cookie->name));
145+
146+
hashmap_add(&state->cookies, &cookie->entry);
147+
148+
trace_printf_key(&trace_fsmonitor, "cookie-wait: '%s' '%s'",
149+
cookie->name, cookie_pathname.buf);
150+
151+
/*
152+
* Create the cookie file on disk and then wait for a notification
153+
* that the listener thread has seen it.
154+
*/
155+
fd = open(cookie_pathname.buf, O_WRONLY | O_CREAT | O_EXCL, 0600);
156+
if (fd >= 0) {
157+
close(fd);
158+
unlink(cookie_pathname.buf);
159+
160+
/*
161+
* NEEDSWORK: This is an infinite wait (well, unless another
162+
* thread sends us an abort). I'd like to change this to
163+
* use `pthread_cond_timedwait()` and return an error/timeout
164+
* and let the caller do the trivial response thing.
165+
*/
166+
while (cookie->result == FCIR_INIT)
167+
pthread_cond_wait(&state->cookies_cond,
168+
&state->main_lock);
169+
} else {
170+
error_errno(_("could not create fsmonitor cookie '%s'"),
171+
cookie->name);
172+
173+
cookie->result = FCIR_ERROR;
174+
}
175+
176+
hashmap_remove(&state->cookies, &cookie->entry, NULL);
177+
178+
result = cookie->result;
179+
180+
free((char*)cookie->name);
181+
free(cookie);
182+
strbuf_release(&cookie_pathname);
183+
184+
return result;
185+
}
186+
187+
/*
188+
* Mark these cookies as _SEEN and wake up the corresponding client threads.
189+
*/
190+
static void with_lock__mark_cookies_seen(struct fsmonitor_daemon_state *state,
191+
const struct string_list *cookie_names)
192+
{
193+
/* assert current thread holding state->main_lock */
194+
195+
int k;
196+
int nr_seen = 0;
197+
198+
for (k = 0; k < cookie_names->nr; k++) {
199+
struct fsmonitor_cookie_item key;
200+
struct fsmonitor_cookie_item *cookie;
201+
202+
key.name = cookie_names->items[k].string;
203+
hashmap_entry_init(&key.entry, strhash(key.name));
204+
205+
cookie = hashmap_get_entry(&state->cookies, &key, entry, NULL);
206+
if (cookie) {
207+
trace_printf_key(&trace_fsmonitor, "cookie-seen: '%s'",
208+
cookie->name);
209+
cookie->result = FCIR_SEEN;
210+
nr_seen++;
211+
}
212+
}
213+
214+
if (nr_seen)
215+
pthread_cond_broadcast(&state->cookies_cond);
216+
}
217+
218+
/*
219+
* Set _ABORT on all pending cookies and wake up all client threads.
220+
*/
221+
static void with_lock__abort_all_cookies(struct fsmonitor_daemon_state *state)
222+
{
223+
/* assert current thread holding state->main_lock */
224+
225+
struct hashmap_iter iter;
226+
struct fsmonitor_cookie_item *cookie;
227+
int nr_aborted = 0;
228+
229+
hashmap_for_each_entry(&state->cookies, &iter, cookie, entry) {
230+
trace_printf_key(&trace_fsmonitor, "cookie-abort: '%s'",
231+
cookie->name);
232+
cookie->result = FCIR_ABORT;
233+
nr_aborted++;
234+
}
235+
236+
if (nr_aborted)
237+
pthread_cond_broadcast(&state->cookies_cond);
238+
}
239+
97240
/*
98241
* Requests to and from a FSMonitor Protocol V2 provider use an opaque
99242
* "token" as a virtual timestamp. Clients can request a summary of all
@@ -391,6 +534,9 @@ static void fsmonitor_free_token_data(struct fsmonitor_token_data *token)
391534
* We should create a new token and start fresh (as if we just
392535
* booted up).
393536
*
537+
* [2] Some of those lost events may have been for cookie files. We
538+
* should assume the worst and abort them rather letting them starve.
539+
*
394540
* If there are no concurrent threads readering the current token data
395541
* series, we can free it now. Otherwise, let the last reader free
396542
* it.
@@ -412,6 +558,8 @@ static void with_lock__do_force_resync(struct fsmonitor_daemon_state *state)
412558
state->current_token_data = new_one;
413559

414560
fsmonitor_free_token_data(free_me);
561+
562+
with_lock__abort_all_cookies(state);
415563
}
416564

417565
void fsmonitor_force_resync(struct fsmonitor_daemon_state *state)
@@ -487,6 +635,8 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
487635
int hash_ret;
488636
int do_trivial = 0;
489637
int do_flush = 0;
638+
int do_cookie = 0;
639+
enum fsmonitor_cookie_item_result cookie_result;
490640

491641
/*
492642
* We expect `command` to be of the form:
@@ -547,6 +697,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
547697
* We have a V2 valid token:
548698
* "builtin:<token_id>:<seq_nr>"
549699
*/
700+
do_cookie = 1;
550701
}
551702
}
552703

@@ -555,6 +706,30 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
555706
if (!state->current_token_data)
556707
BUG("fsmonitor state does not have a current token");
557708

709+
/*
710+
* Write a cookie file inside the directory being watched in
711+
* an effort to flush out existing filesystem events that we
712+
* actually care about. Suspend this client thread until we
713+
* see the filesystem events for this cookie file.
714+
*
715+
* Creating the cookie lets us guarantee that our FS listener
716+
* thread has drained the kernel queue and we are caught up
717+
* with the kernel.
718+
*
719+
* If we cannot create the cookie (or otherwise guarantee that
720+
* we are caught up), we send a trivial response. We have to
721+
* assume that there might be some very, very recent activity
722+
* on the FS still in flight.
723+
*/
724+
if (do_cookie) {
725+
cookie_result = with_lock__wait_for_cookie(state);
726+
if (cookie_result != FCIR_SEEN) {
727+
error(_("fsmonitor: cookie_result '%d' != SEEN"),
728+
cookie_result);
729+
do_trivial = 1;
730+
}
731+
}
732+
558733
if (do_flush)
559734
with_lock__do_force_resync(state);
560735

@@ -775,7 +950,9 @@ static int handle_client(void *data,
775950
return result;
776951
}
777952

778-
#define FSMONITOR_COOKIE_PREFIX ".fsmonitor-daemon-"
953+
#define FSMONITOR_DIR "fsmonitor--daemon"
954+
#define FSMONITOR_COOKIE_DIR "cookies"
955+
#define FSMONITOR_COOKIE_PREFIX (FSMONITOR_DIR "/" FSMONITOR_COOKIE_DIR "/")
779956

780957
enum fsmonitor_path_type fsmonitor_classify_path_workdir_relative(
781958
const char *rel)
@@ -928,6 +1105,9 @@ void fsmonitor_publish(struct fsmonitor_daemon_state *state,
9281105
}
9291106
}
9301107

1108+
if (cookie_names->nr)
1109+
with_lock__mark_cookies_seen(state, cookie_names);
1110+
9311111
pthread_mutex_unlock(&state->main_lock);
9321112
}
9331113

@@ -1019,7 +1199,9 @@ static int fsmonitor_run_daemon(void)
10191199

10201200
memset(&state, 0, sizeof(state));
10211201

1202+
hashmap_init(&state.cookies, cookies_cmp, NULL, 0);
10221203
pthread_mutex_init(&state.main_lock, NULL);
1204+
pthread_cond_init(&state.cookies_cond, NULL);
10231205
state.error_code = 0;
10241206
state.current_token_data = fsmonitor_new_token_data();
10251207

@@ -1044,6 +1226,44 @@ static int fsmonitor_run_daemon(void)
10441226
state.nr_paths_watching = 2;
10451227
}
10461228

1229+
/*
1230+
* We will write filesystem syncing cookie files into
1231+
* <gitdir>/<fsmonitor-dir>/<cookie-dir>/<pid>-<seq>.
1232+
*
1233+
* The extra layers of subdirectories here keep us from
1234+
* changing the mtime on ".git/" or ".git/foo/" when we create
1235+
* or delete cookie files.
1236+
*
1237+
* There have been problems with some IDEs that do a
1238+
* non-recursive watch of the ".git/" directory and run a
1239+
* series of commands any time something happens.
1240+
*
1241+
* For example, if we place our cookie files directly in
1242+
* ".git/" or ".git/foo/" then a `git status` (or similar
1243+
* command) from the IDE will cause a cookie file to be
1244+
* created in one of those dirs. This causes the mtime of
1245+
* those dirs to change. This triggers the IDE's watch
1246+
* notification. This triggers the IDE to run those commands
1247+
* again. And the process repeats and the machine never goes
1248+
* idle.
1249+
*
1250+
* Adding the extra layers of subdirectories prevents the
1251+
* mtime of ".git/" and ".git/foo" from changing when a
1252+
* cookie file is created.
1253+
*/
1254+
strbuf_init(&state.path_cookie_prefix, 0);
1255+
strbuf_addbuf(&state.path_cookie_prefix, &state.path_gitdir_watch);
1256+
1257+
strbuf_addch(&state.path_cookie_prefix, '/');
1258+
strbuf_addstr(&state.path_cookie_prefix, FSMONITOR_DIR);
1259+
mkdir(state.path_cookie_prefix.buf, 0777);
1260+
1261+
strbuf_addch(&state.path_cookie_prefix, '/');
1262+
strbuf_addstr(&state.path_cookie_prefix, FSMONITOR_COOKIE_DIR);
1263+
mkdir(state.path_cookie_prefix.buf, 0777);
1264+
1265+
strbuf_addch(&state.path_cookie_prefix, '/');
1266+
10471267
/*
10481268
* Confirm that we can create platform-specific resources for the
10491269
* filesystem listener before we bother starting all the threads.
@@ -1056,13 +1276,19 @@ static int fsmonitor_run_daemon(void)
10561276
err = fsmonitor_run_daemon_1(&state);
10571277

10581278
done:
1279+
pthread_cond_destroy(&state.cookies_cond);
10591280
pthread_mutex_destroy(&state.main_lock);
10601281
fsm_listen__dtor(&state);
10611282

10621283
ipc_server_free(state.ipc_server_data);
10631284

10641285
strbuf_release(&state.path_worktree_watch);
10651286
strbuf_release(&state.path_gitdir_watch);
1287+
strbuf_release(&state.path_cookie_prefix);
1288+
1289+
/*
1290+
* NEEDSWORK: Consider "rm -rf <gitdir>/<fsmonitor-dir>"
1291+
*/
10661292

10671293
return err;
10681294
}

fsmonitor--daemon.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,11 @@ struct fsmonitor_daemon_state {
4545

4646
struct fsmonitor_token_data *current_token_data;
4747

48+
struct strbuf path_cookie_prefix;
49+
pthread_cond_t cookies_cond;
50+
int cookie_seq;
51+
struct hashmap cookies;
52+
4853
int error_code;
4954
struct fsmonitor_daemon_backend_data *backend_data;
5055

0 commit comments

Comments
 (0)