Re: [PATCH 0/5] fix deadlock in git-push

[Jeff King <peff@peff.net>]

On Wed, Apr 20, 2016 at 02:17:16PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The first patch below fixes the deadlock. Unfortunately, it turns it
> > into a likely SIGPIPE death. Which is an improvement, but not ideal.
> >
> > Patches 2 and 3 address that by fixing the way we handle SIGPIPE in
> > async threads.
> >
> > Patches 4 and 5 are cleanups to earlier topics that are enabled by the
> > new SIGPIPE handling.
> >
> >   [1/5]: send-pack: close demux pipe before finishing async process
> >   [2/5]: run-command: teach async threads to ignore SIGPIPE
> >   [3/5]: send-pack: isolate sigpipe in demuxer thread
> >   [4/5]: fetch-pack: isolate sigpipe in demuxer thread
> >   [5/5]: t5504: drop sigpipe=ok from push tests
> 
> Thanks for a very well explained series.
> 
> We do not call finish_async (rather, we do not use async) from that
> many places, and from a cursory look this codepath is the only case
> where we may encounter this kind of deadlock (the ones in
> receive-pack is about relaying the error messages back to the other
> end over sideband multiplexing)?

Yeah, I checked the other demuxer in fetch-pack, but it does not have
any early returns like this (it just dies :) ).

It does not do an explicit close on demux.out, but I think it is
effectively closed when we hand it off to index-pack/unpack-objects via
cmd.in.

Arguably finish_async() should "close(demux.out)" itself, but that felt
like an ownership violation. Yes, that's how "struct async" passes out
the descriptor, but the caller is then expected to handle it, and
correct callers will typically have closed it themselves, handed it off
to a sub-process, etc. Closing it in finish_async() runs the risk that
we just call close() on a descriptor number that is either unattached,
or attached to some random other thing.

-Peff