Re: [PATCH] Close transport helper on protocol error

From: Jeff King <peff@peff.net>
Date: Jul 23, 2019, 19:40 UTC
Message-ID: <20190723194009.GB3879@sigill.intra.peff.net>
In-Reply-To: <xmqq5znsu04p.fsf@gitster-ct.c.googlers.com>

On Tue, Jul 23, 2019 at 10:33:10AM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> > +		if (recvline(data, &buf)){
> > +			release_helper(transport);
> >  			exit(128);
> > +		}
> 
> This, together with other exit(128) we see in this patch now have
> release_helepr() in front of it, which is in line with what the log
> message claims that the patch does.
> 
> I however wonder if we want to do a bit more, perhaps with atexit().
> I am not hinting-suggesting to do so (as you said, if the init
> process ought to take care of the zombies, the patch under review
> might already be unneeded, and atexit() makes things even worse),
> but having trouble to convince that this patch stops at the right
> place.

I was just writing a similar comment when I read this. It probably fixes the particular case the OP saw, but Git is quite happy to die() in various code-paths when it encounters an error.

Rather than try to annotate every possible exit, atexit() might be a better solution. But isn't this a more general problem even than that? Lots of parts of Git may spawn a sub-process and assume that the children will be reaped automatically (as long as they do exit, but that is usually guaranteed by us closing their input pipes when we ourselves exit).

So I think you'd have to atexit() quite a few places. Or possibly instrument run_command() to do so, though it might need some extra annotation to mark whether a particular sub-process should be waited for (there is some prior art in the child_process.clean_on_exit option).

At which point I do wonder if this is better handled by a wrapper process which simply reaps everything. And indeed, people have already come up with similar solutions for containers:

  https://github.com/Yelp/dumb-init

So I dunno. I am not really opposed to this patch, as it is just adding some extra cleanup. But it seems like it is really hitting the tip of the iceberg, and I'm not sure it's an iceberg I'd like to get to the bottom of.

-Peff