git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Josh Steadmon <steadmon@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] run-command: don't spam trace2_child_exit()
Date: Thu, 28 Apr 2022 14:46:47 -0700	[thread overview]
Message-ID: <xmqqr15gev94.fsf@gitster.g> (raw)
In-Reply-To: <4616d09ffa632bd2c9e308a713c4bdf2a1328c3c.1651179450.git.steadmon@google.com> (Josh Steadmon's message of "Thu, 28 Apr 2022 13:58:10 -0700")

Josh Steadmon <steadmon@google.com> writes:

> In rare cases, wait_or_whine() cannot determine a child process's exit
> status (and will return -1 in this case). This can cause Git to issue
> trace2 child_exit events despite the fact that the child is still
> running.

Rather, we do not even know if the child is still running when it
happens, right?  It is curious what "rare cases" makes the symptom
appear.  Do we know?

The patch looks OK from the "we do not know the child exited in this
case, so we shouldn't be reporting the child exit" point of view, of
course.  Having one event that started a child in the log and then
having millions of events that reports the exit of the (same) child
is way too broken.  With this change, we remove these phoney exit
events from the log.

Do we know, for such a child process that caused these millions
phoney exit events, we got a real exit event at the end?  Otherwise,
we'd still have a similar problem in the opposite direction, i.e. a
child has a start event recorded, many exit event discarded but the
log lacks the true exit event for the child, implying that the child
is still running because we failed to log its exit?

>  int finish_command_in_signal(struct child_process *cmd)
>  {
>  	int ret = wait_or_whine(cmd->pid, cmd->args.v[0], 1);
> -	trace2_child_exit(cmd, ret);
> +	if (ret != -1)
> +		trace2_child_exit(cmd, ret);
>  	return ret;
>  }

Will queue; thanks.

  reply	other threads:[~2022-04-28 21:47 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-28 20:58 Josh Steadmon
2022-04-28 21:46 ` Junio C Hamano [this message]
2022-05-03 14:59   ` Jeff Hostetler
2022-05-05 19:58     ` Josh Steadmon
2022-05-10 20:37       ` Jeff Hostetler
2022-06-07 18:45         ` Josh Steadmon
2022-05-05 19:44   ` Josh Steadmon
2022-06-07 18:21 ` [PATCH v2] " Josh Steadmon
2022-06-07 22:09   ` Ævar Arnfjörð Bjarmason
2022-06-10 15:31   ` Jeff Hostetler

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqqr15gev94.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=steadmon@google.com \
    --subject='Re: [PATCH] run-command: don'\''t spam trace2_child_exit()' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).