Hi Jeff,
On Fri, 22 Apr 2022, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>
> Create another thread to watch over the daemon process and
> automatically shut it down if necessary.
>
> This commit creates the basic framework for a "health" thread
> to monitor the daemon and/or the file system. Later commits
> will add platform-specific code to do the actual work.
>
> The "health" thread is intended to monitor conditions that
> would be difficult to track inside the IPC thread pool and/or
> the file system listener threads. For example, when there are
> file system events outside of the watched worktree root or if
> we want to have an idle-timeout auto-shutdown feature.
>
> This commit creates the health thread itself, defines the thread-proc
> and sets up the thread's event loop. It integrates this new thread
> into the existing IPC and Listener thread models.
>
> This commit defines the API to the platform-specific code where all of
> the monitoring will actually happen.
>
> The platform-specific code for MacOS is just stubs. Meaning that the
> health thread will immediately exit on MacOS, but that is OK and
> expected. Future work can define MacOS-specific monitoring.
>
> The platform-specific code for Windows sets up enough of the
> WaitForMultipleObjects() machinery to watch for system and/or custom
> events. Currently, the set of wait handles only includes our custom
> shutdown event (sent from our other theads). Later commits in this
> series will extend the set of wait handles to monitor other
> conditions.
>
> Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
> ---
> Makefile | 6 ++-
> builtin/fsmonitor--daemon.c | 39 +++++++++++++++
> compat/fsmonitor/fsm-health-darwin.c | 24 ++++++++++
> compat/fsmonitor/fsm-health-win32.c | 72 ++++++++++++++++++++++++++++
> compat/fsmonitor/fsm-health.h | 47 ++++++++++++++++++
> contrib/buildsystems/CMakeLists.txt | 2 +
> fsmonitor--daemon.h | 4 ++
> 7 files changed, 192 insertions(+), 2 deletions(-)
> create mode 100644 compat/fsmonitor/fsm-health-darwin.c
> create mode 100644 compat/fsmonitor/fsm-health-win32.c
> create mode 100644 compat/fsmonitor/fsm-health.h
>
> diff --git a/Makefile b/Makefile
> index 93604fe8ef7..5f1623baadd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -472,8 +472,9 @@ all::
> #
> # If your platform supports a built-in fsmonitor backend, set
> # FSMONITOR_DAEMON_BACKEND to the "<name>" of the corresponding
> -# `compat/fsmonitor/fsm-listen-<name>.c` that implements the
> -# `fsm_listen__*()` routines.
> +# `compat/fsmonitor/fsm-listen-<name>.c` and
> +# `compat/fsmonitor/fsm-health-<name>.c` files
> +# that implement the `fsm_listen__*()` and `fsm_health__*()` routines.
> #
> # If your platform has OS-specific ways to tell if a repo is incompatible with
> # fsmonitor (whether the hook or IPC daemon version), set FSMONITOR_OS_SETTINGS
> @@ -1982,6 +1983,7 @@ endif
> ifdef FSMONITOR_DAEMON_BACKEND
> COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND
> COMPAT_OBJS += compat/fsmonitor/fsm-listen-$(FSMONITOR_DAEMON_BACKEND).o
> + COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_BACKEND).o
> endif
>
> ifdef FSMONITOR_OS_SETTINGS
> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> index b2f578b239a..2c109cf8b37 100644
> --- a/builtin/fsmonitor--daemon.c
> +++ b/builtin/fsmonitor--daemon.c
> @@ -3,6 +3,7 @@
> #include "parse-options.h"
> #include "fsmonitor.h"
> #include "fsmonitor-ipc.h"
> +#include "compat/fsmonitor/fsm-health.h"
> #include "compat/fsmonitor/fsm-listen.h"
> #include "fsmonitor--daemon.h"
> #include "simple-ipc.h"
> @@ -1136,6 +1137,18 @@ void fsmonitor_publish(struct fsmonitor_daemon_state *state,
> pthread_mutex_unlock(&state->main_lock);
> }
>
> +static void *fsm_health__thread_proc(void *_state)
> +{
> + struct fsmonitor_daemon_state *state = _state;
> +
> + trace2_thread_start("fsm-health");
> +
> + fsm_health__loop(state);
> +
> + trace2_thread_exit();
> + return NULL;
> +}
> +
> static void *fsm_listen__thread_proc(void *_state)
> {
> struct fsmonitor_daemon_state *state = _state;
> @@ -1174,6 +1187,7 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)
> */
> .uds_disallow_chdir = 0
> };
> + int health_started = 0;
> int listener_started = 0;
> int err = 0;
>
> @@ -1201,6 +1215,17 @@ static int fsmonitor_run_daemon_1(struct fsmonitor_daemon_state *state)
> }
> listener_started = 1;
>
> + /*
> + * Start the health thread to watch over our process.
> + */
> + if (pthread_create(&state->health_thread, NULL,
> + fsm_health__thread_proc, state) < 0) {
> + ipc_server_stop_async(state->ipc_server_data);
> + err = error(_("could not start fsmonitor health thread"));
> + goto cleanup;
> + }
> + health_started = 1;
> +
> /*
> * The daemon is now fully functional in background threads.
> * Our primary thread should now just wait while the threads
> @@ -1223,10 +1248,17 @@ cleanup:
> pthread_join(state->listener_thread, NULL);
> }
>
> + if (health_started) {
> + fsm_health__stop_async(state);
> + pthread_join(state->health_thread, NULL);
> + }
> +
> if (err)
> return err;
> if (state->listen_error_code)
> return state->listen_error_code;
> + if (state->health_error_code)
> + return state->health_error_code;
> return 0;
> }
>
> @@ -1242,6 +1274,7 @@ static int fsmonitor_run_daemon(void)
> pthread_mutex_init(&state.main_lock, NULL);
> pthread_cond_init(&state.cookies_cond, NULL);
> state.listen_error_code = 0;
> + state.health_error_code = 0;
> state.current_token_data = fsmonitor_new_token_data();
>
> /* Prepare to (recursively) watch the <worktree-root> directory. */
> @@ -1321,6 +1354,11 @@ static int fsmonitor_run_daemon(void)
> goto done;
> }
>
> + if (fsm_health__ctor(&state)) {
> + err = error(_("could not initialize health thread"));
> + goto done;
> + }
> +
> /*
> * CD out of the worktree root directory.
> *
> @@ -1344,6 +1382,7 @@ done:
> pthread_cond_destroy(&state.cookies_cond);
> pthread_mutex_destroy(&state.main_lock);
> fsm_listen__dtor(&state);
> + fsm_health__dtor(&state);
>
> ipc_server_free(state.ipc_server_data);
>
> diff --git a/compat/fsmonitor/fsm-health-darwin.c b/compat/fsmonitor/fsm-health-darwin.c
> new file mode 100644
> index 00000000000..b9f709e8548
> --- /dev/null
> +++ b/compat/fsmonitor/fsm-health-darwin.c
> @@ -0,0 +1,24 @@
> +#include "cache.h"
> +#include "config.h"
> +#include "fsmonitor.h"
> +#include "fsm-health.h"
> +#include "fsmonitor--daemon.h"
> +
> +int fsm_health__ctor(struct fsmonitor_daemon_state *state)
> +{
> + return 0;
> +}
> +
> +void fsm_health__dtor(struct fsmonitor_daemon_state *state)
> +{
> + return;
> +}
> +
> +void fsm_health__loop(struct fsmonitor_daemon_state *state)
> +{
> + return;
> +}
> +
> +void fsm_health__stop_async(struct fsmonitor_daemon_state *state)
> +{
> +}
> diff --git a/compat/fsmonitor/fsm-health-win32.c b/compat/fsmonitor/fsm-health-win32.c
> new file mode 100644
> index 00000000000..94b1d020f25
> --- /dev/null
> +++ b/compat/fsmonitor/fsm-health-win32.c
> @@ -0,0 +1,72 @@
> +#include "cache.h"
> +#include "config.h"
> +#include "fsmonitor.h"
> +#include "fsm-health.h"
> +#include "fsmonitor--daemon.h"
> +
> +struct fsm_health_data
> +{
> + HANDLE hEventShutdown;
> +
> + HANDLE hHandles[1]; /* the array does not own these handles */
> +#define HEALTH_SHUTDOWN 0
How about defining `HANDLE hHandles[HEALTH_SHUTDOWN + 1]` to indicate that
the constant is used as an offset into `hHandles`?
> + int nr_handles; /* number of active event handles */
> +};
> +
> +int fsm_health__ctor(struct fsmonitor_daemon_state *state)
> +{
> + struct fsm_health_data *data;
> +
> + CALLOC_ARRAY(data, 1);
I _guess_ that this is okay, even if `data` is not actually an array. But
it's a convenient construct to get the parameters right.
Thank you!
Dscho
> +
> + data->hEventShutdown = CreateEvent(NULL, TRUE, FALSE, NULL);
> +
> + data->hHandles[HEALTH_SHUTDOWN] = data->hEventShutdown;
> + data->nr_handles++;
> +
> + state->health_data = data;
> + return 0;
> +}
> +
> +void fsm_health__dtor(struct fsmonitor_daemon_state *state)
> +{
> + struct fsm_health_data *data;
> +
> + if (!state || !state->health_data)
> + return;
> +
> + data = state->health_data;
> +
> + CloseHandle(data->hEventShutdown);
> +
> + FREE_AND_NULL(state->health_data);
> +}
> +
> +void fsm_health__loop(struct fsmonitor_daemon_state *state)
> +{
> + struct fsm_health_data *data = state->health_data;
> +
> + for (;;) {
> + DWORD dwWait = WaitForMultipleObjects(data->nr_handles,
> + data->hHandles,
> + FALSE, INFINITE);
> +
> + if (dwWait == WAIT_OBJECT_0 + HEALTH_SHUTDOWN)
> + goto clean_shutdown;
> +
> + error(_("health thread wait failed [GLE %ld]"),
> + GetLastError());
> + goto force_error_stop;
> + }
> +
> +force_error_stop:
> + state->health_error_code = -1;
> + ipc_server_stop_async(state->ipc_server_data);
> +clean_shutdown:
> + return;
> +}
> +
> +void fsm_health__stop_async(struct fsmonitor_daemon_state *state)
> +{
> + SetEvent(state->health_data->hHandles[HEALTH_SHUTDOWN]);
> +}
> diff --git a/compat/fsmonitor/fsm-health.h b/compat/fsmonitor/fsm-health.h
> new file mode 100644
> index 00000000000..45547ba9380
> --- /dev/null
> +++ b/compat/fsmonitor/fsm-health.h
> @@ -0,0 +1,47 @@
> +#ifndef FSM_HEALTH_H
> +#define FSM_HEALTH_H
> +
> +/* This needs to be implemented by each backend */
> +
> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> +
> +struct fsmonitor_daemon_state;
> +
> +/*
> + * Initialize platform-specific data for the fsmonitor health thread.
> + * This will be called from the main thread PRIOR to staring the
> + * thread.
> + *
> + * Returns 0 if successful.
> + * Returns -1 otherwise.
> + */
> +int fsm_health__ctor(struct fsmonitor_daemon_state *state);
> +
> +/*
> + * Cleanup platform-specific data for the health thread.
> + * This will be called from the main thread AFTER joining the thread.
> + */
> +void fsm_health__dtor(struct fsmonitor_daemon_state *state);
> +
> +/*
> + * The main body of the platform-specific event loop to monitor the
> + * health of the daemon process. This will run in the health thread.
> + *
> + * The health thread should call `ipc_server_stop_async()` if it needs
> + * to cause a shutdown. (It should NOT do so if it receives a shutdown
> + * shutdown signal.)
> + *
> + * It should set `state->health_error_code` to -1 if the daemon should exit
> + * with an error.
> + */
> +void fsm_health__loop(struct fsmonitor_daemon_state *state);
> +
> +/*
> + * Gently request that the health thread shutdown.
> + * It does not wait for it to stop. The caller should do a JOIN
> + * to wait for it.
> + */
> +void fsm_health__stop_async(struct fsmonitor_daemon_state *state);
> +
> +#endif /* HAVE_FSMONITOR_DAEMON_BACKEND */
> +#endif /* FSM_HEALTH_H */
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index b8f9f7a0388..16ace43d1c7 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -289,12 +289,14 @@ if(SUPPORTS_SIMPLE_IPC)
> if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
> add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND)
> list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-win32.c)
> + list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-win32.c)
>
> add_compile_definitions(HAVE_FSMONITOR_OS_SETTINGS)
> list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-win32.c)
> elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND)
> list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-darwin.c)
> + list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-darwin.c)
>
> add_compile_definitions(HAVE_FSMONITOR_OS_SETTINGS)
> list(APPEND compat_SOURCES compat/fsmonitor/fsm-settings-darwin.c)
> diff --git a/fsmonitor--daemon.h b/fsmonitor--daemon.h
> index 2c6fa1a5d91..2102a5c9ff5 100644
> --- a/fsmonitor--daemon.h
> +++ b/fsmonitor--daemon.h
> @@ -34,9 +34,11 @@ void fsmonitor_batch__free_list(struct fsmonitor_batch *batch);
> void fsmonitor_batch__add_path(struct fsmonitor_batch *batch, const char *path);
>
> struct fsm_listen_data; /* opaque platform-specific data for listener thread */
> +struct fsm_health_data; /* opaque platform-specific data for health thread */
>
> struct fsmonitor_daemon_state {
> pthread_t listener_thread;
> + pthread_t health_thread;
> pthread_mutex_t main_lock;
>
> struct strbuf path_worktree_watch;
> @@ -51,7 +53,9 @@ struct fsmonitor_daemon_state {
> struct hashmap cookies;
>
> int listen_error_code;
> + int health_error_code;
> struct fsm_listen_data *listen_data;
> + struct fsm_health_data *health_data;
>
> struct ipc_server_data *ipc_server_data;
> struct strbuf path_ipc;
> --
> gitgitgadget
>
>
