From: Eric Wong <e@80x24.org>
Date: Thu, 31 Dec 2020 13:51:49 +0000 (+0000)
Subject: use PublicInbox::DS for dwaitpid
X-Git-Tag: v1.7.0~1440
X-Git-Url: http://www.git.stargrave.org/?p=public-inbox.git;a=commitdiff_plain;h=d59a9cb667f106b29363795539b55116077bcd80

use PublicInbox::DS for dwaitpid

This simplifies our code and provides a more consistent API for
error handling.  PublicInbox::DS can be loaded nowadays on all
*BSDs and Linux distros easily without extra packages to
install.

The downside is possibly increased startup time, but it's
probably not as a big problem with lei being a daemon
(and -mda possibly following suite).
---

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 97a6f6ef..01c9abd4 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -21,19 +21,19 @@
 #        (tmpio = [ GLOB, offset, [ length ] ])
 package PublicInbox::DS;
 use strict;
+use v5.10.1;
+use parent qw(Exporter);
 use bytes;
 use POSIX qw(WNOHANG);
 use IO::Handle qw();
 use Fcntl qw(SEEK_SET :DEFAULT O_APPEND);
 use Time::HiRes qw(clock_gettime CLOCK_MONOTONIC);
-use parent qw(Exporter);
-our @EXPORT_OK = qw(now msg_more);
-use 5.010_001;
 use Scalar::Util qw(blessed);
 use PublicInbox::Syscall qw(:epoll);
 use PublicInbox::Tmpfile;
 use Errno qw(EAGAIN EINVAL);
 use Carp qw(confess carp);
+our @EXPORT_OK = qw(now msg_more dwaitpid);
 
 my $nextq; # queue for next_tick
 my $wait_pids; # list of [ pid, callback, callback_arg ]
@@ -215,8 +215,13 @@ sub reap_pids {
 		my $ret = waitpid($pid, WNOHANG);
 		if ($ret == 0) {
 			push @$wait_pids, $ary; # autovivifies @$wait_pids
-		} elsif ($cb) {
-			eval { $cb->($arg, $pid) };
+		} elsif ($ret == $pid) {
+			if ($cb) {
+				eval { $cb->($arg, $pid) };
+				warn "E: dwaitpid($pid) in_loop: $@" if $@;
+			}
+		} else {
+			warn "waitpid($pid, WNOHANG) = $ret, \$!=$!, \$?=$?";
 		}
 	}
 	# we may not be done, yet, and could've missed/masked a SIGCHLD:
@@ -608,13 +613,23 @@ sub shutdn ($) {
     }
 }
 
-# must be called with eval, PublicInbox::DS may not be loaded (see t/qspawn.t)
-sub dwaitpid ($$$) {
-	die "Not in EventLoop\n" unless $in_loop;
-	push @$wait_pids, [ @_ ]; # [ $pid, $cb, $arg ]
-
-	# We could've just missed our SIGCHLD, cover it, here:
-	enqueue_reap();
+sub dwaitpid ($;$$) {
+	my ($pid, $cb, $arg) = @_;
+	if ($in_loop) {
+		push @$wait_pids, [ $pid, $cb, $arg ];
+		# We could've just missed our SIGCHLD, cover it, here:
+		enqueue_reap();
+	} else {
+		my $ret = waitpid($pid, 0);
+		if ($ret == $pid) {
+			if ($cb) {
+				eval { $cb->($arg, $pid) };
+				warn "E: dwaitpid($pid) !in_loop: $@" if $@;
+			}
+		} else {
+			warn "waitpid($pid, 0) = $ret, \$!=$!, \$?=$?";
+		}
+	}
 }
 
 sub _run_later () {
diff --git a/lib/PublicInbox/Gcf2Client.pm b/lib/PublicInbox/Gcf2Client.pm
index 4bda5520..10820852 100644
--- a/lib/PublicInbox/Gcf2Client.pm
+++ b/lib/PublicInbox/Gcf2Client.pm
@@ -9,6 +9,7 @@ use PublicInbox::Git;
 use PublicInbox::Spawn qw(popen_rd);
 use IO::Handle ();
 use PublicInbox::Syscall qw(EPOLLONESHOT);
+use PublicInbox::DS qw(dwaitpid);
 # fields:
 #	async_cat => GitAsyncCat ref (read-only pipe)
 #	sock => writable pipe to Gcf2::loop
@@ -65,18 +66,11 @@ no warnings 'once';
 
 sub DESTROY {
 	my ($self) = @_;
-	my $pid = delete $self->{pid};
 	delete $self->{in};
-	return unless $pid;
-	eval {
-		PublicInbox::DS::dwaitpid($pid, undef, undef);
-		$self->close; # we're still in the event loop
-	};
-	if ($@) { # wait synchronously if not in event loop
-		my $sock = delete $self->{sock};
-		close $sock if $sock;
-		waitpid($pid, 0);
-	}
+	# GitAsyncCat::event_step may reap us with WNOHANG, too
+	my $pid = delete $self->{pid} or return;
+	PublicInbox::DS->in_loop ? $self->close : delete($self->{sock});
+	dwaitpid $pid;
 }
 
 # used by GitAsyncCat
diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 73dc7d3e..fdfe1269 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -21,6 +21,7 @@ use PublicInbox::Tmpfile;
 use IO::Poll qw(POLLIN);
 use Carp qw(croak);
 use Digest::SHA ();
+use PublicInbox::DS qw(dwaitpid);
 our @EXPORT_OK = qw(git_unquote git_quote);
 our $PIPE_BUFSIZ = 65536; # Linux default
 our $in_cleanup;
@@ -326,10 +327,7 @@ sub _destroy {
 
 	# GitAsyncCat::event_step may delete {pid}
 	my $p = delete $self->{$pid} or return;
-
-	# PublicInbox::DS may not be loaded
-	eval { PublicInbox::DS::dwaitpid($p, undef, undef) };
-	waitpid($p, 0) if $@; # wait synchronously if not in event loop
+	dwaitpid $p;
 }
 
 sub cat_async_abort ($) {
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 1ba9eff3..7b7f45de 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -18,7 +18,7 @@ use Sys::Syslog qw(syslog openlog);
 use PublicInbox::Config;
 use PublicInbox::Syscall qw($SFD_NONBLOCK EPOLLIN EPOLLONESHOT);
 use PublicInbox::Sigfd;
-use PublicInbox::DS qw(now);
+use PublicInbox::DS qw(now dwaitpid);
 use PublicInbox::Spawn qw(spawn run_die);
 use PublicInbox::OnDestroy;
 use Text::Wrap qw(wrap);
@@ -604,7 +604,7 @@ sub lei_git { # support passing through random git commands
 	my ($self, @argv) = @_;
 	my %rdr = map { $_ => $self->{$_} } (0..2);
 	my $pid = spawn(['git', @argv], $self->{env}, \%rdr);
-	PublicInbox::DS::dwaitpid($pid, \&reap_exec, $self);
+	dwaitpid($pid, \&reap_exec, $self);
 }
 
 sub accept_dispatch { # Listener {post_accept} callback
diff --git a/lib/PublicInbox/ProcessPipe.pm b/lib/PublicInbox/ProcessPipe.pm
index c9234f42..afbb048d 100644
--- a/lib/PublicInbox/ProcessPipe.pm
+++ b/lib/PublicInbox/ProcessPipe.pm
@@ -5,6 +5,7 @@
 package PublicInbox::ProcessPipe;
 use strict;
 use v5.10.1;
+use PublicInbox::DS qw(dwaitpid);
 
 sub TIEHANDLE {
 	my ($class, $pid, $fh, $cb, $arg) = @_;
@@ -25,19 +26,21 @@ sub PRINT {
 	print { $self->{fh} } @_;
 }
 
+sub adjust_ret { # dwaitpid callback
+	my ($retref, $pid) = @_;
+	$$retref = '' if $?
+}
+
 sub CLOSE {
 	my $fh = delete($_[0]->{fh});
 	my $ret = defined $fh ? close($fh) : '';
 	my ($pid, $cb, $arg) = delete @{$_[0]}{qw(pid cb arg)};
 	if (defined $pid) {
-		# PublicInbox::DS may not be loaded
-		eval { PublicInbox::DS::dwaitpid($pid, $cb, $arg) };
-
-		if ($@) { # ok, not in the event loop, work synchronously
-			waitpid($pid, 0);
-			$ret = '' if $?;
-			$cb->($arg, $pid) if $cb;
+		unless ($cb) {
+			$cb = \&adjust_ret;
+			$arg = \$ret;
 		}
+		dwaitpid $pid, $cb, $arg;
 	}
 	$ret;
 }
diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm
index 2aa2042a..5bbbb027 100644
--- a/lib/PublicInbox/Qspawn.pm
+++ b/lib/PublicInbox/Qspawn.pm
@@ -12,12 +12,13 @@
 # operate in.  This can be useful to ensure smaller inboxes can
 # be cloned while cloning of large inboxes is maxed out.
 #
-# This does not depend on PublicInbox::DS or any other external
-# scheduling mechanism, you just need to call start() and finish()
-# appropriately. However, public-inbox-httpd (which uses PublicInbox::DS)
-# will be able to schedule this based on readability of stdout from
-# the spawned process.  See GitHTTPBackend.pm and SolverGit.pm for
-# usage examples.  It does not depend on any form of threading.
+# This does not depend on the PublicInbox::DS->EventLoop or any
+# other external scheduling mechanism, you just need to call
+# start() and finish() appropriately. However, public-inbox-httpd
+# (which uses PublicInbox::DS)  will be able to schedule this
+# based on readability of stdout from the spawned process.
+# See GitHTTPBackend.pm and SolverGit.pm for usage examples.
+# It does not depend on any form of threading.
 #
 # This is useful for scheduling CGI execution of both long-lived
 # git-http-backend(1) process (for "git clone") as well as short-lived
@@ -27,6 +28,7 @@ package PublicInbox::Qspawn;
 use strict;
 use PublicInbox::Spawn qw(popen_rd);
 use PublicInbox::GzipFilter;
+use PublicInbox::DS qw(dwaitpid); # doesn't need event loop
 
 # n.b.: we get EAGAIN with public-inbox-httpd, and EINTR on other PSGI servers
 use Errno qw(EAGAIN EINTR);
@@ -116,37 +118,12 @@ sub finalize ($$) {
 }
 
 # callback for dwaitpid
-sub waitpid_err ($$) {
-	my ($self, $pid) = @_;
-	my $xpid = delete $self->{pid};
-	my $err;
-	if (defined $pid) {
-		if ($pid > 0) { # success!
-			$err = child_err($?);
-		} elsif ($pid < 0) { # ??? does this happen in our case?
-			$err = "W: waitpid($xpid, 0) => $pid: $!";
-		} # else should not be called with pid == 0
-	}
-	finalize($self, $err);
-}
-
-sub do_waitpid ($) {
-	my ($self) = @_;
-	my $pid = $self->{pid};
-	# PublicInbox::DS may not be loaded
-	eval { PublicInbox::DS::dwaitpid($pid, \&waitpid_err, $self) };
-	# done if we're running in PublicInbox::DS::EventLoop
-	if ($@) {
-		# non public-inbox-{httpd,nntpd} callers may block:
-		my $ret = waitpid($pid, 0);
-		waitpid_err($self, $ret);
-	}
-}
+sub waitpid_err { finalize($_[0], child_err($?)) }
 
 sub finish ($;$) {
 	my ($self, $err) = @_;
 	if (delete $self->{rpipe}) {
-		do_waitpid($self);
+		dwaitpid $self->{pid}, \&waitpid_err, $self;
 	} else {
 		finalize($self, $err);
 	}