[PATCH 00/13] misc error handling stuff and simplifications

[PATCH 00/13] misc error handling stuff and simplifications

[Eric Wong]

1-4 were things I noticed while chasing the lei SIGPIPE handling
fix.  5-7 are things I noticed while testing on Dragonfly and
NetBSD.  8 was noticed randomly while working on a new mirror,
and the last few complete the work which let me get rid of tied
IO handles in favor of using the PublicInbox::IO subclass.

Eric Wong (13):
  lei_xsearch: put query in process title for debugging
  lei: use cached $daemon_pid when possible
  lei: reuse FDs atfork and close explicitly
  lei_up: use v5.12
  net_nntp_socks: more comments around how it works
  lei ls-mail-source: gracefully handle network failures
  net: retry on EINTR and check for {quit} flag
  lei_mirror: note missing local manifests are non-fatal
  ipc: simplify partial sendmsg fallback
  lei_input: always close single `eml' inputs
  xapcmd: get rid of scalar wantarray popen_rd
  lei: get rid of autoreap usage
  spawn: get rid of wantarray popen_rd/popen_wr

 lib/PublicInbox/IPC.pm             | 13 ++------
 lib/PublicInbox/LEI.pm             | 11 ++++---
 lib/PublicInbox/LeiInput.pm        | 26 +++++++--------
 lib/PublicInbox/LeiLsMailSource.pm |  6 ++--
 lib/PublicInbox/LeiMirror.pm       |  5 +--
 lib/PublicInbox/LeiRemote.pm       | 14 ++++----
 lib/PublicInbox/LeiUp.pm           | 10 +++---
 lib/PublicInbox/LeiXSearch.pm      | 27 ++++++++-------
 lib/PublicInbox/NetNNTPSocks.pm    | 12 ++++---
 lib/PublicInbox/NetReader.pm       | 53 +++++++++++++++++++++---------
 lib/PublicInbox/Spawn.pm           |  6 ++--
 lib/PublicInbox/TestCommon.pm      | 23 ++++++++++++-
 lib/PublicInbox/Watch.pm           |  2 +-
 lib/PublicInbox/Xapcmd.pm          | 12 +++----
 t/io.t                             |  8 +----
 t/ipc.t                            |  7 ++++
 t/lei-import.t                     | 27 +++++++++++++++
 17 files changed, 163 insertions(+), 99 deletions(-)

[PATCH 01/13] lei_xsearch: put query in process title for debugging

[Eric Wong]

Having queries in the process titles makes it easier to diagnose
stuck queries due to IPC problems.  This was used to diagnose
commit e97a30e7624d (lei: fix SIGPIPE on large result sets to pager)).
---
 lib/PublicInbox/LeiXSearch.pm | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index 6c8dfe10..ba8ff293 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -148,13 +148,13 @@ sub mset_progress {
 
 sub query_one_mset { # for --threads and l2m w/o sort
 	my ($self, $ibxish) = @_;
-	local $0 = "$0 query_one_mset";
 	my $lei = $self->{lei};
 	my ($srch, $over) = ($ibxish->search, $ibxish->over);
 	my $dir = $ibxish->{inboxdir} // $ibxish->{topdir};
 	return warn("$dir not indexed by Xapian\n") unless ($srch && $over);
 	bless $srch, 'PublicInbox::LeiSearch'; # for ->qparse_new
 	my $mo = { %{$lei->{mset_opt}} }; # copy
+	local $0 = "$0 1 $mo->{qstr}";
 	my $mset;
 	my $each_smsg = $lei->{ovv}->ovv_each_smsg_cb($lei);
 	my $can_kw = !!$ibxish->can('msg_keywords');
@@ -219,9 +219,9 @@ sub query_one_mset { # for --threads and l2m w/o sort
 
 sub query_combined_mset { # non-parallel for non-"--threads" users
 	my ($self) = @_;
-	local $0 = "$0 query_combined_mset";
 	my $lei = $self->{lei};
 	my $mo = { %{$lei->{mset_opt}} };
+	local $0 = "$0 C $mo->{qstr}";
 	my $mset;
 	for my $loc (locals($self)) {
 		attach_external($self, $loc);
@@ -312,12 +312,11 @@ sub fudge_qstr_time ($$$) {
 
 sub query_remote_mboxrd {
 	my ($self, $uris) = @_;
-	local $0 = "$0 query_remote_mboxrd";
 	local $SIG{TERM} = sub { exit(0) }; # for DESTROY (File::Temp, $reap)
 	my $lei = $self->{lei};
 	my $opt = $lei->{opt};
-	chomp(my $qstr = $lei->{mset_opt}->{qstr});
-	$qstr =~ s/[ \n\t]+/ /sg; # make URLs less ugly
+	my $qstr = $lei->{mset_opt}->{qstr};
+	local $0 = "$0 R $qstr";
 	my @qform = (x => 'm');
 	push(@qform, t => 1) if $opt->{threads};
 	open my $cerr, '+>', undef;
@@ -504,6 +503,9 @@ sub ipc_atfork_child {
 sub do_query {
 	my ($self, $lei) = @_;
 	my $l2m = $lei->{l2m};
+	my $qstr = \($lei->{mset_opt}->{qstr});
+	chomp $$qstr;
+	$$qstr =~ s/[ \n\t]+/ /sg; # make URLs and $0 less ugly
 	my $ops = {
 		sigpipe_handler => [ $lei ],
 		fail_handler => [ $lei ],

[PATCH 02/13] lei: use cached $daemon_pid when possible

[Eric Wong]

->lei_daemon_pid can only be called in the top-level daemon
process when $daemon_pid is valid, so avoid a getpid(2) syscall
in those cases.
---
 lib/PublicInbox/LEI.pm   | 2 +-
 lib/PublicInbox/LeiUp.pm | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 2832db63..f32e5bbc 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -927,7 +927,7 @@ sub _config {
 	run_wait($cmd, \%env, \%opt) ? ($err_ok ? undef : fail($self, $?)) : 1;
 }
 
-sub lei_daemon_pid { puts shift, $$ }
+sub lei_daemon_pid { puts shift, $daemon_pid }
 
 sub lei_daemon_kill {
 	my ($self) = @_;
diff --git a/lib/PublicInbox/LeiUp.pm b/lib/PublicInbox/LeiUp.pm
index cd2337b4..0faa180d 100644
--- a/lib/PublicInbox/LeiUp.pm
+++ b/lib/PublicInbox/LeiUp.pm
@@ -11,6 +11,7 @@ use PublicInbox::LeiSavedSearch; # OverIdx
 use PublicInbox::DS;
 use PublicInbox::PktOp;
 use PublicInbox::LeiFinmsg;
+use PublicInbox::LEI;
 my $REMOTE_RE = qr!\A(?:imap|http)s?://!i; # http(s) will be for JMAP
 
 sub up1 ($$) {
@@ -92,7 +93,6 @@ sub redispatch_all ($$) {
 	$op_c->{ops} = { '' => [ $lei->can('dclose'), $lei ] };
 	my @first_batch = splice(@$upq, 0, $j); # initial parallelism
 	$lei->{-upq} = $upq;
-	$lei->{daemon_pid} = $$;
 	$lei->event_step_init; # wait for client disconnects
 	for my $out (@first_batch) {
 		PublicInbox::DS::requeue(
@@ -212,8 +212,8 @@ sub event_step { # runs via PublicInbox::DS::requeue
 
 sub DESTROY {
 	my ($self) = @_;
+	return if ($PublicInbox::LEI::daemon_pid // -1) != $$;
 	my $lei = $self->{lei}; # the original, from lei_up
-	return if $lei->{daemon_pid} != $$;
 	my $sock = delete $self->{unref_on_destroy};
 	my $s = $lei->{-socks} // [];
 	@$s = grep { $_ != $sock } @$s;

[PATCH 03/13] lei: reuse FDs atfork and close explicitly

[Eric Wong]

We'll avoid having a redundant STDERR FD open in lei workers,
and some explicit close() on `lei up' sockets reduces the
likelyhood of inadvertantly open FDs causing processes to
linger.
---
 lib/PublicInbox/LEI.pm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index f32e5bbc..681044c8 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -574,19 +574,20 @@ sub _lei_atfork_child {
 	my ($self, $persist) = @_;
 	# we need to explicitly close things which are on stack
 	my $cfg = $self->{cfg};
+	delete @$cfg{qw(-watches -lei_note_event)};
 	if ($persist) {
 		open $self->{3}, '<', '/';
 		fchdir($self);
 		close($_) for (grep(defined, delete @$self{qw(0 1 2 sock)}));
-		delete @$cfg{qw(-lei_store -watches -lei_note_event)};
+		delete $cfg->{-lei_store};
 	} else { # worker, Net::NNTP (Net::Cmd) uses STDERR directly
-		open STDERR, '+>&='.fileno($self->{2});
+		open STDERR, '+>&', $self->{2};
 		STDERR->autoflush(1);
+		$self->{2} = \*STDERR;
 		POSIX::setpgid(0, $$) // die "setpgid(0, $$): $!";
-		delete @$cfg{qw(-watches -lei_note_event)};
 	}
 	close($_) for (grep(defined, delete @$self{qw(old_1 au_done)}));
-	delete $self->{-socks};
+	close($_) for (@{delete($self->{-socks}) // []});
 	if (my $op_c = delete $self->{pkt_op_c}) {
 		close(delete $op_c->{sock});
 	}

[PATCH 04/13] lei_up: use v5.12

[Eric Wong]

No unicode_strings dependencies here, AFAIK
---
 lib/PublicInbox/LeiUp.pm | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/LeiUp.pm b/lib/PublicInbox/LeiUp.pm
index 0faa180d..9931f017 100644
--- a/lib/PublicInbox/LeiUp.pm
+++ b/lib/PublicInbox/LeiUp.pm
@@ -3,8 +3,7 @@
 
 # "lei up" - updates the result of "lei q --save"
 package PublicInbox::LeiUp;
-use strict;
-use v5.10.1;
+use v5.12;
 # n.b. we use LeiInput to setup IMAP auth
 use parent qw(PublicInbox::IPC PublicInbox::LeiInput);
 use PublicInbox::LeiSavedSearch; # OverIdx
@@ -174,8 +173,7 @@ no warnings 'once';
 *ipc_atfork_child = \&PublicInbox::LeiInput::input_only_atfork_child;
 
 package PublicInbox::LeiUp1; # for redispatch_all
-use strict;
-use v5.10.1;
+use v5.12;
 
 sub nxt ($$$) {
 	my ($lei, $out, $op_p) = @_;

[PATCH 05/13] net_nntp_socks: more comments around how it works

[Eric Wong]

This is convoluted as hell but I can't figure out a better way
to make Net::NNTP work with SOCKS.
---
 lib/PublicInbox/NetNNTPSocks.pm | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/PublicInbox/NetNNTPSocks.pm b/lib/PublicInbox/NetNNTPSocks.pm
index 5b15dd59..306dcacb 100644
--- a/lib/PublicInbox/NetNNTPSocks.pm
+++ b/lib/PublicInbox/NetNNTPSocks.pm
@@ -1,13 +1,13 @@
 # Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 
-# wrap Net::NNTP client with SOCKS support
+# wrap Net::NNTP client with SOCKS support.  Convoluted, but AFAIK this
+# is the only way to get SOCKS working with Net::NNTP w/o LD_PRELOAD.
 package PublicInbox::NetNNTPSocks;
 use v5.12;
 use Net::NNTP;
-our %OPT;
+our %OPT; # used to pass options between ->new_socks and our ->new
 our @ISA = qw(IO::Socket::Socks);
-my @SOCKS_KEYS = qw(ProxyAddr ProxyPort SocksVersion SocksDebug SocksResolve);
 
 # use this instead of Net::NNTP->new if using Proxy*
 sub new_socks {
@@ -16,9 +16,10 @@ sub new_socks {
 	local @Net::NNTP::ISA = (qw(Net::Cmd), __PACKAGE__);
 	local %OPT = map {;
 		defined($opt{$_}) ? ($_ => $opt{$_}) : ()
-	} @SOCKS_KEYS;
+	} qw(ProxyAddr ProxyPort SocksVersion SocksDebug SocksResolve);
 	no warnings 'uninitialized'; # needed for $SOCKS_ERROR
-	Net::NNTP->new(%opt) // die "errors: \$!=$! SOCKS=",
+	my $ret = Net::NNTP->new(%opt); # calls PublicInbox::NetNNTPSocks::new
+	$ret // die "errors: \$!=$! SOCKS=",
 				eval('$IO::Socket::Socks::SOCKS_ERROR // ""'),
 				', SSL=',
 				(eval('IO::Socket::SSL->errstr')  // ''), "\n";

[PATCH 06/13] lei ls-mail-source: gracefully handle network failures

[Eric Wong]

All network connections may fail, so try to emit a helpful
error message instead of attempting to dispatch methods off
`undef'.
---
 lib/PublicInbox/LeiLsMailSource.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/LeiLsMailSource.pm b/lib/PublicInbox/LeiLsMailSource.pm
index 50799270..4b427b26 100644
--- a/lib/PublicInbox/LeiLsMailSource.pm
+++ b/lib/PublicInbox/LeiLsMailSource.pm
@@ -19,7 +19,8 @@ sub input_path_url { # overrides LeiInput version
 	if ($url =~ m!\Aimaps?://!i) {
 		my $uri = PublicInbox::URIimap->new($url);
 		my $sec = $lei->{net}->can('uri_section')->($uri);
-		my $mic = $lei->{net}->mic_get($uri);
+		my $mic = $lei->{net}->mic_get($uri) or
+			return $lei->err("E: $uri");
 		my $l = $mic->folders_hash($uri->path); # server-side filter
 		@$l = map { $_->[2] } # undo Schwartzian transform below:
 			sort { $a->[0] cmp $b->[0] || $a->[1] <=> $b->[1] }
@@ -39,7 +40,8 @@ sub input_path_url { # overrides LeiInput version
 		}
 	} elsif ($url =~ m!\A(?:nntps?|s?news)://!i) {
 		my $uri = PublicInbox::URInntps->new($url);
-		my $nn = $lei->{net}->nn_get($uri);
+		my $nn = $lei->{net}->nn_get($uri) or
+			return $lei->err("E: $uri");
 		my $l = $nn->newsgroups($uri->group); # name => description
 		my $sec = $lei->{net}->can('uri_section')->($uri);
 		if ($json) {

[PATCH 07/13] net: retry on EINTR and check for {quit} flag

[Eric Wong]

This should allow us to detect shutdown signals in -watch
more quickly and not unnecessarily fail on inconsequential
signals such as SIGWINCH.
---
 lib/PublicInbox/NetNNTPSocks.pm |  1 +
 lib/PublicInbox/NetReader.pm    | 53 +++++++++++++++++++++++----------
 lib/PublicInbox/Watch.pm        |  2 +-
 3 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/lib/PublicInbox/NetNNTPSocks.pm b/lib/PublicInbox/NetNNTPSocks.pm
index 306dcacb..d27efba1 100644
--- a/lib/PublicInbox/NetNNTPSocks.pm
+++ b/lib/PublicInbox/NetNNTPSocks.pm
@@ -19,6 +19,7 @@ sub new_socks {
 	} qw(ProxyAddr ProxyPort SocksVersion SocksDebug SocksResolve);
 	no warnings 'uninitialized'; # needed for $SOCKS_ERROR
 	my $ret = Net::NNTP->new(%opt); # calls PublicInbox::NetNNTPSocks::new
+	return $ret if $ret || $!{EINTR};
 	$ret // die "errors: \$!=$! SOCKS=",
 				eval('$IO::Socket::Socks::SOCKS_ERROR // ""'),
 				', SSL=',
diff --git a/lib/PublicInbox/NetReader.pm b/lib/PublicInbox/NetReader.pm
index 95cf1de8..e3e5d596 100644
--- a/lib/PublicInbox/NetReader.pm
+++ b/lib/PublicInbox/NetReader.pm
@@ -60,6 +60,7 @@ sub mic_new ($$$$) {
 	my ($self, $mic_arg, $sec, $uri) = @_;
 	my %mic_arg = (%$mic_arg, Keepalive => 1);
 	my $sa = $self->{cfg_opt}->{$sec}->{-proxy_cfg} || $self->{-proxy_cli};
+	my ($mic, $s, $t);
 	if ($sa) {
 		# this `require' needed for worker[1..Inf], since socks_args
 		# only got called in worker[0]
@@ -68,24 +69,37 @@ sub mic_new ($$$$) {
 		$opt{SocksDebug} = 1 if $mic_arg{Debug};
 		$opt{ConnectAddr} = delete $mic_arg{Server};
 		$opt{ConnectPort} = delete $mic_arg{Port};
-		my $s = IO::Socket::Socks->new(%opt) or die
-			"E: <$uri> ".eval('$IO::Socket::Socks::SOCKS_ERROR');
+		do {
+			$! = 0;
+			$s = IO::Socket::Socks->new(%opt);
+		} until ($s || !$!{EINTR} || $self->{quit});
+		return if $self->{quit};
+		$s or die "E: <$uri> ".eval('$IO::Socket::Socks::SOCKS_ERROR');
+		$mic_arg{Socket} = $s;
 		if (my $o = delete $mic_arg{Ssl}) { # for imaps://
 			$o = mic_tls_opt($o, $opt{ConnectAddr});
-			$s = IO::Socket::SSL->start_SSL($s, @$o) or die
-				"E: <$uri> ".(IO::Socket::SSL->errstr // '');
+			do {
+				$! = 0;
+				$t = IO::Socket::SSL->start_SSL($s, @$o);
+			} until ($t || !$!{EINTR} || $self->{quit});
+			return if $self->{quit};
+			$t or die "E: <$uri> ".(IO::Socket::SSL->errstr // '');
+			$mic_arg{Socket} = $t;
 		} elsif ($o = $mic_arg{Starttls}) {
 			# Mail::IMAPClient will use this:
 			$mic_arg{Starttls} = mic_tls_opt($o, $opt{ConnectAddr});
 		}
-		$mic_arg{Socket} = $s;
 	} elsif ($mic_arg{Ssl} || $mic_arg{Starttls}) {
 		for my $f (qw(Ssl Starttls)) {
 			my $o = $mic_arg{$f} or next;
 			$mic_arg{$f} = mic_tls_opt($o, $mic_arg{Server});
 		}
 	}
-	PublicInbox::IMAPClient->new(%mic_arg);
+	do {
+		$! = 0;
+		$mic = PublicInbox::IMAPClient->new(%mic_arg);
+	} until ($mic || !$!{EINTR} || $self->{quit});
+	$mic;
 }
 
 sub auth_anon_cb { '' }; # for Mail::IMAPClient::Authcallback
@@ -152,6 +166,7 @@ sub mic_for ($$$$) { # mic = Mail::IMAPClient
 	};
 	require PublicInbox::IMAPClient;
 	my $mic = mic_new($self, $mic_arg, $sec, $uri);
+	return if $self->{quit};
 	($mic && $mic->IsConnected) or
 		die "E: <$uri> new: $@".onion_hint($lei, $uri);
 
@@ -202,16 +217,20 @@ sub mic_for ($$$$) { # mic = Mail::IMAPClient
 	$mic;
 }
 
-sub nn_new ($$$) {
-	my ($nn_arg, $nntp_cfg, $uri) = @_;
+sub nn_new ($$$$) {
+	my ($self, $nn_arg, $nntp_cfg, $uri) = @_;
 	my $nn;
+	my ($Net_NNTP, $new) = qw(Net::NNTP new);
 	if (defined $nn_arg->{ProxyAddr}) {
 		require PublicInbox::NetNNTPSocks;
+		($Net_NNTP, $new) = qw(PublicInbox::NetNNTPSocks new_socks);
 		$nn_arg->{SocksDebug} = 1 if $nn_arg->{Debug};
-		$nn = PublicInbox::NetNNTPSocks->new_socks(%$nn_arg) or return;
-	} else {
-		$nn = Net::NNTP->new(%$nn_arg) or return;
 	}
+	do {
+		$! = 0;
+		$nn = $Net_NNTP->$new(%$nn_arg);
+	} until ($nn || !$!{EINTR} || $self->{quit});
+	$nn // return;
 	setsockopt($nn, Socket::SOL_SOCKET(), Socket::SO_KEEPALIVE(), 1);
 
 	# default to using STARTTLS if it's available, but allow
@@ -268,8 +287,9 @@ sub nn_for ($$$$) { # nn = Net::NNTP
 	$nn_arg->{SSL} = 1 if $uri->secure; # snews == nntps
 	my $sa = $self->{-proxy_cli};
 	%$nn_arg = (%$nn_arg, %$sa) if $sa;
-	my $nn = nn_new($nn_arg, $nntp_cfg, $uri) or
-		die "E: <$uri> new: $@".onion_hint($lei, $uri);
+	my $nn = nn_new($self, $nn_arg, $nntp_cfg, $uri);
+	return if $self->{quit};
+	$nn // die "E: <$uri> new: $@".onion_hint($lei, $uri);
 	if ($cred) {
 		$cred->fill($lei) unless defined($p); # may prompt user here
 		if ($nn->authinfo($u, $p)) {
@@ -382,8 +402,9 @@ sub imap_common_init ($;$) {
 		my $sec = uri_section($orig_uri);
 		my $uri = PublicInbox::URIimap->new("$sec/");
 		my $mic = $mics->{$sec} //=
-				mic_for($self, $uri, $mic_common, $lei) //
-				die "Unable to continue\n";
+				mic_for($self, $uri, $mic_common, $lei);
+		return if $self->{quit};
+		$mic // die "Unable to continue\n";
 		next unless $self->isa('PublicInbox::NetWriter');
 		next if $self->{-skip_creat};
 		my $dst = $orig_uri->mailbox // next;
@@ -751,7 +772,7 @@ sub nn_get {
 	my $nn_arg = $self->{net_arg}->{$sec} or
 			die "BUG: no Net::NNTP->new arg for $sec";
 	my $nntp_cfg = $self->{cfg_opt}->{$sec};
-	$nn = nn_new($nn_arg, $nntp_cfg, $uri) or return;
+	$nn = nn_new($self, $nn_arg, $nntp_cfg, $uri) or return;
 	if (my $postconn = $nntp_cfg->{-postconn}) {
 		for my $m_arg (@$postconn) {
 			my ($method, @args) = @$m_arg;
diff --git a/lib/PublicInbox/Watch.pm b/lib/PublicInbox/Watch.pm
index c2b1312a..1cdf12a5 100644
--- a/lib/PublicInbox/Watch.pm
+++ b/lib/PublicInbox/Watch.pm
@@ -494,7 +494,7 @@ sub poll_fetch_reap { # awaitpid callback
 
 sub watch_imap_init ($$) {
 	my ($self, $poll) = @_;
-	my $mics = PublicInbox::NetReader::imap_common_init($self);
+	my $mics = PublicInbox::NetReader::imap_common_init($self) or return;
 	my $idle = []; # [ uri1, intvl1, uri2, intvl2 ]
 	for my $uri (@{$self->{imap_order}}) {
 		my $sec = uri_section($uri);

[PATCH 08/13] lei_mirror: note missing local manifests are non-fatal

[Eric Wong]

Sometimes seeing that warning is alarming.
---
 lib/PublicInbox/LeiMirror.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 49febe9e..84266d03 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -950,10 +950,11 @@ sub load_current_manifest ($) {
 	if (CORE::open(my $fh, '<', $fn)) {
 		decode_manifest($fh, $fn, $fn);
 	} elsif ($!{ENOENT}) { # non-fatal, we can just do it slowly
-		warn "open($fn): $!\n" if !$self->{-initial_clone};
+		$self->{-initial_clone} or
+			warn "W: open($fn): $! (non-fatal)\n";
 		undef;
 	} else {
-		die "open($fn): $!\n";
+		die "E: open($fn): $!\n";
 	}
 }
 

[PATCH 09/13] ipc: simplify partial sendmsg fallback

[Eric Wong]

In the rare case sendmsg(2) isn't able to send the full amount
(due to buffers >=2GB on Linux), use print + (autodie)close
to send the remainder and retry on EINTR.  `substr' should be
able to avoid a large malloc via offsets and CoW on modern Perl.
---
 lib/PublicInbox/IPC.pm | 13 +++----------
 t/ipc.t                |  7 +++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/PublicInbox/IPC.pm b/lib/PublicInbox/IPC.pm
index 3292d960..a5cae6f2 100644
--- a/lib/PublicInbox/IPC.pm
+++ b/lib/PublicInbox/IPC.pm
@@ -10,7 +10,7 @@
 package PublicInbox::IPC;
 use v5.12;
 use parent qw(Exporter);
-use autodie qw(fork pipe read socketpair sysread);
+use autodie qw(close fork pipe read socketpair sysread);
 use Carp qw(croak);
 use PublicInbox::DS qw(awaitpid);
 use PublicInbox::Spawn;
@@ -266,15 +266,8 @@ sub stream_in_full ($$$) {
 			0) // croak "sendmsg: $!";
 	undef $r;
 	$n = $send_cmd->($w, $fds, $buf, 0) // croak "sendmsg: $!";
-	while ($n < length($buf)) {
-		my $x = syswrite($w, $buf, length($buf) - $n, $n);
-		if (!defined($n)) {
-			next if $!{EINTR};
-			croak "syswrite: $!";
-		}
-		$x or croak "syswrite wrote 0 bytes";
-		$n += $x;
-	}
+	print $w substr($buf, $n) if $n < length($buf); # need > 2G on Linux
+	close $w; # autodies
 }
 
 sub wq_io_do { # always async
diff --git a/t/ipc.t b/t/ipc.t
index 519ef089..23ae2e7b 100644
--- a/t/ipc.t
+++ b/t/ipc.t
@@ -132,6 +132,13 @@ for my $t ('worker', 'worker again') {
 		$exp = sha1_hex($bigger)."\n";
 		is(readline($rb), $exp, "SHA WQWorker limit ($t)");
 	}
+	SKIP: {
+		$ENV{TEST_EXPENSIVE} or skip 'TEST_EXPENSIVE not set', 1;
+		my $bigger = $big x 75000; # over 2G to trigger partial sendmsg
+		$ipc->wq_io_do('test_sha', [ $wa, $wb ], $bigger);
+		my $exp = sha1_hex($bigger)."\n";
+		is(readline($rb), $exp, "SHA WQWorker sendmsg limit ($t)");
+	}
 }
 
 # wq_io_do works across fork (siblings can feed)

[PATCH 10/13] lei_input: always close single `eml' inputs

[Eric Wong]

This matches the behavior we have for multi-message mbox files
since we rely on ->close to detect errors on bad mboxes.  This
ensures we'll notice errors reading single messages from stdin.

We'll also start relying more on strace error injection to test
error handling.
---
 lib/PublicInbox/LeiInput.pm   | 13 +++++++------
 lib/PublicInbox/TestCommon.pm | 23 ++++++++++++++++++++++-
 t/io.t                        |  8 +-------
 t/lei-import.t                | 27 +++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/lib/PublicInbox/LeiInput.pm b/lib/PublicInbox/LeiInput.pm
index 4cd18c09..adb356c9 100644
--- a/lib/PublicInbox/LeiInput.pm
+++ b/lib/PublicInbox/LeiInput.pm
@@ -81,9 +81,11 @@ sub input_fh {
 	my ($self, $ifmt, $fh, $name, @args) = @_;
 	if ($ifmt eq 'eml') {
 		my $buf = do { local $/; <$fh> };
-		(defined($buf) && eof($fh) && close($fh)) or
-			return $self->{lei}->child_error(0, <<"");
-error reading $name: $!
+		my $ok = defined($buf) ? 1 : 0;
+		++$ok if eof($fh);
+		++$ok if $fh->close;
+		$ok == 3 or return $self->{lei}->child_error($?, <<"");
+error reading $name: $! (\$?=$?)
 
 		PublicInbox::Eml::strip_from($buf);
 
@@ -246,9 +248,8 @@ sub input_path_url {
 			my $rdr = { 2 => $lei->{2} };
 			my $fh = popen_rd($fp, undef, $rdr);
 			eval { $self->input_fh('eml', $fh, $input, @args) };
-			my @err = ($@ ? $@ : ());
-			$fh->close or push @err, "\$?=$?";
-			$lei->child_error($?, "@$fp failed: @err") if @err;
+			my $err = $@ ? ": $@" : '';
+			$lei->child_error($?, "@$fp failed$err") if $err || $?;
 		} else {
 			$self->folder_missing("$ifmt:$input");
 		}
diff --git a/lib/PublicInbox/TestCommon.pm b/lib/PublicInbox/TestCommon.pm
index 83e99b42..46e6a538 100644
--- a/lib/PublicInbox/TestCommon.pm
+++ b/lib/PublicInbox/TestCommon.pm
@@ -28,7 +28,8 @@ BEGIN {
 		quit_waiter_pipe wait_for_eof
 		tcp_host_port test_lei lei lei_ok $lei_out $lei_err $lei_opt
 		test_httpd xbail require_cmd is_xdeeply tail_f
-		ignore_inline_c_missing no_pollerfd no_coredump cfg_new);
+		ignore_inline_c_missing no_pollerfd no_coredump cfg_new
+		strace strace_inject);
 	require Test::More;
 	my @methods = grep(!/\W/, @Test::More::EXPORT);
 	eval(join('', map { "*$_=\\&Test::More::$_;" } @methods));
@@ -933,6 +934,26 @@ sub cfg_new ($;@) {
 	PublicInbox::Config->new($f);
 }
 
+our $strace_cmd;
+sub strace () {
+	skip 'linux only test' if $^O ne 'linux';
+	require_cmd('strace', 1);
+}
+
+sub strace_inject () {
+	my $cmd = strace;
+	state $ver = do {
+		require PublicInbox::Spawn;
+		my $v = PublicInbox::Spawn::run_qx([$cmd, '--version']);
+		$v =~ m!version\s+([1-9]+\.[0-9]+)! or
+				xbail "no strace --version: $v";
+		eval("v$1");
+	};
+	$ver ge v4.16 or skip "$cmd too old for syscall injection (".
+				sprintf('v%vd', $ver). ' < v4.16)';
+	$cmd
+}
+
 package PublicInbox::TestCommon::InboxWakeup;
 use strict;
 sub on_inbox_unlock { ${$_[0]}->($_[1]) }
diff --git a/t/io.t b/t/io.t
index 4c7a97a3..3ea00ed8 100644
--- a/t/io.t
+++ b/t/io.t
@@ -9,13 +9,7 @@ use PublicInbox::Spawn qw(which run_qx);
 
 # only test failures
 SKIP: {
-skip 'linux only test' if $^O ne 'linux';
-my $strace = which('strace') or skip 'strace missing for test';
-my $v = run_qx([$strace, '--version']);
-$v =~ m!version\s+([1-9]+\.[0-9]+)! or xbail "no strace --version: $v";
-$v = eval("v$1");
-$v ge v4.16 or skip "$strace too old for syscall injection (".
-		sprintf('v%vd', $v). ' < v4.16)';
+my $strace = strace_inject;
 my $env = { PERL5LIB => join(':', @INC) };
 my $opt = { 1 => \my $out, 2 => \my $err };
 my $dst = "$tmpdir/dst";
diff --git a/t/lei-import.t b/t/lei-import.t
index b2c1de9b..6ad4c97b 100644
--- a/t/lei-import.t
+++ b/t/lei-import.t
@@ -154,6 +154,33 @@ do {
 } until (!lei('ls-label') || $lei_out =~ /\bbin\b/ || now > $end);
 like($lei_out, qr/\bbin\b/, 'commit-delay eventually commits');
 
+SKIP: {
+	my $strace = strace_inject; # skips if strace is old or non-Linux
+	my $tmpdir = tmpdir;
+	my $tr = "$tmpdir/tr";
+	my $cmd = [ $strace, "-o$tr", '-f',
+		"-P", File::Spec->rel2abs('t/plack-qp.eml'),
+		'-e', 'inject=readv,read:error=EIO'];
+	lei_ok qw(daemon-pid);
+	chomp(my $daemon_pid = $lei_out);
+	push @$cmd, '-p', $daemon_pid;
+	my $strace_opt = { 1 => \my $out, 2 => \my $err };
+	require PublicInbox::Spawn;
+	require PublicInbox::AutoReap;
+	my $pid = PublicInbox::Spawn::spawn($cmd, \%ENV, $strace_opt);
+	my $ar = PublicInbox::AutoReap->new($pid);
+	tick; # wait for strace to attach
+	ok(!lei(qw(import -F eml t/plack-qp.eml)),
+		'-F eml import fails on pathname error injection');
+	like($lei_err, qr!error reading t/plack-qp\.eml: Input/output error!,
+		'EIO noted in stderr');
+	open $fh, '<', 't/plack-qp.eml';
+	ok(!lei(qw(import -F eml -), undef, { %$lei_opt, 0 => $fh }),
+		'-F eml import fails on stdin error injection');
+	like($lei_err, qr!error reading .*?: Input/output error!,
+		'EIO noted in stderr');
+}
+
 # see t/lei_to_mail.t for "import -F mbox*"
 });
 done_testing;

[PATCH 11/13] xapcmd: get rid of scalar wantarray popen_rd

[Eric Wong]

We can rely on Process::IO->attached_pid and work towards
simplifying popen_rd.
---
 lib/PublicInbox/Xapcmd.pm | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/Xapcmd.pm b/lib/PublicInbox/Xapcmd.pm
index c2b66e69..69f0af43 100644
--- a/lib/PublicInbox/Xapcmd.pm
+++ b/lib/PublicInbox/Xapcmd.pm
@@ -329,8 +329,8 @@ sub progress_pfx ($) {
 }
 
 sub kill_compact { # setup_signals callback
-	my ($sig, $pidref) = @_;
-	kill($sig, $$pidref) if defined($$pidref);
+	my ($sig, $ioref) = @_;
+	kill($sig, $$ioref->attached_pid // return) if defined($$ioref);
 }
 
 # xapian-compact wrapper
@@ -358,18 +358,16 @@ sub compact ($$) { # cb_spawn callback
 	}
 	$pr->("$pfx `".join(' ', @$cmd)."'\n") if $pr;
 	push @$cmd, $src, $dst;
-	my ($rd, $pid);
 	local @SIG{keys %SIG} = values %SIG;
-	setup_signals(\&kill_compact, \$pid);
-	($rd, $pid) = popen_rd($cmd, undef, $rdr);
+	setup_signals(\&kill_compact, \my $rd);
+	$rd = popen_rd($cmd, undef, $rdr);
 	while (<$rd>) {
 		if ($pr) {
 			s/\r/\r$pfx /g;
 			$pr->("$pfx $_");
 		}
 	}
-	waitpid($pid, 0);
-	die "@$cmd failed: \$?=$?\n" if $?;
+	$rd->close or die "@$cmd failed: \$?=$?\n";
 }
 
 sub cpdb_loop ($$$;$$) {

[PATCH 12/13] lei: get rid of autoreap usage

[Eric Wong]

We can rely on Process::IO->DESTROY to close and reap
in these cases.  This is the final step in eliminating
the wantarray invocations of popen_rd (and popen_wr).
---
 lib/PublicInbox/LeiInput.pm   | 13 +++++--------
 lib/PublicInbox/LeiRemote.pm  | 14 ++++++--------
 lib/PublicInbox/LeiXSearch.pm | 15 +++++++++------
 3 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/lib/PublicInbox/LeiInput.pm b/lib/PublicInbox/LeiInput.pm
index adb356c9..68c3c459 100644
--- a/lib/PublicInbox/LeiInput.pm
+++ b/lib/PublicInbox/LeiInput.pm
@@ -7,7 +7,6 @@ use v5.12;
 use PublicInbox::DS;
 use PublicInbox::Spawn qw(which popen_rd);
 use PublicInbox::InboxWritable qw(eml_from_path);
-use PublicInbox::AutoReap;
 
 # JMAP RFC 8621 4.1.1
 # https://www.iana.org/assignments/imap-jmap-keywords/imap-jmap-keywords.xhtml
@@ -114,15 +113,13 @@ sub handle_http_input ($$@) {
 	push @$curl, '-s', @$curl_opt;
 	my $cmd = $curl->for_uri($lei, $uri);
 	$lei->qerr("# $cmd");
-	my ($fh, $pid) = popen_rd($cmd, undef, { 2 => $lei->{2} });
-	my $ar = PublicInbox::AutoReap->new($pid);
+	my $fh = popen_rd($cmd, undef, { 2 => $lei->{2} });
 	grep(/\A--compressed\z/, @$curl) or
-		$fh = IO::Uncompress::Gunzip->new($fh, MultiStream => 1);
+		$fh = IO::Uncompress::Gunzip->new($fh,
+					MultiStream => 1, AutoClose => 1);
 	eval { $self->input_fh('mboxrd', $fh, $url, @args) };
-	my @err = ($@ ? $@ : ());
-	$ar->join;
-	push(@err, "\$?=$?") if $?;
-	$lei->child_error($?, "@$cmd failed: @err") if @err;
+	my $err = $@ ? ": $@" : '';
+	$lei->child_error($?, "@$cmd failed$err") if $err || $?;
 }
 
 sub oid2eml { # git->cat_async cb
diff --git a/lib/PublicInbox/LeiRemote.pm b/lib/PublicInbox/LeiRemote.pm
index 54750062..559fb8d5 100644
--- a/lib/PublicInbox/LeiRemote.pm
+++ b/lib/PublicInbox/LeiRemote.pm
@@ -12,7 +12,6 @@ use IO::Uncompress::Gunzip;
 use PublicInbox::MboxReader;
 use PublicInbox::Spawn qw(popen_rd);
 use PublicInbox::LeiCurl;
-use PublicInbox::AutoReap;
 use PublicInbox::ContentHash qw(git_sha);
 
 sub new {
@@ -22,7 +21,7 @@ sub new {
 
 sub isrch { $_[0] } # SolverGit expcets this
 
-sub _each_mboxrd_eml { # callback for MboxReader->mboxrd
+sub each_mboxrd_eml { # callback for MboxReader->mboxrd
 	my ($eml, $self) = @_;
 	my $lei = $self->{lei};
 	my $xoids = $lei->{ale}->xoids_for($eml, 1);
@@ -47,14 +46,13 @@ sub mset {
 	$uri->query_form(q => $qstr, x => 'm', r => 1); # r=1: relevance
 	my $cmd = $curl->for_uri($self->{lei}, $uri);
 	$self->{lei}->qerr("# $cmd");
-	my ($fh, $pid) = popen_rd($cmd, undef, { 2 => $lei->{2} });
-	my $ar = PublicInbox::AutoReap->new($pid);
 	$self->{smsg} = [];
-	$fh = IO::Uncompress::Gunzip->new($fh, MultiStream => 1);
-	PublicInbox::MboxReader->mboxrd($fh, \&_each_mboxrd_eml, $self);
+	my $fh = popen_rd($cmd, undef, { 2 => $lei->{2} });
+	$fh = IO::Uncompress::Gunzip->new($fh, MultiStream=>1, AutoClose=>1);
+	eval { PublicInbox::MboxReader->mboxrd($fh, \&each_mboxrd_eml, $self) };
+	my $err = $@ ? ": $@" : '';
 	my $wait = $self->{lei}->{sto}->wq_do('done');
-	$ar->join;
-	$lei->child_error($?) if $?;
+	$lei->child_error($?, "@$cmd failed$err") if $err || $?;
 	$self; # we are the mset (and $ibx, and $self)
 }
 
diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index ba8ff293..b09c2462 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -346,14 +346,17 @@ print STDERR $_;
 		my $cmd = $curl->for_uri($lei, $uri);
 		$lei->qerr("# $cmd");
 		$rdr->{2} //= popen_wr(@lbf_tee) if @lbf_tee;
-		my $cfh = popen_rd($cmd, undef, $rdr);
-		my $fh = IO::Uncompress::Gunzip->new($cfh, MultiStream => 1);
-		PublicInbox::MboxReader->mboxrd($fh, \&each_remote_eml, $self,
-						$lei, $each_smsg);
+		my $fh = popen_rd($cmd, undef, $rdr);
+		$fh = IO::Uncompress::Gunzip->new($fh,
+					MultiStream => 1, AutoClose => 1);
+		eval {
+			PublicInbox::MboxReader->mboxrd($fh, \&each_remote_eml,
+						$self, $lei, $each_smsg);
+		};
+		my ($exc, $code) = ($@, $?);
 		$lei->sto_done_request if delete($self->{-sto_imported});
+		die "E: $exc" if $exc && !$code;
 		my $nr = delete $lei->{-nr_remote_eml} // 0;
-		$cfh->close;
-		my $code = $?;
 		if (!$code) { # don't update if no results, maybe MTA is down
 			$lei->{lss}->cfg_set($key, $start) if $key && $nr;
 			mset_progress($lei, $lei->{-current_url}, $nr, $nr);

[PATCH 13/13] spawn: get rid of wantarray popen_rd/popen_wr

[Eric Wong]

We've updated all of our users to use Process::IO (and avoiding
tied handles) so the trade-off for using the array context
no longer exists.
---
 lib/PublicInbox/Spawn.pm | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index 8c798b39..8cc4dfaf 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -384,16 +384,14 @@ sub spawn ($;$$) {
 sub popen_rd {
 	my ($cmd, $env, $opt, @cb_arg) = @_;
 	pipe(my $r, local $opt->{1});
-	my $pid = spawn($cmd, $env, $opt);
-	wantarray ? ($r, $pid) : PublicInbox::IO::attach_pid($r, $pid, @cb_arg)
+	PublicInbox::IO::attach_pid($r, spawn($cmd, $env, $opt), @cb_arg);
 }
 
 sub popen_wr {
 	my ($cmd, $env, $opt, @cb_arg) = @_;
 	pipe(local $opt->{0}, my $w);
 	$w->autoflush(1);
-	my $pid = spawn($cmd, $env, $opt);
-	wantarray ? ($w, $pid) : PublicInbox::IO::attach_pid($w, $pid, @cb_arg)
+	PublicInbox::IO::attach_pid($w, spawn($cmd, $env, $opt), @cb_arg);
 }
 
 sub read_out_err ($) {