All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 6/7] run-command: create start_bg_command
Date: Thu, 16 Sep 2021 07:56:52 +0200	[thread overview]
Message-ID: <87r1dpgibi.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <f97038a563d889d740a7e968fcbdfaadb41e2008.1631738177.git.gitgitgadget@gmail.com>


On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Create a variation of `run_command()` and `start_command()` to launch a command
> into the background and optionally wait for it to become "ready" before returning.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  run-command.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  run-command.h |  48 ++++++++++++++++++++
>  2 files changed, 171 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 3e4e082e94d..fe75fd08f74 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1901,3 +1901,126 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
>  	}
>  	strvec_pushf(env_array, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir);
>  }
> +
> +enum start_bg_result start_bg_command(struct child_process *cmd,
> +				      start_bg_wait_cb *wait_cb,
> +				      void *cb_data,
> +				      unsigned int timeout_sec)
> +{
> +	enum start_bg_result sbgr = SBGR_ERROR;
> +	int ret;
> +	int wait_status;
> +	pid_t pid_seen;
> +	time_t time_limit;
> +
> +	/*
> +	 * Silently disallow child cleanup -- even if requested.
> +	 * The child process should persist in the background
> +	 * and possibly/probably after this process exits.  That
> +	 * is, don't kill the child during our atexit routine.
> +	 */
> +	cmd->clean_on_exit = 0;

Since we have no existing users, can we change this to:

if (cmd->clean_on_exit)
    BUG("start_bg_command() doesn't support non-zero clean_on_exit");

Just silently discarding what the caller asked for seems like the wrong
thing to do, why the silence?

> +
> +	ret = start_command(cmd);
> +	if (ret) {
> +		/*
> +		 * We assume that if `start_command()` fails, we
> +		 * either get a complete `trace2_child_start() /
> +		 * trace2_child_exit()` pair or it fails before the
> +		 * `trace2_child_start()` is emitted, so we do not
> +		 * need to worry about it here.
> +		 *
> +		 * We also assume that `start_command()` does not add
> +		 * us to the cleanup list.  And that it calls
> +		 * calls `child_process_clear()`.
> +		 */

These all look like sensible things to assume, but I think commentary /
writing on this would be much better just documenting that the
start_command() API does this in its comment in run-command.h, or
perhaps the comment starting with "The functions: child_process_init".

> +		sbgr = SBGR_ERROR;
> +		goto done;
> +	}
> +
> +	time(&time_limit);
> +	time_limit += timeout_sec;
> +
> +wait:
> +	pid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);
> +
> +	if (pid_seen == 0) {
> +		/*
> +		 * The child is currently running.  Ask the callback
> +		 * if the child is ready to do work or whether we
> +		 * should keep waiting for it to boot up.
> +		 */
> +		ret = (*wait_cb)(cb_data, cmd);
> +		if (!ret) {
> +			/*
> +			 * The child is running and "ready".
> +			 *
> +			 * NEEDSWORK: As we prepare to orphan (release to
> +			 * the background) this child, it is not appropriate
> +			 * to emit a `trace2_child_exit()` event.  Should we
> +			 * create a new event for this case?
> +			 */
> +			sbgr = SBGR_READY;
> +			goto done;

Per api-trace2.txt:

    `"child_exit"`::
            This event is generated after the current process has returned
            from the waitpid() and collected the exit information from the
            child.

My (perhaps wrong) reading of that is that yes, we should do that, after
all if we've released the child are we otherwise going to hear from them
again in any way trace2 could log with a child_exit later?

Ditto the "commentary better elsewhere" whatever the result of this
discussion is, let's add a note to api-trace2.txt about how detached
children are handled by child_exit events.

> [...]
> +			 * NEEDSWORK: Like the "ready" case, should we
> +			 * log a custom child-something Trace2 event here?
> +			 */
> +			sbgr = SBGR_TIMEOUT;
> +			goto done;

[...]

> +		} else {
> +			/*
> +			 * The cb gave up on this child.
> +			 *
> +			 * NEEDSWORK: Like above, should we log a custom
> +			 * Trace2 child-something event here?
> +			 */
> +			sbgr = SBGR_CB_ERROR;
> +			goto done;

*ditto*

> +		}
> +	}
> +
> +	if (pid_seen == cmd->pid) {
> +		int child_code = -1;
> +
> +		/*
> +		 * The child started, but exited or was terminated
> +		 * before becoming "ready".
> +		 *
> +		 * We try to match the behavior of `wait_or_whine()`
> +		 * and convert the child's status to a return code for
> +		 * tracing purposes and emit the `trace2_child_exit()`
> +		 * event.
> +		 */
> +		if (WIFEXITED(wait_status))
> +			child_code = WEXITSTATUS(wait_status);
> +		else if (WIFSIGNALED(wait_status))
> +			child_code = WTERMSIG(wait_status) + 128;
> +		trace2_child_exit(cmd, child_code);

Although with the above: Perhaps I've missed some subtleties...

> [...]
> diff --git a/run-command.h b/run-command.h
> index af1296769f9..58065197d1b 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -496,4 +496,52 @@ int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,
>   */
>  void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);
>  
> +/**
> + * Possible return values for `start_bg_command()`.
> + */
> +enum start_bg_result {
> +	/* child process is "ready" */
> +	SBGR_READY = 0,

Clarity nit, whenever I see an explicit enum assignment I start hunting
for things that depend on the specific value, as we sometimes do, but
there appears to be nothing...

> +	/* child process could not be started */
> +	SBGR_ERROR,
> +
> +	/* callback error when testing for "ready" */
> +	SBGR_CB_ERROR,
> +
> +	/* timeout expired waiting for child to become "ready" */
> +	SBGR_TIMEOUT,
> +
> +	/* child process exited or was signalled before becomming "ready" */
> +	SBGR_DIED,

...I'd think if we were tweaking the value we'd want to put all the
error values at < 0, but I'm fine with this pattern of them being
positive, it encourages users to use switch/case & compiler checks, and
since it's all new users...

> + * This is a combination of `start_command()` and `finish_command()`, but

Doc nit: I think with whatever syntax standard we use for /** comments
that we prefer functions like_this() not quoted `like_this()`, that
being for values like `123` or whatever. But I'm just going by a quick
grep there & what Emacs is highlighting.

  parent reply	other threads:[~2021-09-16  6:12 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-15 20:36 [PATCH 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-15 20:36 ` [PATCH 1/7] trace2: fix memory leak of thread name Jeff Hostetler via GitGitGadget
2021-09-15 21:01   ` Junio C Hamano
2021-09-16  4:19     ` Taylor Blau
2021-09-16  5:35   ` Ævar Arnfjörð Bjarmason
2021-09-16  5:43     ` Taylor Blau
2021-09-16  8:01       ` Ævar Arnfjörð Bjarmason
2021-09-16 15:35         ` Jeff Hostetler
2021-09-16 15:47           ` Ævar Arnfjörð Bjarmason
2021-09-16 19:13         ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-15 20:43   ` Eric Sunshine
2021-09-17 16:52     ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-15 21:06   ` Junio C Hamano
2021-09-17 16:58     ` Jeff Hostetler
2021-09-18  7:03       ` Carlo Arenas
2021-09-20 15:51       ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-16  5:40   ` Ævar Arnfjörð Bjarmason
2021-09-17 17:27     ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-16  5:47   ` Ævar Arnfjörð Bjarmason
2021-09-17 18:10     ` Jeff Hostetler
2021-09-17 19:14       ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16  4:53   ` Taylor Blau
2021-09-16  4:58     ` Taylor Blau
2021-09-16  5:56   ` Ævar Arnfjörð Bjarmason [this message]
2021-09-15 20:36 ` [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16  5:06   ` Taylor Blau
2021-09-17 19:41     ` Jeff Hostetler
2021-09-18  8:59       ` Ævar Arnfjörð Bjarmason
2021-09-16  5:55   ` Ævar Arnfjörð Bjarmason
2021-09-20 15:36 ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 1/7] trace2: add trace2_child_ready() to report on background children Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-23 15:03     ` Ævar Arnfjörð Bjarmason
2021-09-23 17:58       ` Jeff Hostetler
2021-09-23 18:37       ` Junio C Hamano
2021-11-04 19:46     ` Adam Dinwoodie
2021-11-04 20:14       ` Ramsay Jones
2021-11-08 14:58       ` Jeff Hostetler
2021-11-08 23:59       ` Johannes Schindelin
2021-11-09 18:53         ` Ramsay Jones
2021-11-09 23:01           ` Johannes Schindelin
2021-11-09 23:34             ` Junio C Hamano
2021-11-10 12:27               ` Johannes Schindelin
2021-11-12  8:56                 ` Adam Dinwoodie
2021-11-12 16:01                   ` Junio C Hamano
2021-11-12 21:33                     ` Adam Dinwoodie
2021-11-16 10:56                       ` Johannes Schindelin
2021-11-16 11:02                   ` Johannes Schindelin
2021-11-13 19:11                 ` Ramsay Jones
2021-11-14 19:34                   ` Ramsay Jones
2021-11-14 20:10                   ` Adam Dinwoodie
2021-09-23 14:33   ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Ævar Arnfjörð Bjarmason
2021-09-23 17:12     ` Jeff Hostetler
2021-09-23 20:47       ` Ævar Arnfjörð Bjarmason
2021-09-27 13:37         ` Jeff Hostetler
2021-09-23 17:51     ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r1dpgibi.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.