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

From: Jeff King <peff@peff.net>
Date: Jun 27, 2016, 22:22 UTC
Message-ID: <20160627222238.GA23645@sigill.intra.peff.net>
In-Reply-To: <20160627214947.GA17149@sigill.intra.peff.net>

On Mon, Jun 27, 2016 at 05:49:48PM -0400, Jeff King wrote:

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

The patch for that is below, for the curious. It works with even this:

  {
    for i in H E A D; do
      sleep 1
      printf $i
    done
    printf "\n"
  } | nonblock git cat-file --batch-check

Note that I folded the "did we see EAGAIN" check into my sub-function (which is the equivalent of your io_wait). You might want to do that in the xread() code path, too, as it makes the resulting code there a very nice:

  if (errno == EINTR)
	continue;
  if (handle_nonblock(fd, POLLIN))
	continue;

---

diff --git a/strbuf.c b/strbuf.c
index 1ba600b..2147873 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -452,6 +452,38 @@ int strbuf_getcwd(struct strbuf *sb)
 	return -1;
 }
 
+int handle_nonblock(FILE *fp, short poll_events)
+{
+	if (ferror(fp) && (errno == EAGAIN || errno == EWOULDBLOCK)) {
+		struct pollfd pfd;
+
+		pfd.events = poll_events;
+		pfd.fd = fileno(fp);
+		poll(&pfd, 1, -1);
+		clearerr(fp);
+		return 1;
+	}
+	return 0;
+}
+
+static int getline_stdio_loop(struct strbuf *sb, FILE *fp, int term)
+{
+	int ch;
+	do {
+		errno = 0;
+		flockfile(fp);
+		while ((ch = getc_unlocked(fp)) != EOF) {
+			if (!strbuf_avail(sb))
+				strbuf_grow(sb, 1);
+			sb->buf[sb->len++] = ch;
+			if (ch == term)
+				break;
+		}
+		funlockfile(fp);
+	} while (handle_nonblock(fp, POLLIN));
+	return ch;
+}
+
 #ifdef HAVE_GETDELIM
 int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 {
@@ -465,12 +497,31 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	/* Translate slopbuf to NULL, as we cannot call realloc on it */
 	if (!sb->alloc)
 		sb->buf = NULL;
+
+	errno = 0;
 	r = getdelim(&sb->buf, &sb->alloc, term, fp);
 
 	if (r > 0) {
 		sb->len = r;
-		return 0;
 	}
+
+	/*
+	 * getdelim will return characters to us even if it sees EAGAIN;
+	 * we want to read all the way to an actual delimiter or EOF.
+	 *
+	 * We can't just loop on getdelim, though, as it wants to write
+	 * the whole buffer itself. So fall back to a stdio loop, which
+	 * can incrementally add to our strbuf.
+	 */
+	if (handle_nonblock(fp, POLLIN)) {
+		getline_stdio_loop(sb, fp, term);
+		/* Propagate real errors */
+		if (ferror(fp))
+			r = -1;
+	}
+	if (r > 0)
+		return 0;
+
 	assert(r == -1);
 
 	/*
@@ -507,15 +558,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 		return EOF;
 
 	strbuf_reset(sb);
-	flockfile(fp);
-	while ((ch = getc_unlocked(fp)) != EOF) {
-		if (!strbuf_avail(sb))
-			strbuf_grow(sb, 1);
-		sb->buf[sb->len++] = ch;
-		if (ch == term)
-			break;
-	}
-	funlockfile(fp);
+	ch = getline_stdio_loop(sb, fp, term);
 	if (ch == EOF && sb->len == 0)
 		return EOF;