From: Jeff King <peff@peff.net>
Date: Mon, 27 Jun 2016 21:49:48 GMT
Subject: Re: [PATCH 1/2] xread: retry after poll on EAGAIN/EWOULDBLOCK
Message-ID: <20160627214947.GA17149@sigill.intra.peff.net>
In-Reply-To: <20160627201311.GA7039@dcvr.yhbt.net>

On Mon, Jun 27, 2016 at 08:13:11PM +0000, Eric Wong wrote:

> > Quite a while ago, when I started doing code reviews professionally, I wondered
> > if the code review procedure can be semi-automated, as automation helps keeping
> > the error rate low. By that I mean having a check list which I can
> > check off each point
> 
> Maybe a test case or even a small unit test would've helped.
> I didn't notice the problem in xread until:
> 
> 1) I copied the code into xwrite
> 2) s/POLLIN/POLLOUT/;
> 3) forced EAGAIN using a patched, home-baked HTTP server
> 
> The biggish comment before the poll() obscured the missing
> "continue" for me.  I read xread() before and did not notice
> the missing "continue".

Here's an easier reproduction:

	nonblock () {
		perl -e '
			use Fcntl;
			fcntl(STDIN, F_SETFL, Fcntl::O_NONBLOCK);
			exec @ARGV;
		' "$@"
	}
	{
	  sleep 1
	  git pack-objects --all --stdout </dev/null
	} | nonblock git index-pack --stdin

Or even simpler:

        sleep 1 | nonblock git index-pack --stdin

The first one is nicer because it shows in the working case that we
actually do eventually read the input, as opposed to just getting EOF.
Prior to v2.8.0, or current master with your patch applied, it works
fine.

It turned out to be harder than I thought to find somebody who calls
xread() on stdin.  My first attempt was:

	{
		printf 'HE'
		sleep 1
		printf 'AD\n'
	} |
	nonblock git cat-file --batch-check

but this ends up in strbuf_getwholeline(), which uses getdelim(). Much to
my disappointment, getdelim() does not handle this case transparently;
it will quietly return the first two bytes (though with errno set to
EAGAIN), and we process bogus input.

So in general I would say that handing non-blocking descriptors to git
is not safe. I think it's possible to loop on getdelim() when we see
EAGAIN, but I'm not sure if it's worth it.

> Maybe the following optional patch on top of this series
> improves readability:
> 
> ----------8<--------
> Subject: [PATCH 3/2] hoist out io_wait function for xread and xwrite
> 
> At least for me, this improves the readability of xread and
> xwrite; hopefully allowing missing "continue" statements to
> be spotted more easily.

IMHO the end result is much nicer with this patch.

-Peff