{"thread":{"id":"62235","subject":"[PATCH] fsmonitor: fix hangs by delayed fs event listening","startedAt":"2024-10-02T09:47:08Z","lastAt":"2024-10-11T18:01:14Z","messageCount":10,"participants":["Koji Nakamaru via GitGitGadget","Jeff King","Koji Nakamaru","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"503895","messageId":"pull.1804.git.1727862424713.gitgitgadget@gmail.com","threadId":"62235","inReplyTo":null,"subject":"[PATCH] fsmonitor: fix hangs by delayed fs event listening","fromName":"Koji Nakamaru via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-02T09:47:04Z","receivedAt":"2024-10-02T09:47:08Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"From: Koji Nakamaru <koji.nakamaru@gree.net>\n\nThe thread serving the client (ipc-thread) calls\nwith_lock__wait_for_cookie() in which a cookie file is\ncreated. with_lock__wait_for_cookie() then waits for the event caused by\nthe cookie file from the thread for fs events (fsevent-thread).\n\nHowever, in high load situations, the fsevent-thread may start actual fs\nevent listening (triggered by FSEventStreamStart() for Darwin, for\nexample) *after* the cookie file is created. In this case, the\nfsevent-thread cannot detect the cookie file and\nwith_lock__wait_for_cookie() waits forever, so that the whole daemon\nhangs [1].\n\nExtend listen_error_code to express that actual fs event listening\nstarts. listen_error_code is accessed in a thread-safe manner by\nutilizing a dedicated mutex.\n\n[1]: https://lore.kernel.org/git/20241002062539.GA2863841@coredump.intra.peff.net/\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Koji Nakamaru <koji.nakamaru@gree.net>\n---\n    fsmonitor: fix hangs by delayed fs event listening\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1804%2FKojiNakamaru%2Ffix%2Ffsmonitor-deadlock-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1804/KojiNakamaru/fix/fsmonitor-deadlock-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1804\n\n builtin/fsmonitor--daemon.c          | 21 +++++++++++++++++++++\n compat/fsmonitor/fsm-listen-darwin.c |  5 +++--\n compat/fsmonitor/fsm-listen-win32.c  |  5 ++---\n fsmonitor--daemon.h                  |  4 ++++\n 4 files changed, 30 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex dce8a3b2482..ccff2cb8bed 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -172,6 +172,9 @@ static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(\n \ttrace_printf_key(&trace_fsmonitor, \"cookie-wait: '%s' '%s'\",\n \t\t\t cookie->name, cookie_pathname.buf);\n \n+\twhile (fsmonitor_get_listen_error_code(state) == 0)\n+\t\tsleep_millisec(50);\n+\n \t/*\n \t * Create the cookie file on disk and then wait for a notification\n \t * that the listener thread has seen it.\n@@ -606,6 +609,23 @@ void fsmonitor_force_resync(struct fsmonitor_daemon_state *state)\n \tpthread_mutex_unlock(&state->main_lock);\n }\n \n+int fsmonitor_get_listen_error_code(struct fsmonitor_daemon_state *state)\n+{\n+\tint error_code;\n+\n+\tpthread_mutex_lock(&state->listen_lock);\n+\terror_code = state->listen_error_code;\n+\tpthread_mutex_unlock(&state->listen_lock);\n+\treturn error_code;\n+}\n+\n+void fsmonitor_set_listen_error_code(struct fsmonitor_daemon_state *state, int error_code)\n+{\n+\tpthread_mutex_lock(&state->listen_lock);\n+\tstate->listen_error_code = error_code;\n+\tpthread_mutex_unlock(&state->listen_lock);\n+}\n+\n /*\n  * Format an opaque token string to send to the client.\n  */\n@@ -1285,6 +1305,7 @@ static int fsmonitor_run_daemon(void)\n \thashmap_init(&state.cookies, cookies_cmp, NULL, 0);\n \tpthread_mutex_init(&state.main_lock, NULL);\n \tpthread_cond_init(&state.cookies_cond, NULL);\n+\tpthread_mutex_init(&state.listen_lock, NULL);\n \tstate.listen_error_code = 0;\n \tstate.health_error_code = 0;\n \tstate.current_token_data = fsmonitor_new_token_data();\ndiff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c\nindex 2fc67442eb5..92373ce247f 100644\n--- a/compat/fsmonitor/fsm-listen-darwin.c\n+++ b/compat/fsmonitor/fsm-listen-darwin.c\n@@ -515,6 +515,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \t\tgoto force_error_stop_without_loop;\n \t}\n \tdata->stream_started = 1;\n+\tfsmonitor_set_listen_error_code(state, 1);\n \n \tpthread_mutex_lock(&data->dq_lock);\n \tpthread_cond_wait(&data->dq_finished, &data->dq_lock);\n@@ -522,7 +523,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \n \tswitch (data->shutdown_style) {\n \tcase FORCE_ERROR_STOP:\n-\t\tstate->listen_error_code = -1;\n+\t\tfsmonitor_set_listen_error_code(state, -1);\n \t\t/* fall thru */\n \tcase FORCE_SHUTDOWN:\n \t\tipc_server_stop_async(state->ipc_server_data);\n@@ -534,7 +535,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \treturn;\n \n force_error_stop_without_loop:\n-\tstate->listen_error_code = -1;\n+\tfsmonitor_set_listen_error_code(state, -1);\n \tipc_server_stop_async(state->ipc_server_data);\n \treturn;\n }\ndiff --git a/compat/fsmonitor/fsm-listen-win32.c b/compat/fsmonitor/fsm-listen-win32.c\nindex 5a21dade7b8..609efd1d6a8 100644\n--- a/compat/fsmonitor/fsm-listen-win32.c\n+++ b/compat/fsmonitor/fsm-listen-win32.c\n@@ -732,14 +732,13 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \tDWORD dwWait;\n \tint result;\n \n-\tstate->listen_error_code = 0;\n-\n \tif (start_rdcw_watch(data->watch_worktree) == -1)\n \t\tgoto force_error_stop;\n \n \tif (data->watch_gitdir &&\n \t    start_rdcw_watch(data->watch_gitdir) == -1)\n \t\tgoto force_error_stop;\n+\tfsmonitor_set_listen_error_code(state, 1);\n \n \tfor (;;) {\n \t\tdwWait = WaitForMultipleObjects(data->nr_listener_handles,\n@@ -797,7 +796,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \t}\n \n force_error_stop:\n-\tstate->listen_error_code = -1;\n+\tfsmonitor_set_listen_error_code(state, -1);\n \n force_shutdown:\n \t/*\ndiff --git a/fsmonitor--daemon.h b/fsmonitor--daemon.h\nindex 5cbbec8d940..77efc59b7bb 100644\n--- a/fsmonitor--daemon.h\n+++ b/fsmonitor--daemon.h\n@@ -51,6 +51,7 @@ struct fsmonitor_daemon_state {\n \tint cookie_seq;\n \tstruct hashmap cookies;\n \n+\tpthread_mutex_t listen_lock;\n \tint listen_error_code;\n \tint health_error_code;\n \tstruct fsm_listen_data *listen_data;\n@@ -167,5 +168,8 @@ void fsmonitor_publish(struct fsmonitor_daemon_state *state,\n  */\n void fsmonitor_force_resync(struct fsmonitor_daemon_state *state);\n \n+int fsmonitor_get_listen_error_code(struct fsmonitor_daemon_state *state);\n+void fsmonitor_set_listen_error_code(struct fsmonitor_daemon_state *state, int error_code);\n+\n #endif /* HAVE_FSMONITOR_DAEMON_BACKEND */\n #endif /* FSMONITOR_DAEMON_H */\n\nbase-commit: e9356ba3ea2a6754281ff7697b3e5a1697b21e24\n-- \ngitgitgadget\n"},{"id":"504250","messageId":"20241007055821.GA34037@coredump.intra.peff.net","threadId":"62235","inReplyTo":"pull.1804.git.1727862424713.gitgitgadget@gmail.com","subject":"Re: [PATCH] fsmonitor: fix hangs by delayed fs event listening","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-07T05:58:21Z","receivedAt":"2024-10-07T05:58:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 02, 2024 at 09:47:04AM +0000, Koji Nakamaru via GitGitGadget wrote:\n\n> From: Koji Nakamaru <koji.nakamaru@gree.net>\n> \n> The thread serving the client (ipc-thread) calls\n> with_lock__wait_for_cookie() in which a cookie file is\n> created. with_lock__wait_for_cookie() then waits for the event caused by\n> the cookie file from the thread for fs events (fsevent-thread).\n> \n> However, in high load situations, the fsevent-thread may start actual fs\n> event listening (triggered by FSEventStreamStart() for Darwin, for\n> example) *after* the cookie file is created. In this case, the\n> fsevent-thread cannot detect the cookie file and\n> with_lock__wait_for_cookie() waits forever, so that the whole daemon\n> hangs [1].\n\nFirst off, thank you for looking into this. I _think_ what you have here\nwould work, but I had envisioned something a little different. So let me\nfirst try to walk through your solution...\n\n> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\n> index dce8a3b2482..ccff2cb8bed 100644\n> --- a/builtin/fsmonitor--daemon.c\n> +++ b/builtin/fsmonitor--daemon.c\n> @@ -172,6 +172,9 @@ static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(\n>  \ttrace_printf_key(&trace_fsmonitor, \"cookie-wait: '%s' '%s'\",\n>  \t\t\t cookie->name, cookie_pathname.buf);\n>  \n> +\twhile (fsmonitor_get_listen_error_code(state) == 0)\n> +\t\tsleep_millisec(50);\n> +\n>  \t/*\n>  \t * Create the cookie file on disk and then wait for a notification\n>  \t * that the listener thread has seen it.\n\nOK, so here we're going to basically busy-wait for the error code value\nto become non-zero.\n\nThat happens in a thread-safe way inside our helper function, and the\nmatching thread-safe set() is called here:\n\n> diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c\n> index 2fc67442eb5..92373ce247f 100644\n> --- a/compat/fsmonitor/fsm-listen-darwin.c\n> +++ b/compat/fsmonitor/fsm-listen-darwin.c\n> @@ -515,6 +515,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n>  \t\tgoto force_error_stop_without_loop;\n>  \t}\n>  \tdata->stream_started = 1;\n> +\tfsmonitor_set_listen_error_code(state, 1);\n>  \n>  \tpthread_mutex_lock(&data->dq_lock);\n>  \tpthread_cond_wait(&data->dq_finished, &data->dq_lock);\n\nwhich then releases all of the waiting clients to start working.\n\nThey'd similarly be released on errors such as this one:\n\n> @@ -522,7 +523,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n>  \n>  \tswitch (data->shutdown_style) {\n>  \tcase FORCE_ERROR_STOP:\n> -\t\tstate->listen_error_code = -1;\n> +\t\tfsmonitor_set_listen_error_code(state, -1);\n>  \t\t/* fall thru */\n>  \tcase FORCE_SHUTDOWN:\n>  \t\tipc_server_stop_async(state->ipc_server_data);\n\nThere's some risk that we'd have a code path fails to set the listen\ncode, in which case our clients might spin forever. But from a cursory\nlook, I think you've got all spots.\n\nI think your patch has one small bug, which is that you don't\npthread_mutex_destroy() at the end of fsmonitor_run_daemon(). But other\nthan that I think it should work OK (I haven't tried it in practice yet;\nI assume you did?).\n\nMy two small-ish complaints are:\n\n  - busy-waiting feels a bit hacky. I think it's not too bad here\n    because it only happens during startup (and even then only if we get\n    a request) because after that the listen code will already be set to\n    \"1\".\n\n  - the check for the listen code is in the wait_for_cookie() function.\n    So even after we've started up, we'll still keep taking a lock to\n    check it for every such request. I guess it probably isn't much\n    overhead in practice, though.\n\nBut what I had envisioned instead was teaching the ipc_server code to\nlet us control when it starts actually running. We can do that by\nholding the existing work lock, and letting the callers (in this case,\nthe individual fsm backends) tell it when to start operating.\n\nAnd then we are not introducing a new lock, but rather relying on the\nexisting lock-checks for getting work to do. So there's no busy-wait.\nAnd after the startup sequence finishes, there are no additional lock\nchecks.\n\nThe patch below implements that. The only complication is that we have\nto keep a \"started\" flag to avoid double-locking during an early\nshutdown. Access to the \"started\" flag is not thread-safe, but I think\nthat's OK. Until we've started the clients, only the main thread would\ndo either a start or shutdown operation.\n\nSo I dunno. Both solutions have their own little warts, I suppose, and\nI'm not sure if one is vastly better than the other.\n\n---\ndiff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c\nindex 2fc67442eb..17d0b426e3 100644\n--- a/compat/fsmonitor/fsm-listen-darwin.c\n+++ b/compat/fsmonitor/fsm-listen-darwin.c\n@@ -516,6 +516,12 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \t}\n \tdata->stream_started = 1;\n \n+\t/*\n+\t * Our fs event listener is now running, so it's safe to start\n+\t * serving client requests.\n+\t */\n+\tipc_server_start(state->ipc_server_data);\n+\n \tpthread_mutex_lock(&data->dq_lock);\n \tpthread_cond_wait(&data->dq_finished, &data->dq_lock);\n \tpthread_mutex_unlock(&data->dq_lock);\ndiff --git a/compat/fsmonitor/fsm-listen-win32.c b/compat/fsmonitor/fsm-listen-win32.c\nindex 5a21dade7b..d9a02bc989 100644\n--- a/compat/fsmonitor/fsm-listen-win32.c\n+++ b/compat/fsmonitor/fsm-listen-win32.c\n@@ -741,6 +741,8 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \t    start_rdcw_watch(data->watch_gitdir) == -1)\n \t\tgoto force_error_stop;\n \n+\tipc_server_start(state->ipc_server_data);\n+\n \tfor (;;) {\n \t\tdwWait = WaitForMultipleObjects(data->nr_listener_handles,\n \t\t\t\t\t\tdata->hListener,\ndiff --git a/compat/simple-ipc/ipc-shared.c b/compat/simple-ipc/ipc-shared.c\nindex cb176d966f..603b60403b 100644\n--- a/compat/simple-ipc/ipc-shared.c\n+++ b/compat/simple-ipc/ipc-shared.c\n@@ -21,6 +21,7 @@ int ipc_server_run(const char *path, const struct ipc_server_opts *opts,\n \tif (ret)\n \t\treturn ret;\n \n+\tipc_server_start(server_data);\n \tret = ipc_server_await(server_data);\n \n \tipc_server_free(server_data);\ndiff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c\nindex 9b3f2cdf8c..9a2e93cff5 100644\n--- a/compat/simple-ipc/ipc-unix-socket.c\n+++ b/compat/simple-ipc/ipc-unix-socket.c\n@@ -328,6 +328,7 @@ struct ipc_server_data {\n \tint back_pos;\n \tint front_pos;\n \n+\tint started;\n \tint shutdown_requested;\n \tint is_stopped;\n };\n@@ -888,6 +889,12 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \tserver_data->accept_thread->fd_send_shutdown = sv[0];\n \tserver_data->accept_thread->fd_wait_shutdown = sv[1];\n \n+\t/*\n+\t * Hold work-available mutex so that no work can start until\n+\t * we unlock it.\n+\t */\n+\tpthread_mutex_lock(&server_data->work_available_mutex);\n+\n \tif (pthread_create(&server_data->accept_thread->pthread_id, NULL,\n \t\t\t   accept_thread_proc, server_data->accept_thread))\n \t\tdie_errno(_(\"could not start accept_thread '%s'\"), path);\n@@ -918,6 +925,15 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \treturn 0;\n }\n \n+void ipc_server_start(struct ipc_server_data *server_data)\n+{\n+\tif (!server_data || server_data->started)\n+\t\treturn;\n+\n+\tserver_data->started = 1;\n+\tpthread_mutex_unlock(&server_data->work_available_mutex);\n+}\n+\n /*\n  * Gently tell the IPC server treads to shutdown.\n  * Can be run on any thread.\n@@ -933,7 +949,9 @@ int ipc_server_stop_async(struct ipc_server_data *server_data)\n \n \ttrace2_region_enter(\"ipc-server\", \"server-stop-async\", NULL);\n \n-\tpthread_mutex_lock(&server_data->work_available_mutex);\n+\t/* If we haven't started yet, we are already holding lock. */\n+\tif (!server_data->started)\n+\t\tpthread_mutex_lock(&server_data->work_available_mutex);\n \n \tserver_data->shutdown_requested = 1;\n \ndiff --git a/simple-ipc.h b/simple-ipc.h\nindex a849d9f841..764bf7a309 100644\n--- a/simple-ipc.h\n+++ b/simple-ipc.h\n@@ -179,12 +179,21 @@ struct ipc_server_opts\n  * When a client IPC message is received, the `application_cb` will be\n  * called (possibly on a random thread) to handle the message and\n  * optionally compose a reply message.\n+ *\n+ * This initializes all threads but no actual work will be done until\n+ * ipc_server_start() is called.\n  */\n int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \t\t\t const char *path, const struct ipc_server_opts *opts,\n \t\t\t ipc_server_application_cb *application_cb,\n \t\t\t void *application_data);\n \n+/*\n+ * Let an async server start running. This needs to be called only once\n+ * after initialization.\n+ */\n+void ipc_server_start(struct ipc_server_data *server_data);\n+\n /*\n  * Gently signal the IPC server pool to shutdown.  No new client\n  * connections will be accepted, but existing connections will be\n\n"},{"id":"504253","messageId":"20241007060813.GA34827@coredump.intra.peff.net","threadId":"62235","inReplyTo":"20241007055821.GA34037@coredump.intra.peff.net","subject":"Re: [PATCH] fsmonitor: fix hangs by delayed fs event listening","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-07T06:08:13Z","receivedAt":"2024-10-07T06:08:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 07, 2024 at 01:58:21AM -0400, Jeff King wrote:\n\n> I think your patch has one small bug, which is that you don't\n> pthread_mutex_destroy() at the end of fsmonitor_run_daemon(). But other\n> than that I think it should work OK (I haven't tried it in practice yet;\n> I assume you did?).\n\nI just checked your patch in our CI, using the sleep(1) you suggested\nearlier to more predictably lose the race(). It does work reliably (and\nI confirmed with some extra trace statements that it does spin on the\nsleep_millisec() loop).\n\nI had also previously checked my suggested solution. So I do think\neither is a valid solution to the problem.\n\n-Peff\n"},{"id":"504288","messageId":"CAOTNsDwwikiX3u6DG=+4hn+mcgfXzzDoqR3ZFVEdGi=mPGQbpg@mail.gmail.com","threadId":"62235","inReplyTo":"20241007060813.GA34827@coredump.intra.peff.net","subject":"Re: [PATCH] fsmonitor: fix hangs by delayed fs event listening","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-07T09:45:22Z","receivedAt":"2024-10-07T09:45:34Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Mon, Oct 07, 2024 at 01:58:21AM -0400, Jeff King wrote:\n> First off, thank you for looking into this. I _think_ what you have here\n> would work, but I had envisioned something a little different. So let me\n> first try to walk through your solution...\n\nThank you very much for looking through my patch in detail and providing\nanother approach. I agree that busy-waiting is not smart ;) I utilized\nit to minimize code modification and not to worry about any new\ndeadlock. Your approach is more natural if the code is written from\nscratch with the problem in mind.\n\n> @@ -933,7 +949,9 @@ int ipc_server_stop_async(struct ipc_server_data *server_data)\n>\n>       trace2_region_enter(\"ipc-server\", \"server-stop-async\", NULL);\n>\n> -     pthread_mutex_lock(&server_data->work_available_mutex);\n> +     /* If we haven't started yet, we are already holding lock. */\n> +     if (!server_data->started)\n> +             pthread_mutex_lock(&server_data->work_available_mutex);\n>\n>       server_data->shutdown_requested = 1;\n\nIs this condition inverted?\n\nOn Mon, Oct 7, 2024 at 3:08 PM Jeff King <peff@peff.net> wrote:\n> I just checked your patch in our CI, using the sleep(1) you suggested\n> earlier to more predictably lose the race(). It does work reliably (and\n> I confirmed with some extra trace statements that it does spin on the\n> sleep_millisec() loop).\n>\n> I had also previously checked my suggested solution. So I do think\n> either is a valid solution to the problem.\n\nI also tested your approach on Windows with a few additions to\nipc-win32.c, and it worked correctly.\n\n* define work_available_mutex and started in ipc_server_data.\n\n* call pthread_mutex_lock(&server_data->work_available_mutex) before\ncreating the server_thread_proc thread.\n\n* define ipc_server_start()\n\nShall we adopt your approach as it is more natural. Can I ask you to\nsubmit a new patch?\n\nKoji Nakamaru\n"},{"id":"504402","messageId":"20241008083121.GA676391@coredump.intra.peff.net","threadId":"62235","inReplyTo":"CAOTNsDwwikiX3u6DG=+4hn+mcgfXzzDoqR3ZFVEdGi=mPGQbpg@mail.gmail.com","subject":"[PATCH 0/2] alternate approach to fixing fsmonitor hangs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-08T08:31:21Z","receivedAt":"2024-10-08T08:31:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 07, 2024 at 06:45:22PM +0900, Koji Nakamaru wrote:\n\n> > -     pthread_mutex_lock(&server_data->work_available_mutex);\n> > +     /* If we haven't started yet, we are already holding lock. */\n> > +     if (!server_data->started)\n> > +             pthread_mutex_lock(&server_data->work_available_mutex);\n> >\n> >       server_data->shutdown_requested = 1;\n> \n> Is this condition inverted?\n\nYes, good catch.\n\n> > I had also previously checked my suggested solution. So I do think\n> > either is a valid solution to the problem.\n> \n> I also tested your approach on Windows with a few additions to\n> ipc-win32.c, and it worked correctly.\n\nYeah, I never even tried building mine on Windows, and was just testing\nvia tmate on our macOS CI environment. :-/\n\nI've added in the necessary Windows bits, along with smoothing a few\nrough edges. Especially with the extra Windows changes (which I mostly\nhad to guess-and-check by pushing to CI), I'm beginning to wonder if my\nsolution isn't getting a bit too complicated, and maybe yours was the\nright way after all.\n\nBut I've cleaned it up for presentation here, so at least we can look at\nthe final form of both and see which we prefer.\n\n  [1/2]: simple-ipc: split async server initialization and running\n  [2/2]: fsmonitor: initialize fs event listener before accepting clients\n\n builtin/fsmonitor--daemon.c          |  6 ++--\n compat/fsmonitor/fsm-listen-darwin.c |  6 ++++\n compat/fsmonitor/fsm-listen-win32.c  |  6 ++++\n compat/simple-ipc/ipc-shared.c       |  5 +--\n compat/simple-ipc/ipc-unix-socket.c  | 28 +++++++++++++---\n compat/simple-ipc/ipc-win32.c        | 48 +++++++++++++++++++++++++---\n simple-ipc.h                         | 17 +++++++---\n 7 files changed, 98 insertions(+), 18 deletions(-)\n\n-Peff\n"},{"id":"504403","messageId":"20241008083347.GA1210958@coredump.intra.peff.net","threadId":"62235","inReplyTo":"20241008083121.GA676391@coredump.intra.peff.net","subject":"[PATCH 1/2] simple-ipc: split async server initialization and running","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-08T08:33:47Z","receivedAt":"2024-10-08T08:33:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To start an async ipc server, you call ipc_server_run_async(). That\ninitializes the ipc_server_data object, and starts all of the threads\nrunning, which may immediately start serving clients.\n\nThis can create some awkward timing problems, though. In the fsmonitor\ndaemon (the sole user of the simple-ipc system), we want to create the\nipc server early in the process, which means we may start serving\nclients before the rest of the daemon is fully initialized.\n\nTo solve this, let's break run_async() into two parts: an initialization\nwhich allocates all data and spawns the threads (without letting them\nrun), and a start function which actually lets them begin work. Since we\nhave two simple-ipc implementations, we have to handle this twice:\n\n  - in ipc-unix-socket.c, we have a central listener thread which hands\n    connections off to worker threads using a work_available mutex. We\n    can hold that mutex after init, and release it when we're ready to\n    start.\n\n    We do need an extra \"started\" flag so that we know whether the main\n    thread is holding the mutex or not (e.g., if we prematurely stop the\n    server, we want to make sure all of the worker threads are released\n    to hear about the shutdown).\n\n  - in ipc-win32.c, we don't have a central mutex. So we'll introduce a\n    new startup_barrier mutex, which we'll similarly hold until we're\n    ready to let the threads proceed.\n\n    We again need a \"started\" flag here to make sure that we release the\n    barrier mutex when shutting down, so that the sub-threads can\n    proceed to the finish.\n\nI've renamed the run_async() function to init_async() to make sure we\ncatch all callers, since they'll now need to call the matching\nstart_async().\n\nWe could leave run_async() as a wrapper that does both, but there's not\nmuch point. There are only two callers, one of which is fsmonitor, which\nwill want to actually do work between the two calls. And the other is\njust a test-tool wrapper.\n\nFor now I've added the start_async() calls in fsmonitor where they would\notherwise have happened, so there should be no behavior change with this\npatch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsmonitor--daemon.c         |  8 +++--\n compat/simple-ipc/ipc-shared.c      |  5 +--\n compat/simple-ipc/ipc-unix-socket.c | 28 ++++++++++++++---\n compat/simple-ipc/ipc-win32.c       | 48 ++++++++++++++++++++++++++---\n simple-ipc.h                        | 17 +++++++---\n 5 files changed, 88 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex e1e6b96d09..4a077810d0 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1208,13 +1208,15 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)\n \t * system event listener thread so that we have the IPC handle\n \t * before we need it.\n \t */\n-\tif (ipc_server_run_async(&state->ipc_server_data,\n-\t\t\t\t state->path_ipc.buf, &ipc_opts,\n-\t\t\t\t handle_client, state))\n+\tif (ipc_server_init_async(&state->ipc_server_data,\n+\t\t\t\t  state->path_ipc.buf, &ipc_opts,\n+\t\t\t\t  handle_client, state))\n \t\treturn error_errno(\n \t\t\t_(\"could not start IPC thread pool on '%s'\"),\n \t\t\tstate->path_ipc.buf);\n \n+\tipc_server_start_async(&state->ipc_server_data);\n+\n \t/*\n \t * Start the fsmonitor listener thread to collect filesystem\n \t * events.\ndiff --git a/compat/simple-ipc/ipc-shared.c b/compat/simple-ipc/ipc-shared.c\nindex cb176d966f..d1c21b49bd 100644\n--- a/compat/simple-ipc/ipc-shared.c\n+++ b/compat/simple-ipc/ipc-shared.c\n@@ -16,11 +16,12 @@ int ipc_server_run(const char *path, const struct ipc_server_opts *opts,\n \tstruct ipc_server_data *server_data = NULL;\n \tint ret;\n \n-\tret = ipc_server_run_async(&server_data, path, opts,\n-\t\t\t\t   application_cb, application_data);\n+\tret = ipc_server_init_async(&server_data, path, opts,\n+\t\t\t\t    application_cb, application_data);\n \tif (ret)\n \t\treturn ret;\n \n+\tipc_server_start_async(server_data);\n \tret = ipc_server_await(server_data);\n \n \tipc_server_free(server_data);\ndiff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c\nindex 9b3f2cdf8c..57d919c6b4 100644\n--- a/compat/simple-ipc/ipc-unix-socket.c\n+++ b/compat/simple-ipc/ipc-unix-socket.c\n@@ -328,6 +328,7 @@ struct ipc_server_data {\n \tint back_pos;\n \tint front_pos;\n \n+\tint started;\n \tint shutdown_requested;\n \tint is_stopped;\n };\n@@ -824,10 +825,10 @@ static int setup_listener_socket(\n /*\n  * Start IPC server in a pool of background threads.\n  */\n-int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n-\t\t\t const char *path, const struct ipc_server_opts *opts,\n-\t\t\t ipc_server_application_cb *application_cb,\n-\t\t\t void *application_data)\n+int ipc_server_init_async(struct ipc_server_data **returned_server_data,\n+\t\t\t  const char *path, const struct ipc_server_opts *opts,\n+\t\t\t  ipc_server_application_cb *application_cb,\n+\t\t\t  void *application_data)\n {\n \tstruct unix_ss_socket *server_socket = NULL;\n \tstruct ipc_server_data *server_data;\n@@ -888,6 +889,12 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \tserver_data->accept_thread->fd_send_shutdown = sv[0];\n \tserver_data->accept_thread->fd_wait_shutdown = sv[1];\n \n+\t/*\n+\t * Hold work-available mutex so that no work can start until\n+\t * we unlock it.\n+\t */\n+\tpthread_mutex_lock(&server_data->work_available_mutex);\n+\n \tif (pthread_create(&server_data->accept_thread->pthread_id, NULL,\n \t\t\t   accept_thread_proc, server_data->accept_thread))\n \t\tdie_errno(_(\"could not start accept_thread '%s'\"), path);\n@@ -918,6 +925,15 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \treturn 0;\n }\n \n+void ipc_server_start_async(struct ipc_server_data *server_data)\n+{\n+\tif (!server_data || server_data->started)\n+\t\treturn;\n+\n+\tserver_data->started = 1;\n+\tpthread_mutex_unlock(&server_data->work_available_mutex);\n+}\n+\n /*\n  * Gently tell the IPC server treads to shutdown.\n  * Can be run on any thread.\n@@ -933,7 +949,9 @@ int ipc_server_stop_async(struct ipc_server_data *server_data)\n \n \ttrace2_region_enter(\"ipc-server\", \"server-stop-async\", NULL);\n \n-\tpthread_mutex_lock(&server_data->work_available_mutex);\n+\t/* If we haven't started yet, we are already holding lock. */\n+\tif (server_data->started)\n+\t\tpthread_mutex_lock(&server_data->work_available_mutex);\n \n \tserver_data->shutdown_requested = 1;\n \ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 8bfe51248e..a8fc812adf 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -371,6 +371,9 @@ struct ipc_server_data {\n \tHANDLE hEventStopRequested;\n \tstruct ipc_server_thread_data *thread_list;\n \tint is_stopped;\n+\n+\tpthread_mutex_t startup_barrier;\n+\tint started;\n };\n \n enum connect_result {\n@@ -526,6 +529,16 @@ static int use_connection(struct ipc_server_thread_data *server_thread_data)\n \treturn ret;\n }\n \n+static void wait_for_startup_barrier(struct ipc_server_data *server_data)\n+{\n+\t/*\n+\t * Temporarily hold the startup_barrier mutex before starting,\n+\t * which lets us know that it's OK to start serving requests.\n+\t */\n+\tpthread_mutex_lock(&server_data->startup_barrier);\n+\tpthread_mutex_unlock(&server_data->startup_barrier);\n+}\n+\n /*\n  * Thread proc for an IPC server worker thread.  It handles a series of\n  * connections from clients.  It cleans and reuses the hPipe between each\n@@ -550,6 +563,8 @@ static void *server_thread_proc(void *_server_thread_data)\n \tmemset(&oConnect, 0, sizeof(oConnect));\n \toConnect.hEvent = hEventConnected;\n \n+\twait_for_startup_barrier(server_thread_data->server_data);\n+\n \tfor (;;) {\n \t\tcr = wait_for_connection(server_thread_data, &oConnect);\n \n@@ -752,10 +767,10 @@ static HANDLE create_new_pipe(wchar_t *wpath, int is_first)\n \treturn hPipe;\n }\n \n-int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n-\t\t\t const char *path, const struct ipc_server_opts *opts,\n-\t\t\t ipc_server_application_cb *application_cb,\n-\t\t\t void *application_data)\n+int ipc_server_init_async(struct ipc_server_data **returned_server_data,\n+\t\t\t  const char *path, const struct ipc_server_opts *opts,\n+\t\t\t  ipc_server_application_cb *application_cb,\n+\t\t\t  void *application_data)\n {\n \tstruct ipc_server_data *server_data;\n \twchar_t wpath[MAX_PATH];\n@@ -787,6 +802,13 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \tstrbuf_addstr(&server_data->buf_path, path);\n \twcscpy(server_data->wpath, wpath);\n \n+\t/*\n+\t * Hold the startup_barrier lock so that no threads will progress\n+\t * until ipc_server_start_async() is called.\n+\t */\n+\tpthread_mutex_init(&server_data->startup_barrier, NULL);\n+\tpthread_mutex_lock(&server_data->startup_barrier);\n+\n \tif (nr_threads < 1)\n \t\tnr_threads = 1;\n \n@@ -837,6 +859,15 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \treturn 0;\n }\n \n+void ipc_server_start_async(struct ipc_server_data *server_data)\n+{\n+\tif (!server_data || server_data->started)\n+\t\treturn;\n+\n+\tserver_data->started = 1;\n+\tpthread_mutex_unlock(&server_data->startup_barrier);\n+}\n+\n int ipc_server_stop_async(struct ipc_server_data *server_data)\n {\n \tif (!server_data)\n@@ -850,6 +881,13 @@ int ipc_server_stop_async(struct ipc_server_data *server_data)\n \t * We DO NOT attempt to force them to drop an active connection.\n \t */\n \tSetEvent(server_data->hEventStopRequested);\n+\n+\t/*\n+\t * If we haven't yet told the threads they are allowed to run,\n+\t * do so now, so they can receive the shutdown event.\n+\t */\n+\tipc_server_start_async(server_data);\n+\n \treturn 0;\n }\n \n@@ -900,5 +938,7 @@ void ipc_server_free(struct ipc_server_data *server_data)\n \t\tfree(std);\n \t}\n \n+\tpthread_mutex_destroy(&server_data->startup_barrier);\n+\n \tfree(server_data);\n }\ndiff --git a/simple-ipc.h b/simple-ipc.h\nindex a849d9f841..3916eaf70d 100644\n--- a/simple-ipc.h\n+++ b/simple-ipc.h\n@@ -179,11 +179,20 @@ struct ipc_server_opts\n  * When a client IPC message is received, the `application_cb` will be\n  * called (possibly on a random thread) to handle the message and\n  * optionally compose a reply message.\n+ *\n+ * This initializes all threads but no actual work will be done until\n+ * ipc_server_start_async() is called.\n+ */\n+int ipc_server_init_async(struct ipc_server_data **returned_server_data,\n+\t\t\t  const char *path, const struct ipc_server_opts *opts,\n+\t\t\t  ipc_server_application_cb *application_cb,\n+\t\t\t  void *application_data);\n+\n+/*\n+ * Let an async server start running. This needs to be called only once\n+ * after initialization.\n  */\n-int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n-\t\t\t const char *path, const struct ipc_server_opts *opts,\n-\t\t\t ipc_server_application_cb *application_cb,\n-\t\t\t void *application_data);\n+void ipc_server_start_async(struct ipc_server_data *server_data);\n \n /*\n  * Gently signal the IPC server pool to shutdown.  No new client\n-- \n2.47.0.427.g067ae60d0d\n\n"},{"id":"504404","messageId":"20241008083613.GB1210958@coredump.intra.peff.net","threadId":"62235","inReplyTo":"20241008083121.GA676391@coredump.intra.peff.net","subject":"[PATCH 2/2] fsmonitor: initialize fs event listener before accepting clients","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-08T08:36:13Z","receivedAt":"2024-10-08T08:36:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a racy hang in fsmonitor on macOS that we sometimes see in CI.\nWhen we serve a client, what's supposed to happen is:\n\n  1. The client thread calls with_lock__wait_for_cookie() in which we\n     create a cookie file and then wait for a pthread_cond event\n\n  2. The filesystem event listener sees the cookie file creation, does\n     some internal book-keeping, and then triggers the pthread_cond.\n\nBut there's a problem: we start the listener that accepts client threads\nbefore we start the fs event thread. So it's possible for us to accept a\nclient which creates the cookie file and starts waiting before the fs\nevent thread is initialized, and we miss those filesystem events\nentirely. That leaves the client thread hanging forever.\n\nIn CI, the symptom is that t9210 (which is testing scalar, which always\nenables fsmonitor under the hood) may hang forever in \"scalar clone\". It\nis waiting on \"git fetch\" which is waiting on the fsmonitor daemon.\n\nThe race happens more frequently under load, but you can trigger it\npredictably with a sleep like this, which delays the start of the fs\nevent thread:\n\n  --- a/compat/fsmonitor/fsm-listen-darwin.c\n  +++ b/compat/fsmonitor/fsm-listen-darwin.c\n  @@ -510,6 +510,7 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n          FSEventStreamSetDispatchQueue(data->stream, data->dq);\n          data->stream_scheduled = 1;\n\n  +       sleep(1);\n          if (!FSEventStreamStart(data->stream)) {\n                  error(_(\"Failed to start the FSEventStream\"));\n                  goto force_error_stop_without_loop;\n\nOne solution might be to reverse the order of initialization: start the\nfs event thread before we start the thread listening for clients. But\nthe fsmonitor code explicitly does it in the opposite direction. The fs\nevent thread wants to refer to the ipc_server_data struct, so we need it\nto be initialized first.\n\nA further complication is that we need a signal from the fs event thread\nthat it is actually ready and listening. And those details happen within\nbackend-specific fsmonitor code, whereas the initialization is in the\nshared code.\n\nSo instead, let's use the ipc_server init/start split added in the\nprevious commit. The generic fsmonitor code will init the ipc_server but\n_not_ start it, leaving that to the backend specific code, which now\nneeds to call ipc_server_start_async() at the right time.\n\nFor macOS, that is right after we start the FSEventStream that you can\nsee in the diff above.\n\nIt's not clear to me if Windows suffers from the same problem (and we\nsimply don't trigger it in CI), or if it is immune. Regardless, the\nobvious place to start accepting clients there is right after we've\nestablished the ReadDirectoryChanges watch.\n\nThis makes the hangs go away in our macOS CI environment, even when\ncompiled with the sleep() above.\n\nHelped-by: Koji Nakamaru <koji.nakamaru@gree.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsmonitor--daemon.c          | 2 --\n compat/fsmonitor/fsm-listen-darwin.c | 6 ++++++\n compat/fsmonitor/fsm-listen-win32.c  | 6 ++++++\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex 4a077810d0..f3f6bd330b 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1215,8 +1215,6 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)\n \t\t\t_(\"could not start IPC thread pool on '%s'\"),\n \t\t\tstate->path_ipc.buf);\n \n-\tipc_server_start_async(&state->ipc_server_data);\n-\n \t/*\n \t * Start the fsmonitor listener thread to collect filesystem\n \t * events.\ndiff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c\nindex 2fc67442eb..dfa551459d 100644\n--- a/compat/fsmonitor/fsm-listen-darwin.c\n+++ b/compat/fsmonitor/fsm-listen-darwin.c\n@@ -516,6 +516,12 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \t}\n \tdata->stream_started = 1;\n \n+\t/*\n+\t * Our fs event listener is now running, so it's safe to start\n+\t * serving client requests.\n+\t */\n+\tipc_server_start_async(state->ipc_server_data);\n+\n \tpthread_mutex_lock(&data->dq_lock);\n \tpthread_cond_wait(&data->dq_finished, &data->dq_lock);\n \tpthread_mutex_unlock(&data->dq_lock);\ndiff --git a/compat/fsmonitor/fsm-listen-win32.c b/compat/fsmonitor/fsm-listen-win32.c\nindex 5a21dade7b..80e092b511 100644\n--- a/compat/fsmonitor/fsm-listen-win32.c\n+++ b/compat/fsmonitor/fsm-listen-win32.c\n@@ -741,6 +741,12 @@ void fsm_listen__loop(struct fsmonitor_daemon_state *state)\n \t    start_rdcw_watch(data->watch_gitdir) == -1)\n \t\tgoto force_error_stop;\n \n+\t/*\n+\t * Now that we've established the rdcw watches, we can start\n+\t * serving clients.\n+\t */\n+\tipc_server_start_async(state->ipc_server_data);\n+\n \tfor (;;) {\n \t\tdwWait = WaitForMultipleObjects(data->nr_listener_handles,\n \t\t\t\t\t\tdata->hListener,\n-- \n2.47.0.427.g067ae60d0d\n"},{"id":"504453","messageId":"CAOTNsDyxmRZ155vV-Jh=1obMnR+F4ExY9B136fiGk0Vd23-zrw@mail.gmail.com","threadId":"62235","inReplyTo":"20241008083121.GA676391@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] alternate approach to fixing fsmonitor hangs","fromName":"Koji Nakamaru","fromEmail":"koji.nakamaru@gree.net","sentAt":"2024-10-08T16:03:14Z","receivedAt":"2024-10-08T16:03:26Z","isPatch":true,"sender":{"key":"koji.nakamaru@gree.net","avatar":"https://avatars.githubusercontent.com/u/2645978?v=4"},"body":"On Tue, Oct 8, 2024 at 5:31 PM Jeff King <peff@peff.net> wrote:\n> I've added in the necessary Windows bits, along with smoothing a few\n> rough edges. Especially with the extra Windows changes (which I mostly\n> had to guess-and-check by pushing to CI), I'm beginning to wonder if my\n> solution isn't getting a bit too complicated, and maybe yours was the\n> right way after all.\n>\n> But I've cleaned it up for presentation here, so at least we can look at\n> the final form of both and see which we prefer.\n\nThank you for the new patch. It prevents to start accepting requests\nuntil starting fs event listening and simplifies the code flow. It also\nhas sufficient comments, so later everyone can easily understand how it\nworks. I also tested it both on mac and windows and it works correctly.\n\nI think this one should be adopted :)\n\nKoji Nakamaru\n"},{"id":"504828","messageId":"20241011090051.GA563709@coredump.intra.peff.net","threadId":"62235","inReplyTo":"CAOTNsDyxmRZ155vV-Jh=1obMnR+F4ExY9B136fiGk0Vd23-zrw@mail.gmail.com","subject":"Re: [PATCH 0/2] alternate approach to fixing fsmonitor hangs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-11T09:00:51Z","receivedAt":"2024-10-11T09:00:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 09, 2024 at 01:03:14AM +0900, Koji Nakamaru wrote:\n\n> > But I've cleaned it up for presentation here, so at least we can look at\n> > the final form of both and see which we prefer.\n> \n> Thank you for the new patch. It prevents to start accepting requests\n> until starting fs event listening and simplifies the code flow. It also\n> has sufficient comments, so later everyone can easily understand how it\n> works. I also tested it both on mac and windows and it works correctly.\n> \n> I think this one should be adopted :)\n\nThanks for reviewing, and for all your work identifying the problem in\nthe first place! Looks like Junio has picked up my patch and it's\nalready in 'next', so hopefully these 6-hour CI timeouts will soon be a\nthing of the past. :)\n\n-Peff\n"},{"id":"504862","messageId":"xmqqiktyecyf.fsf@gitster.g","threadId":"62235","inReplyTo":"20241011090051.GA563709@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] alternate approach to fixing fsmonitor hangs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-11T18:01:12Z","receivedAt":"2024-10-11T18:01:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Oct 09, 2024 at 01:03:14AM +0900, Koji Nakamaru wrote:\n>\n>> > But I've cleaned it up for presentation here, so at least we can look at\n>> > the final form of both and see which we prefer.\n>> \n>> Thank you for the new patch. It prevents to start accepting requests\n>> until starting fs event listening and simplifies the code flow. It also\n>> has sufficient comments, so later everyone can easily understand how it\n>> works. I also tested it both on mac and windows and it works correctly.\n>> \n>> I think this one should be adopted :)\n>\n> Thanks for reviewing, and for all your work identifying the problem in\n> the first place! Looks like Junio has picked up my patch and it's\n> already in 'next', so hopefully these 6-hour CI timeouts will soon be a\n> thing of the past. :)\n\nYup.  Thanks, both of you.\n"}]}