Re: Bug report: orphaned pack-objects after killing upload-pack on [

[Jeff King <peff@peff.net>]

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