Re: [PATCH 1/2] xread: retry after poll on EAGAIN/EWOULDBLOCK

From: Jeff King <peff@peff.net>
Date: Jun 27, 2016, 21:49 UTC
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:

Show 15 quoted lines

> > 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.

Show 9 quoted lines

> 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