Re: [PATCH 02/23] fsmonitor-ipc: create client routines for git-fsmonitor--daemon

[Derrick Stolee <stolee@gmail.com>]

On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> +#define FSMONITOR_DAEMON_IS_SUPPORTED 1
> +#else
> +#define FSMONITOR_DAEMON_IS_SUPPORTED 0
> +#endif
> +
> +/*
> + * A trivial function so that this source file always defines at least
> + * one symbol even when the feature is not supported.  This quiets an
> + * annoying compiler error.
> + */
> +int fsmonitor_ipc__is_supported(void)
> +{
> +	return FSMONITOR_DAEMON_IS_SUPPORTED;
> +}

I don't see any other use of FSMONITOR_DAEMON_IS_SUPPORTED,
so I was thinking you could use the #ifdef/#else/#endif
construct within the implementation of this method instead
of creating a macro outside. But my suggestion might be an
anti-pattern, so feel free to ignore me.

> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> +
> +GIT_PATH_FUNC(fsmonitor_ipc__get_path, "fsmonitor")
> +
> +enum ipc_active_state fsmonitor_ipc__get_state(void)
> +{
> +	return ipc_get_active_state(fsmonitor_ipc__get_path());
> +}
> +
> +static int spawn_daemon(void)
> +{
> +	const char *args[] = { "fsmonitor--daemon", "--start", NULL };
> +
> +	return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
> +				    "fsmonitor");
> +}
> +
> +int fsmonitor_ipc__send_query(const char *since_token,
> +			      struct strbuf *answer)
> +{
> +	int ret = -1;
> +	int tried_to_spawn = 0;
> +	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
> +	struct ipc_client_connection *connection = NULL;
> +	struct ipc_client_connect_options options
> +		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
> +
> +	options.wait_if_busy = 1;
> +	options.wait_if_not_found = 0;
> +
> +	trace2_region_enter("fsm_client", "query", NULL);
> +
> +	trace2_data_string("fsm_client", NULL, "query/command",
> +			   since_token);
> +
> +try_again:
> +	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
> +				       &connection);
> +
> +	switch (state) {
> +	case IPC_STATE__LISTENING:
> +		ret = ipc_client_send_command_to_connection(
> +			connection, since_token, answer);
> +		ipc_client_close_connection(connection);
> +
> +		trace2_data_intmax("fsm_client", NULL,
> +				   "query/response-length", answer->len);
> +
> +		if (fsmonitor_is_trivial_response(answer))
> +			trace2_data_intmax("fsm_client", NULL,
> +					   "query/trivial-response", 1);
> +
> +		goto done;
> +
> +	case IPC_STATE__NOT_LISTENING:
> +		ret = error(_("fsmonitor_ipc__send_query: daemon not available"));
> +		goto done;

I'll need to read up on the IPC layer a bit to find out the difference
between IPC_STATE__NOT_LISTENING and IPC_STATE__PATH_NOT_FOUND. When
testing on my macOS machine, I got this error. I was expecting the
daemon to be spawned. After spawning it myself, it started working.

I expect that there are some cases where the process can fail and the
named pipe is not cleaned up. Let's investigate that soon. I should
make it clear that I had tested the builtin FS Monitor on this machine
a few weeks ago, but hadn't been using it much since. We should auto-
recover from this situation.

But also: what is the cost of treating these two cases the same? Could
we attempt to "restart" the daemon by spawning a new one? Will the new
one find a way to kill a stale one?

(Reading on.)

> +	case IPC_STATE__PATH_NOT_FOUND:
> +		if (tried_to_spawn)
> +			goto done;
> +
> +		tried_to_spawn++;
> +		if (spawn_daemon())
> +			goto done;

This should return zero on success, OK.

> +		/*
> +		 * Try again, but this time give the daemon a chance to
> +		 * actually create the pipe/socket.
> +		 *
> +		 * Granted, the daemon just started so it can't possibly have
> +		 * any FS cached yet, so we'll always get a trivial answer.
> +		 * BUT the answer should include a new token that can serve
> +		 * as the basis for subsequent requests.
> +		 */
> +		options.wait_if_not_found = 1;
> +		goto try_again;

Because of the tried_to_spawn check, we will re-run the request over
IPC but will not retry the spawn_daemon() request. I'm unsure how
this could be helpful: is it possible that spawn_daemon() returns a
non-zero error code after starting the daemon and somehow that
daemon starts working? Or, is this a race-condition thing with parallel
processes also starting up the daemon? It could be good to use this
comment to describe why a retry might be helpful.

> +
> +	case IPC_STATE__INVALID_PATH:
> +		ret = error(_("fsmonitor_ipc__send_query: invalid path '%s'"),
> +			    fsmonitor_ipc__get_path());
> +		goto done;
> +
> +	case IPC_STATE__OTHER_ERROR:
> +	default:
> +		ret = error(_("fsmonitor_ipc__send_query: unspecified error on '%s'"),
> +			    fsmonitor_ipc__get_path());
> +		goto done;
> +	}
> +
> +done:
> +	trace2_region_leave("fsm_client", "query", NULL);
> +
> +	return ret;
> +}
> +
> +int fsmonitor_ipc__send_command(const char *command,
> +				struct strbuf *answer)
> +{
> +	struct ipc_client_connection *connection = NULL;
> +	struct ipc_client_connect_options options
> +		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
> +	int ret;
> +	enum ipc_active_state state;
> +
> +	strbuf_reset(answer);
> +
> +	options.wait_if_busy = 1;
> +	options.wait_if_not_found = 0;
> +
> +	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
> +				       &connection);
> +	if (state != IPC_STATE__LISTENING) {
> +		die("fsmonitor--daemon is not running");
> +		return -1;
> +	}
> +
> +	ret = ipc_client_send_command_to_connection(connection, command, answer);
> +	ipc_client_close_connection(connection);
> +
> +	if (ret == -1) {
> +		die("could not send '%s' command to fsmonitor--daemon",
> +		    command);
> +		return -1;
> +	}
> +
> +	return 0;
> +}

I wondier if this ...send_command() method is too generic. It might
be nice to have more structure to its inputs and outputs to lessen
the cognitive load when plugging into other portions of the code.
However, I'll wait to see what those consumers look like in case the
generality is merited.
>  struct category_description {
>  	uint32_t category;
> @@ -664,6 +665,9 @@ void get_version_info(struct strbuf *buf, int show_build_options)
>  		strbuf_addf(buf, "sizeof-size_t: %d\n", (int)sizeof(size_t));
>  		strbuf_addf(buf, "shell-path: %s\n", SHELL_PATH);
>  		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
> +
> +		if (fsmonitor_ipc__is_supported())
> +			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");

This change might deserve its own patch, including some documentation
about how users can use 'git version --build-options' to determine if
the builtin FS Monitor feature is available on their platform.

Thanks,
-Stolee