git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: "Junio C Hamano" <gitster@pobox.com>, 唐宇奕 <winglovet@gmail.com>,
	git@vger.kernel.org
Subject: Re: Bug report: orphaned pack-objects after killing upload-pack on [
Date: Sat, 28 Nov 2020 01:30:07 -0500	[thread overview]
Message-ID: <X8Hub2OeFy3V6MhV@coredump.intra.peff.net> (raw)
In-Reply-To: <d0fcfa7f-9835-a9e6-c4a4-af4de177ff8c@web.de>

On Fri, Nov 27, 2020 at 09:43:06PM +0100, René Scharfe wrote:

> > [...zombie processes...]
> OK, so overall the situation sounds a bit messy to me and perhaps
> there's room for improvement, but I agree now that we can leave the
> specialists (init, tini) to deal with our zombies.

Keep in mind we are not changing anything here, either. clean_on_exit is
kicking in when we already would be exiting without calling wait(). We
could _also_ instruct run-command to wait, but nobody seems to be
complaining about it.

> With the debug patch above and GIT_DEBUG_ABANDON_CHILD=git-upload-pack I
> need the following patch get rid of the spawned process:

I don't think that is an interesting case, though. We've been discussing
the spawn between upload-pack and its child pack-objects, but here
you're running a bogus infinite loop instead of upload-pack. It will
spin forever, because the client is expecting it to say something.

In a normal setup, a severing of the connection between the client and
upload-pack will be noticed quickly, because each one is always either
trying to read from or write to the other (the exception is while
pack-objects is processing without progress meters, but there we send a
keep-alive every 5 seconds).

If one of them were to spin in a true infinite loop due to a bug (but
not break the connection), we would wait forever. But the solution there
is a timeout on inactivity.

So...

> --- >8 ---
> 
> diff --git a/connect.c b/connect.c
> index 8b8f56cf6d..e1b1b73ef5 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -1369,6 +1369,7 @@ struct child_process *git_connect(int fd[2], const char *url,
> 
>  		conn->use_shell = 1;
>  		conn->in = conn->out = -1;
> +		conn->clean_on_exit = 1;
>  		if (protocol == PROTO_SSH) {
>  			char *ssh_host = hostandport;
>  			const char *port = NULL;
> 
> --- 8< ---

I don't think there's much point here. If the client side dies, it will
close the socket, and upload-pack would notice anyway. It's only your
fake "spin without doing any I/O" patch that misbehaves.

Moreover, this wouldn't do _anything_ in many cases, because upload-pack
is going to be on the far side of a network socket. So really you'd be
killing ssh here for those cases (which then likewise would close the
socket, but this isn't necessary because ssh will also exit when it sees
us close _our_ end). And it would do nothing at all for git-over-http,
git://, etc.

> So is there a downside to clean_on_exit?  It doesn't make sense when we
> start browsers or pagers, but for hooks and helpers (which are probably
> the majority of started processes) cascading program termination makes
> sense, no?

In general, no, I suspect clean_on_exit wouldn't be hurting anything if
we used it more. But generally we get a natural cascade of termination
because exiting processes close the input and output pipes going to the
other sub-processes.

-Peff

  reply	other threads:[~2020-11-28 22:55 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19  8:18 唐宇奕
2020-11-20 18:52 ` Bug report: orphaned pack-objects after killing upload-pack on [was: (no subject)] René Scharfe
2020-11-21  0:29   ` Jeff King
2020-11-21 21:54     ` Bug report: orphaned pack-objects after killing upload-pack on [ Junio C Hamano
2020-11-24  3:21       ` 唐宇奕
2020-11-24  9:11       ` Jeff King
2020-11-25 21:42         ` Junio C Hamano
2020-11-26  0:53           ` Jeff King
2020-11-26  1:04             ` Junio C Hamano
2020-11-26 20:04               ` René Scharfe
2020-11-27  4:17                 ` Jeff King
2020-11-27 20:43                   ` René Scharfe
2020-11-28  6:30                     ` Jeff King [this message]
2020-12-01 12:15                 ` Jeff King
2020-12-02 11:45                   ` René Scharfe
2020-12-02 22:14                     ` Junio C Hamano

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=X8Hub2OeFy3V6MhV@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=winglovet@gmail.com \
    --subject='Re: Bug report: orphaned pack-objects after killing upload-pack on [' \
    /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).