On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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.
