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

From: Jeff King <peff@peff.net>
Date: Apr 20, 2016, 21:51 UTC
Message-ID: <20160420215117.GA18297@sigill.intra.peff.net>
In-Reply-To: <xmqqwpnst1yb.fsf@gitster.mtv.corp.google.com>

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

Show 24 quoted lines

> 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