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

From: Jeff King <peff@peff.net>
Date: Nov 28, 2020, 06:30 UTC
Message-ID: <X8Hub2OeFy3V6MhV@coredump.intra.peff.net>
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...

Show 16 quoted lines

> --- >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