unofficial mirror of meta@public-inbox.org
 help / color / mirror / Atom feed
From: Eric Wong <e@80x24.org>
To: meta@public-inbox.org
Subject: [PATCH 31/36] use PublicInbox::DS for dwaitpid
Date: Thu, 31 Dec 2020 13:51:49 +0000	[thread overview]
Message-ID: <20201231135154.6070-32-e@80x24.org> (raw)
In-Reply-To: <20201231135154.6070-1-e@80x24.org>

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).
---
 lib/PublicInbox/DS.pm          | 39 ++++++++++++++++++++----------
 lib/PublicInbox/Gcf2Client.pm  | 16 ++++---------
 lib/PublicInbox/Git.pm         |  6 ++---
 lib/PublicInbox/LEI.pm         |  4 ++--
 lib/PublicInbox/ProcessPipe.pm | 17 ++++++++------
 lib/PublicInbox/Qspawn.pm      | 43 ++++++++--------------------------
 6 files changed, 56 insertions(+), 69 deletions(-)

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);
 	}

  parent reply	other threads:[~2020-12-31 13:51 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-31 13:51 [PATCH 00/36] another round of lei stuff Eric Wong
2020-12-31 13:51 ` [PATCH 01/36] import: respect init.defaultBranch Eric Wong
2020-12-31 13:51 ` [PATCH 02/36] lei_store: use per-machine refname as git HEAD Eric Wong
2020-12-31 13:51 ` [PATCH 03/36] revert "lei_store: use per-machine refname as git HEAD" Eric Wong
2020-12-31 13:51 ` [PATCH 04/36] lei_to_mail: initial implementation for writing mbox formats Eric Wong
2020-12-31 13:51 ` [PATCH 05/36] sharedkv: fork()-friendly key-value store Eric Wong
2020-12-31 13:51 ` [PATCH 06/36] sharedkv: split out index_values Eric Wong
2020-12-31 13:51 ` [PATCH 07/36] lei_to_mail: start atomic and compressed mbox writing Eric Wong
2020-12-31 13:51 ` [PATCH 08/36] mboxreader: new class for reading various mbox formats Eric Wong
2020-12-31 13:51 ` [PATCH 09/36] lei_to_mail: start --augment, dedupe, bz2 and xz Eric Wong
2020-12-31 13:51 ` [PATCH 10/36] lei: implement various deduplication strategies Eric Wong
2020-12-31 13:51 ` [PATCH 11/36] lei_to_mail: lazy-require LeiDedupe Eric Wong
2020-12-31 13:51 ` [PATCH 12/36] lei_to_mail: support for non-seekable outputs Eric Wong
2020-12-31 13:51 ` [PATCH 13/36] lei_to_mail: support Maildir, fix+test --augment Eric Wong
2020-12-31 13:51 ` [PATCH 14/36] ipc: generic IPC dispatch based on Storable Eric Wong
2020-12-31 13:51 ` [PATCH 15/36] ipc: support Sereal Eric Wong
2020-12-31 13:51 ` [PATCH 16/36] lei_store: add ->set_eml, ->add_eml can return smsg Eric Wong
2020-12-31 13:51 ` [PATCH 17/36] lei: rename "extinbox" => "external" Eric Wong
2020-12-31 13:51 ` [PATCH 18/36] mid: use defined-or with `push' for uniqueness check Eric Wong
2020-12-31 13:51 ` [PATCH 19/36] mid: hoist out mids_in sub Eric Wong
2020-12-31 13:51 ` [PATCH 20/36] lei_store: handle messages without Message-ID at all Eric Wong
2020-12-31 13:51 ` [PATCH 21/36] ipc: use shutdown(2), base atfork* callback Eric Wong
2020-12-31 13:51 ` [PATCH 22/36] lei_to_mail: unlink mboxes if not augmenting Eric Wong
2020-12-31 13:51 ` [PATCH 23/36] lei: add --mfolder as an --output alias Eric Wong
2020-12-31 13:51 ` [PATCH 24/36] spawn: move run_die here from PublicInbox::Import Eric Wong
2020-12-31 13:51 ` [PATCH 25/36] init: remove embedded UnlinkMe package Eric Wong
2020-12-31 13:51 ` [PATCH 26/36] t/run: avoid uninitialized var on incomplete test Eric Wong
2020-12-31 13:51 ` [PATCH 27/36] gcf2client: reap process on DESTROY Eric Wong
2020-12-31 13:51 ` [PATCH 28/36] lei_to_mail: open FIFOs O_WRONLY so we block Eric Wong
2020-12-31 13:51 ` [PATCH 29/36] searchidxshard: call DS->Reset at worker start Eric Wong
2020-12-31 13:51 ` [PATCH 30/36] t/ipc.t: test for references via `die' Eric Wong
2020-12-31 13:51 ` Eric Wong [this message]
2020-12-31 13:51 ` [PATCH 32/36] syscall: SFD_NONBLOCK can be a constant, again Eric Wong
2020-12-31 13:51 ` [PATCH 33/36] lei: avoid Spawn package when starting daemon Eric Wong
2020-12-31 13:51 ` [PATCH 34/36] avoid calling waitpid from children in DESTROY Eric Wong
2020-12-31 13:51 ` [PATCH 35/36] ds: clobber $in_loop first at reset Eric Wong
2020-12-31 13:51 ` [PATCH 36/36] on_destroy: support PID owner guard Eric Wong

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: https://public-inbox.org/README

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201231135154.6070-32-e@80x24.org \
    --to=e@80x24.org \
    --cc=meta@public-inbox.org \
    /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.
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).