From: Jeff King <peff@peff.net>
To: Eric Wong <e@80x24.org>
Cc: Stefan Beller <sbeller@google.com>,
Junio C Hamano <gitster@pobox.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
Johannes Sixt <j6t@kdbg.org>
Subject: Re: [PATCH 1/2] xread: retry after poll on EAGAIN/EWOULDBLOCK
Date: Mon, 27 Jun 2016 18:22:38 -0400 [thread overview]
Message-ID: <20160627222238.GA23645@sigill.intra.peff.net> (raw)
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;
next prev parent reply other threads:[~2016-06-27 22:22 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-26 23:21 [PATCH 0/2] wrapper: xread/xwrite fixes for non-blocking FDs Eric Wong
2016-06-26 23:21 ` [PATCH 1/2] xread: retry after poll on EAGAIN/EWOULDBLOCK Eric Wong
2016-06-26 23:42 ` Jeff King
2016-06-27 13:02 ` Junio C Hamano
2016-06-27 14:36 ` Jeff King
2016-06-27 16:49 ` Stefan Beller
2016-06-27 19:17 ` Jeff King
2016-06-27 19:43 ` Stefan Beller
2016-06-27 19:51 ` Jeff King
2016-06-27 20:13 ` Eric Wong
2016-06-27 21:49 ` Jeff King
2016-06-27 22:22 ` Jeff King [this message]
2016-06-27 23:19 ` Eric Wong
2016-06-26 23:51 ` Jeff King
2016-06-27 3:56 ` [PATCHv2 " Eric Wong
2016-06-26 23:21 ` [PATCH 2/2] xwrite: poll on non-blocking FDs Eric Wong
2016-06-26 23:49 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160627222238.GA23645@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=e@80x24.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).