On Tue, Jul 23, 2019 at 10:33:10AM -0700, Junio C Hamano wrote:
> > + 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
