[v5,11/30] fsmonitor--daemon: implement 'start' command

Commit Message

Jeff Hostetler Feb. 11, 2022, 8:56 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Implement 'git fsmonitor--daemon start' command.  This command starts
an instance of 'git fsmonitor--daemon run' in the background using
the new 'start_bg_command()' function.

We avoid the fork-and-call technique on Unix systems in favor of a
fork-and-exec technique.  This gives us more uniform Trace2 child-*
events.  It also makes our usage more consistent with Windows usage.

On Windows, teach 'git fsmonitor--daemon run' to optionally call
'FreeConsole()' to release handles to the inherited Win32 console
(despite being passed invalid handles for stdin/out/err).  Without
this, command prompts and powershell terminal windows could hang
in "exit" until the last background child process exited or released
their Win32 console handle.  (This was not seen with git-bash shells
because they don't have a Win32 console attached to them.)

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 builtin/fsmonitor--daemon.c | 107 +++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 2 deletions(-)

Comments

Johannes Schindelin Feb. 17, 2022, 4:50 p.m. UTC | #1

Hi Jeff,

On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:

> +static int try_to_start_background_daemon(void)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	enum start_bg_result sbgr;
> +
> +	/*
> +	 * Before we try to create a background daemon process, see
> +	 * if a daemon process is already listening.  This makes it
> +	 * easier for us to report an already-listening error to the
> +	 * console, since our spawn/daemon can only report the success
> +	 * of creating the background process (and not whether it
> +	 * immediately exited).
> +	 */
> +	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
> +		die("fsmonitor--daemon is already running '%s'",
> +		    the_repository->worktree);
> +
> +	printf(_("starting fsmonitor-daemon in '%s'\n"),
> +	       the_repository->worktree);
> +	fflush(stdout);

Just like for the patch before, my question whether `stdout` or `stderr`
is preferable here?

> +	cp.git_cmd = 1;
> +
> +	strvec_push(&cp.args, "fsmonitor--daemon");
> +	strvec_push(&cp.args, "run");
> +	strvec_push(&cp.args, "--free-console");

We could call this function `--detached` or `--detach`, too, to indicate
the intention.

I am fine with the code as-is, just wanted to make sure that these
questions are on record ;-)

Ciao,
Dscho

Johannes Schindelin Feb. 24, 2022, 3:30 p.m. UTC | #2

Hi Jeff,

something I missed in my review, but which causes failures in `seen`
because of the interplay with `ac/usage-string-fixups`:

On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:

>  int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
>  {
>  	const char *subcmd;
> +	int free_console = 0;
>
>  	struct option options[] = {
> +		OPT_BOOL(0, "free-console", &free_console, N_("free console")),
>  		OPT_INTEGER(0, "ipc-threads",
>  			    &fsmonitor__ipc_threads,
>  			    N_("use <n> ipc worker threads")),
> +		OPT_INTEGER(0, "start-timeout",
> +			    &fsmonitor__start_timeout_sec,
> +			    N_("Max seconds to wait for background daemon startup")),

Git is about to be stricter about these option usage strings: they are no
longer allowed to start with an upper-case letter. This diff fixes it for me:

-- snip --
From: Johannes Schindelin <[e-mail]>
Date: Thu, 24 Feb 2022 15:48:01 +0100
Subject: [PATCH] fixup??? fsmonitor--daemon: implement 'start' command

There is a patch series in `seen` that errors out on option usage
strings starting with a capital letter. Let's avoid that preemptively.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 builtin/fsmonitor--daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 591433e897df..775e4de5584d 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1504,7 +1504,7 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
 			    N_("use <n> ipc worker threads")),
 		OPT_INTEGER(0, "start-timeout",
 			    &fsmonitor__start_timeout_sec,
-			    N_("Max seconds to wait for background daemon startup")),
+			    N_("max seconds to wait for background daemon startup")),

 		OPT_END()
 	};
--
2.35.1.windows.2
-- snap --

Could I ask you to squash this in before you send a new iteration?

Thank you,
Dscho


> +
>  		OPT_END()
>  	};
>

Patch

diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index b5ebd1eca33..8988440b7c1 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -9,6 +9,7 @@ 
 #include "khash.h"
 
 static const char * const builtin_fsmonitor__daemon_usage[] = {
+	N_("git fsmonitor--daemon start [<options>]"),
 	N_("git fsmonitor--daemon run [<options>]"),
 	N_("git fsmonitor--daemon stop"),
 	N_("git fsmonitor--daemon status"),
@@ -22,6 +23,9 @@  static const char * const builtin_fsmonitor__daemon_usage[] = {
 #define FSMONITOR__IPC_THREADS "fsmonitor.ipcthreads"
 static int fsmonitor__ipc_threads = 8;
 
+#define FSMONITOR__START_TIMEOUT "fsmonitor.starttimeout"
+static int fsmonitor__start_timeout_sec = 60;
+
 static int fsmonitor_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, FSMONITOR__IPC_THREADS)) {
@@ -33,6 +37,15 @@  static int fsmonitor_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, FSMONITOR__START_TIMEOUT)) {
+		int i = git_config_int(var, value);
+		if (i < 0)
+			return error(_("value of '%s' out of range: %d"),
+				     FSMONITOR__START_TIMEOUT, i);
+		fsmonitor__start_timeout_sec = i;
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 
@@ -237,7 +250,7 @@  done:
 	return err;
 }
 
-static int try_to_run_foreground_daemon(void)
+static int try_to_run_foreground_daemon(int free_console)
 {
 	/*
 	 * Technically, we don't need to probe for an existing daemon
@@ -255,17 +268,104 @@  static int try_to_run_foreground_daemon(void)
 	       the_repository->worktree);
 	fflush(stdout);
 
+#ifdef GIT_WINDOWS_NATIVE
+	if (free_console)
+		FreeConsole();
+#endif
+
 	return !!fsmonitor_run_daemon();
 }
 
+static start_bg_wait_cb bg_wait_cb;
+
+static int bg_wait_cb(const struct child_process *cp, void *cb_data)
+{
+	enum ipc_active_state s = fsmonitor_ipc__get_state();
+
+	switch (s) {
+	case IPC_STATE__LISTENING:
+		/* child is "ready" */
+		return 0;
+
+	case IPC_STATE__NOT_LISTENING:
+	case IPC_STATE__PATH_NOT_FOUND:
+		/* give child more time */
+		return 1;
+
+	default:
+	case IPC_STATE__INVALID_PATH:
+	case IPC_STATE__OTHER_ERROR:
+		/* all the time in world won't help */
+		return -1;
+	}
+}
+
+static int try_to_start_background_daemon(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	enum start_bg_result sbgr;
+
+	/*
+	 * Before we try to create a background daemon process, see
+	 * if a daemon process is already listening.  This makes it
+	 * easier for us to report an already-listening error to the
+	 * console, since our spawn/daemon can only report the success
+	 * of creating the background process (and not whether it
+	 * immediately exited).
+	 */
+	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
+		die("fsmonitor--daemon is already running '%s'",
+		    the_repository->worktree);
+
+	printf(_("starting fsmonitor-daemon in '%s'\n"),
+	       the_repository->worktree);
+	fflush(stdout);
+
+	cp.git_cmd = 1;
+
+	strvec_push(&cp.args, "fsmonitor--daemon");
+	strvec_push(&cp.args, "run");
+	strvec_push(&cp.args, "--free-console");
+	strvec_pushf(&cp.args, "--ipc-threads=%d", fsmonitor__ipc_threads);
+
+	cp.no_stdin = 1;
+	cp.no_stdout = 1;
+	cp.no_stderr = 1;
+
+	sbgr = start_bg_command(&cp, bg_wait_cb, NULL,
+				fsmonitor__start_timeout_sec);
+
+	switch (sbgr) {
+	case SBGR_READY:
+		return 0;
+
+	default:
+	case SBGR_ERROR:
+	case SBGR_CB_ERROR:
+		return error("daemon failed to start");
+
+	case SBGR_TIMEOUT:
+		return error("daemon not online yet");
+
+	case SBGR_DIED:
+		return error("daemon terminated");
+	}
+}
+
 int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
 {
 	const char *subcmd;
+	int free_console = 0;
 
 	struct option options[] = {
+		OPT_BOOL(0, "free-console", &free_console, N_("free console")),
 		OPT_INTEGER(0, "ipc-threads",
 			    &fsmonitor__ipc_threads,
 			    N_("use <n> ipc worker threads")),
+		OPT_INTEGER(0, "start-timeout",
+			    &fsmonitor__start_timeout_sec,
+			    N_("Max seconds to wait for background daemon startup")),
+
 		OPT_END()
 	};
 
@@ -281,8 +381,11 @@  int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
 		die(_("invalid 'ipc-threads' value (%d)"),
 		    fsmonitor__ipc_threads);
 
+	if (!strcmp(subcmd, "start"))
+		return !!try_to_start_background_daemon();
+
 	if (!strcmp(subcmd, "run"))
-		return !!try_to_run_foreground_daemon();
+		return !!try_to_run_foreground_daemon(free_console);
 
 	if (!strcmp(subcmd, "stop"))
 		return !!do_as_client__send_stop();