* [PATCH 1/2] git: set non-blocking flag in case of other bugs
2020-11-29 10:52 5% [PATCH 0/2] git: some safety fixes Eric Wong
@ 2020-11-29 10:52 7% ` Eric Wong
0 siblings, 0 replies; 2+ results
From: Eric Wong @ 2020-11-29 10:52 UTC (permalink / raw)
To: meta
This makes GitAsyncCat more resilient to bugs in Gcf2 or even
git-cat-file itself. I noticed -imapd stuck on read(2) from
the Gcf2 pipe, so there may be a bug somewhere in Gcf2 or
PublicInbox::Git. This should make us more resilient to them
and hopefully help us notice and fix them.
---
lib/PublicInbox/Git.pm | 28 +++++++++++++++++++++-------
lib/PublicInbox/GitAsyncCat.pm | 6 +-----
2 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 917fa4a1..d53427d7 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -12,17 +12,19 @@ use v5.10.1;
use parent qw(Exporter);
use POSIX ();
use IO::Handle; # ->autoflush
-use Errno qw(EINTR);
+use Errno qw(EINTR EAGAIN);
use File::Glob qw(bsd_glob GLOB_NOSORT);
use File::Spec ();
use Time::HiRes qw(stat);
use PublicInbox::Spawn qw(popen_rd);
use PublicInbox::Tmpfile;
+use IO::Poll qw(POLLIN);
use Carp qw(croak);
use Digest::SHA ();
our @EXPORT_OK = qw(git_unquote git_quote);
our $PIPE_BUFSIZ = 65536; # Linux default
our $in_cleanup;
+our $RDTIMEO = 60_000; # milliseconds
use constant MAX_INFLIGHT =>
(($^O eq 'linux' ? 4096 : POSIX::_POSIX_PIPE_BUF()) * 3)
@@ -132,6 +134,8 @@ sub _bidi_pipe {
$self->{$in} = $in_r;
}
+sub poll_in ($) { IO::Poll::_poll($RDTIMEO, fileno($_[0]), my $ev = POLLIN) }
+
sub my_read ($$$) {
my ($fh, $rbuf, $len) = @_;
my $left = $len - length($$rbuf);
@@ -140,9 +144,12 @@ sub my_read ($$$) {
$r = sysread($fh, $$rbuf, $PIPE_BUFSIZ, length($$rbuf));
if ($r) {
$left -= $r;
+ } elsif (defined($r)) { # EOF
+ return 0;
} else {
- next if (!defined($r) && $! == EINTR);
- return $r;
+ next if ($! == EAGAIN and poll_in($fh));
+ next if $! == EINTR; # may be set by sysread or poll_in
+ return; # unrecoverable error
}
}
\substr($$rbuf, 0, $len, '');
@@ -154,9 +161,15 @@ sub my_readline ($$) {
if ((my $n = index($$rbuf, "\n")) >= 0) {
return substr($$rbuf, 0, $n + 1, '');
}
- my $r = sysread($fh, $$rbuf, $PIPE_BUFSIZ, length($$rbuf));
- next if $r || (!defined($r) && $! == EINTR);
- return defined($r) ? '' : undef; # EOF or error
+ my $r = sysread($fh, $$rbuf, $PIPE_BUFSIZ, length($$rbuf))
+ and next;
+
+ # return whatever's left on EOF
+ return substr($$rbuf, 0, length($$rbuf)+1, '') if defined($r);
+
+ next if ($! == EAGAIN and poll_in($fh));
+ next if $! == EINTR; # may be set by sysread or poll_in
+ return; # unrecoverable error
}
}
@@ -204,7 +217,8 @@ sub cat_async_step ($$) {
$type = 'missing';
$oid = ref($req) ? $$req : $req if $oid eq '';
} else {
- $self->fail("Unexpected result from async git cat-file: $head");
+ my $err = $! ? " ($!)" : '';
+ $self->fail("bad result from async cat-file: $head$err");
}
$self->{cat_rbuf} = $rbuf if $$rbuf ne '';
eval { $cb->($bref, $oid, $type, $size, $arg) };
diff --git a/lib/PublicInbox/GitAsyncCat.pm b/lib/PublicInbox/GitAsyncCat.pm
index be51f673..dc97af16 100644
--- a/lib/PublicInbox/GitAsyncCat.pm
+++ b/lib/PublicInbox/GitAsyncCat.pm
@@ -3,11 +3,6 @@
#
# internal class used by PublicInbox::Git + PublicInbox::DS
# This parses the output pipe of "git cat-file --batch"
-#
-# Note: this does NOT set the non-blocking flag, we expect `git cat-file'
-# to be a local process, and git won't start writing a blob until it's
-# fully read. So minimize context switching and read as much as possible
-# and avoid holding a buffer in our heap any longer than it has to live.
package PublicInbox::GitAsyncCat;
use strict;
use parent qw(PublicInbox::DS Exporter);
@@ -69,6 +64,7 @@ sub git_async_cat ($$$$) {
$gitish->{async_cat} //= do {
# read-only end of pipe (Gcf2Client is write-only end)
my $self = bless { gitish => $gitish }, __PACKAGE__;
+ $gitish->{in}->blocking(0);
$self->SUPER::new($gitish->{in}, EPOLLIN|EPOLLET);
\undef; # this is a true ref()
};
^ permalink raw reply related [relevance 7%]
* [PATCH 0/2] git: some safety fixes
@ 2020-11-29 10:52 5% Eric Wong
2020-11-29 10:52 7% ` [PATCH 1/2] git: set non-blocking flag in case of other bugs Eric Wong
0 siblings, 1 reply; 2+ results
From: Eric Wong @ 2020-11-29 10:52 UTC (permalink / raw)
To: meta
I noticed -imapd on public-inbox.org was stuck reading on the
Gcf2 pipe; so patch 1/2 should allow us to recover within 60s if
it happens again.
And 2/2 makes the code easier to follow/modify in case we do
hit failures, since we need to ensure we call DS::close in the
daemons.
Not sure what caused that read(2) to get stuck, I reread the
partial writev calculations in lib/PublicInbox/gcf2_libgit2.h
and it all seems to make sense to me. I've also written and
tested similar code for writev/sendmmsg many times over the
years in other projects...
Eric Wong (2):
git: set non-blocking flag in case of other bugs
git: ensure subclassed ->fail gets called
lib/PublicInbox/Git.pm | 52 +++++++++++++++++++++-------------
lib/PublicInbox/GitAsyncCat.pm | 6 +---
2 files changed, 34 insertions(+), 24 deletions(-)
^ permalink raw reply [relevance 5%]
Results 1-2 of 2 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2020-11-29 10:52 5% [PATCH 0/2] git: some safety fixes Eric Wong
2020-11-29 10:52 7% ` [PATCH 1/2] git: set non-blocking flag in case of other bugs Eric Wong
Code repositories for project(s) associated with this public inbox
https://80x24.org/public-inbox.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).