Re: [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error

From: Jeff King <peff@peff.net>
Date: Apr 14, 2011, 13:54 UTC
Message-ID: <20110414135449.GD12410@sigill.intra.peff.net>
In-Reply-To: <201104132153.06429.j6t@kdbg.org>

On Wed, Apr 13, 2011 at 09:53:06PM +0200, Johannes Sixt wrote:

Show 29 quoted lines

> > Meanwhile, the async sideband demuxer will continue trying
> > to stream data from the remote repo until it gets EOF.
> > Depending on what data pack-objects actually sent, the
> > remote repo may not actually send anything (e.g., if we sent
> > nothing and it is simply waiting for the pack). This leads
> > to deadlock cycle in which send-pack waits on the demuxer,
> > the demuxer waits on the remote receive-pack, and the remote
> > receive-pack waits on send-pack to send the pack data.
> 
> This is an indication that a writable end of the pipe between send-pack and 
> receive-pack is still open. This fixes the deadlock for me without having to 
> kill the demuxer explicitly:
> 
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 5e772c7..db32ded 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -229,6 +229,9 @@ static void print_helper_status(struct ref *ref)
>  static int sideband_demux(int in, int out, void *data)
>  {
>  	int *fd = data;
> +#ifdef NO_PTHREADS
> +	close(fd[1]);
> +#endif
>  	int ret = recv_sideband("send-pack", fd[0], out);
>  	close(out);
>  	return ret;
> 
> If only I had a brilliant idea how to forge this into a re-usable pattern...

Thanks for finding that. I had the notion that there was a pipe end hanging open somewhere, but looking through the async code, I found us closing the pipes properly. But of course I failed to check the fds coming into send_pack.

Obviously it totally breaks the start_async abstraction if the called code needs to care whether it forked or not. But we can use that to our advantage, since it means start_async callers must assume the interface is very limited. So I think we can do something like:

  1. Async code declares which file descriptors it cares about. This
     would automatically include the pipe we give to it, of course.
     So the declared ones for a sideband demuxer would be stderr, and
     some network fd for reading.

  2. In the pthreads case, we do nothing. In the forked case, the child
     closes every descriptor except the "interesting" ones.

And that solves this problem, and the general case that async-callers have no idea if they have just leaked pipe descriptors in the forked case.

I'm still slightly confused, though, because I never see that descriptor get closed in the threaded case. So I still don't understand why it _doesn't_ deadlock with pthreads.

-Peff