user/dev discussion of public-inbox itself
 help / color / mirror / code / Atom feed
* [PATCH 0/3] clone+fetch stuff
@ 2021-10-14  4:32 Eric Wong
  2021-10-14  4:32 ` [PATCH 1/3] clone+fetch: respect umask for all downloaded files Eric Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14  4:32 UTC (permalink / raw)
  To: meta

Eric Wong (3):
  clone+fetch: respect umask for all downloaded files
  lei add-external --mirror: respect client umask
  lei: give workers their own process group

 lib/PublicInbox/Fetch.pm      |  5 ++---
 lib/PublicInbox/LEI.pm        | 21 +++++++++++++++++----
 lib/PublicInbox/LeiMirror.pm  | 35 +++++++++++++++++++++--------------
 lib/PublicInbox/LeiXSearch.pm |  2 +-
 script/lei                    |  2 ++
 5 files changed, 43 insertions(+), 22 deletions(-)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] clone+fetch: respect umask for all downloaded files
  2021-10-14  4:32 [PATCH 0/3] clone+fetch stuff Eric Wong
@ 2021-10-14  4:32 ` Eric Wong
  2021-10-14  4:32 ` [PATCH 2/3] lei add-external --mirror: respect client umask Eric Wong
  2021-10-14  4:32 ` [PATCH 3/3] lei: give workers their own process group Eric Wong
  2 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14  4:32 UTC (permalink / raw)
  To: meta

Since public inboxes are usually intended to be public,
the File::Temp default permission of 0600 is wrong.
Just respect the user's umask in this case as git-clone
does.

This doesn't work for "lei add-external --mirror", yet;
but it will...
---
 lib/PublicInbox/Fetch.pm     |  5 ++---
 lib/PublicInbox/LeiMirror.pm | 30 ++++++++++++++++++------------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/Fetch.pm b/lib/PublicInbox/Fetch.pm
index 0d4badbf216f..5261cad19855 100644
--- a/lib/PublicInbox/Fetch.pm
+++ b/lib/PublicInbox/Fetch.pm
@@ -218,13 +218,12 @@ EOM
 	}
 	for my $i (@new_epoch) { $mg->epoch_cfg_set($i) }
 	if ($ft) {
-		my $fn = $ft->filename;
 		if ($mculled) {
 			my $json = PublicInbox::Config->json->encode($m1);
+			my $fn = $ft->filename;
 			gzip(\$json => $fn) or die "gzip: $GzipError";
 		}
-		rename($fn, $mf) or die "E: rename($fn, $mf): $!\n";
-		$ft->unlink_on_destroy(0);
+		PublicInbox::LeiMirror::ft_rename($ft, $mf, 0666);
 	}
 	$lei->child_error($xit << 8) if $fp2 && $xit;
 }
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index ec41bec6f16b..1369c00c57fd 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -12,6 +12,7 @@ use IO::Compress::Gzip qw(gzip $GzipError);
 use PublicInbox::Spawn qw(popen_rd spawn run_die);
 use File::Temp ();
 use Fcntl qw(SEEK_SET O_CREAT O_EXCL O_WRONLY);
+use Carp qw(croak);
 
 sub _wq_done_wait { # dwaitpid callback (via wq_eof)
 	my ($arg, $pid) = @_;
@@ -89,24 +90,31 @@ sub clone_cmd {
 	@cmd;
 }
 
+sub ft_rename ($$$) {
+	my ($ft, $dst, $open_mode) = @_;
+	my $fn = $ft->filename;
+	my @st = stat($dst);
+	my $mode = @st ? ($st[2] & 07777) : ($open_mode & ~umask);
+	chmod($mode, $ft) or croak "E: chmod $fn: $!";
+	rename($fn, $dst) or croak "E: rename($fn => $ft): $!";
+	$ft->unlink_on_destroy(0);
+}
+
 sub _get_txt { # non-fatal
-	my ($self, $endpoint, $file) = @_;
+	my ($self, $endpoint, $file, $mode) = @_;
 	my $uri = URI->new($self->{src});
 	my $lei = $self->{lei};
 	my $path = $uri->path;
 	chop($path) eq '/' or die "BUG: $uri not canonicalized";
 	$uri->path("$path/$endpoint");
 	my $ft = File::Temp->new(TEMPLATE => "$file-XXXX", DIR => $self->{dst});
-	my $f = $ft->filename;
 	my $opt = { 0 => $lei->{0}, 1 => $lei->{1}, 2 => $lei->{2} };
 	my $cmd = $self->{curl}->for_uri($lei, $uri,
-					qw(--compressed -R -o), $f);
+					qw(--compressed -R -o), $ft->filename);
 	my $cerr = run_reap($lei, $cmd, $opt);
 	return "$uri missing" if ($cerr >> 8) == 22;
 	return "# @$cmd failed (non-fatal)" if $cerr;
-	my $ce = "$self->{dst}/$file";
-	rename($f, $ce) or return "rename($f, $ce): $! (non-fatal)";
-	$ft->unlink_on_destroy(0);
+	ft_rename($ft, "$self->{dst}/$file", $mode);
 	undef; # success
 }
 
@@ -119,10 +127,10 @@ sub _try_config {
 		File::Path::mkpath($dst);
 		-d $dst or die "mkpath($dst): $!\n";
 	}
-	my $err = _get_txt($self, qw(_/text/config/raw inbox.config.example));
+	my $err = _get_txt($self,
+			qw(_/text/config/raw inbox.config.example), 0444);
 	return warn($err, "\n") if $err;
 	my $f = "$self->{dst}/inbox.config.example";
-	chmod((stat($f))[2] & 0444, $f) or die "chmod(a-w, $f): $!";
 	my $cfg = PublicInbox::Config->git_config_dump($f, $self->{lei}->{2});
 	my $ibx = $self->{ibx} = {};
 	for my $sec (grep(/\Apublicinbox\./, @{$cfg->{-section_order}})) {
@@ -150,7 +158,7 @@ sub set_description ($) {
 sub index_cloned_inbox {
 	my ($self, $iv) = @_;
 	my $lei = $self->{lei};
-	my $err = _get_txt($self, qw(description description));
+	my $err = _get_txt($self, qw(description description), 0666);
 	warn($err, "\n") if $err; # non fatal
 	eval { set_description($self) };
 	warn $@ if $@;
@@ -404,9 +412,7 @@ EOM
 		my $json = PublicInbox::Config->json->encode($m);
 		gzip(\$json => $fn) or die "gzip: $GzipError";
 	}
-	my $fin = "$self->{dst}/manifest.js.gz";
-	rename($fn, $fin) or die "E: rename($fn, $fin): $!";
-	$ft->unlink_on_destroy(0);
+	ft_rename($ft, "$self->{dst}/manifest.js.gz", 0666);
 }
 
 sub start_clone_url {

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3] lei add-external --mirror: respect client umask
  2021-10-14  4:32 [PATCH 0/3] clone+fetch stuff Eric Wong
  2021-10-14  4:32 ` [PATCH 1/3] clone+fetch: respect umask for all downloaded files Eric Wong
@ 2021-10-14  4:32 ` Eric Wong
  2021-10-14  4:32 ` [PATCH 3/3] lei: give workers their own process group Eric Wong
  2 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14  4:32 UTC (permalink / raw)
  To: meta

While lei is intended for non-public mail and runs umask(077)
by default, externals are one area which can safely defer to
the user's umask.

Instead of sending it unconditionally with every command, only
have lei-daemon request it when necessary.
---
 lib/PublicInbox/LEI.pm       | 11 +++++++++++
 lib/PublicInbox/LeiMirror.pm |  2 ++
 script/lei                   |  2 ++
 3 files changed, 15 insertions(+)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index bd8a6bef632b..635cd0c5508a 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -1518,4 +1518,15 @@ sub cfg_dump ($$) {
 	undef;
 }
 
+sub request_umask {
+	my ($lei) = @_;
+	my $s = $lei->{sock} // return;
+	send($s, 'umask', MSG_EOR) // die "send: $!";
+	vec(my $rvec = '', fileno($s), 1) = 1;
+	select($rvec, undef, undef, 2) or die 'timeout waiting for umask';
+	recv($s, my $v, 5, 0) // die "recv: $!";
+	(my $u, $lei->{client_umask}) = unpack('AV', $v);
+	$u eq 'u' or warn "E: recv $v has no umask";
+}
+
 1;
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 1369c00c57fd..fb73d8631670 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -424,6 +424,7 @@ sub start_clone_url {
 sub do_mirror { # via wq_io_do
 	my ($self) = @_;
 	my $lei = $self->{lei};
+	umask($lei->{client_umask}) if defined $lei->{client_umask};
 	eval {
 		my $iv = $lei->{opt}->{'inbox-version'};
 		if (defined $iv) {
@@ -448,6 +449,7 @@ sub start {
 	require PublicInbox::Inbox;
 	require PublicInbox::Admin;
 	require PublicInbox::InboxWritable;
+	$lei->request_umask;
 	my ($op_c, $ops) = $lei->workers_start($self, 1);
 	$lei->{wq1} = $self;
 	$self->wq_io_do('do_mirror', []);
diff --git a/script/lei b/script/lei
index bc43779821e7..8f6e8aacb86b 100755
--- a/script/lei
+++ b/script/lei
@@ -122,6 +122,8 @@ while (1) {
 		$exec_cmd->(\@fds, split(/\0/, $1));
 	} elsif ($buf eq '-WINCH') {
 		kill($buf, @parent); # for MUA
+	} elsif ($buf eq 'umask') {
+		send($sock, 'u'.pack('V', umask), MSG_EOR) or die "send: $!"
 	} elsif ($buf =~ /\Ax_it ([0-9]+)\z/) {
 		$x_it_code ||= $1 + 0;
 		last;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/3] lei: give workers their own process group
  2021-10-14  4:32 [PATCH 0/3] clone+fetch stuff Eric Wong
  2021-10-14  4:32 ` [PATCH 1/3] clone+fetch: respect umask for all downloaded files Eric Wong
  2021-10-14  4:32 ` [PATCH 2/3] lei add-external --mirror: respect client umask Eric Wong
@ 2021-10-14  4:32 ` Eric Wong
  2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
  2 siblings, 1 reply; 12+ messages in thread
From: Eric Wong @ 2021-10-14  4:32 UTC (permalink / raw)
  To: meta

This lets users Ctrl-Z from their terminal to pause an entire
git-clone process hierarchy.
---
 lib/PublicInbox/LEI.pm        | 10 ++++++----
 lib/PublicInbox/LeiMirror.pm  |  3 +--
 lib/PublicInbox/LeiXSearch.pm |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 635cd0c5508a..145af7e2cb59 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -459,9 +459,9 @@ my @WQ_KEYS = qw(lxs l2m ikw pmd wq1 lne); # internal workers
 sub _drop_wq {
 	my ($self) = @_;
 	for my $wq (grep(defined, delete(@$self{@WQ_KEYS}))) {
-		if ($wq->wq_kill) {
+		if ($wq->wq_kill('-TERM')) {
 			$wq->wq_close(0, undef, $self);
-		} elsif ($wq->wq_kill_old) {
+		} elsif ($wq->wq_kill_old('-TERM')) {
 			$wq->wq_wait_old(undef, $self);
 		}
 		$wq->DESTROY;
@@ -575,6 +575,7 @@ sub _lei_atfork_child {
 	} else { # worker, Net::NNTP (Net::Cmd) uses STDERR directly
 		open STDERR, '+>&='.fileno($self->{2}) or warn "open $!";
 		STDERR->autoflush(1);
+		POSIX::setpgid(0, $$) // die "setpgid(0, $$): $!";
 	}
 	close($_) for (grep(defined, delete @$self{qw(3 old_1 au_done)}));
 	if (my $op_c = delete $self->{pkt_op_c}) {
@@ -1147,9 +1148,10 @@ sub event_step {
 		if ($buf eq '') {
 			_drop_wq($self); # EOF, client disconnected
 			dclose($self);
-		} elsif ($buf =~ /\A(STOP|CONT)\z/) {
+		} elsif ($buf =~ /\A(?:STOP|CONT)\z/) {
+			my $sig = "-$buf";
 			for my $wq (grep(defined, @$self{@WQ_KEYS})) {
-				$wq->wq_kill($buf) or $wq->wq_kill_old($buf);
+				$wq->wq_kill($sig) or $wq->wq_kill_old($sig);
 			}
 		} else {
 			die "unrecognized client signal: $buf";
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index fb73d8631670..f1bc82e27205 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -9,7 +9,7 @@ use parent qw(PublicInbox::IPC);
 use PublicInbox::Config;
 use IO::Uncompress::Gunzip qw(gunzip $GunzipError);
 use IO::Compress::Gzip qw(gzip $GzipError);
-use PublicInbox::Spawn qw(popen_rd spawn run_die);
+use PublicInbox::Spawn qw(popen_rd spawn);
 use File::Temp ();
 use Fcntl qw(SEEK_SET O_CREAT O_EXCL O_WRONLY);
 use Carp qw(croak);
@@ -192,7 +192,6 @@ sub index_cloned_inbox {
 sub run_reap {
 	my ($lei, $cmd, $opt) = @_;
 	$lei->qerr("# @$cmd");
-	$opt->{pgid} = 0 if $lei->{sock};
 	my $pid = spawn($cmd, undef, $opt);
 	my $reap = PublicInbox::OnDestroy->new($lei->can('sigint_reap'), $pid);
 	waitpid($pid, 0) == $pid or die "waitpid @$cmd: $!";
diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index ee9216feeb23..668d0b6e5df3 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -469,7 +469,7 @@ sub do_post_augment {
 	$err = $@;
 	if ($err) {
 		if (my $lxs = delete $lei->{lxs}) {
-			$lxs->wq_kill;
+			$lxs->wq_kill('-TERM');
 			$lxs->wq_close(0, undef, $lei);
 		}
 		$lei->fail("$err");

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 0/7] lei: more process handling fixes
  2021-10-14  4:32 ` [PATCH 3/3] lei: give workers their own process group Eric Wong
@ 2021-10-14 13:16   ` Eric Wong
  2021-10-14 13:16     ` [PATCH 1/7] lei: use send() perlop for signals Eric Wong
                       ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14 13:16 UTC (permalink / raw)
  To: meta

"lei up --all" SIGTSTP/CONT/INT handling was totally
broken and now fixed.  And we put cat-file processes
into their own pgrp, so it avoids scary errors when
hitting Ctrl-C on -extindex, too.

Automated testing of interactive stuff is tricky, so
it's not being done, currently :<

Eric Wong (7):
  lei: use send() perlop for signals
  git: async_err shows retried requests properly
  git: ->fail invokes current callback
  git: cat-file --batch are their own pgrp
  lei: TSTP affects all curl and related subprocesses
  lei up: actually rely on DESTROY for --all
  lei up --all: send signals to workers, receive errors

 MANIFEST                      |  1 +
 lib/PublicInbox/AutoReap.pm   | 34 +++++++++++++++++++++++++++++++++
 lib/PublicInbox/Git.pm        | 36 +++++++++++++++++++----------------
 lib/PublicInbox/LEI.pm        | 14 +++++++-------
 lib/PublicInbox/LeiInput.pm   |  7 ++++---
 lib/PublicInbox/LeiMirror.pm  |  8 +++-----
 lib/PublicInbox/LeiRemote.pm  | 13 +++++--------
 lib/PublicInbox/LeiToMail.pm  |  2 +-
 lib/PublicInbox/LeiUp.pm      | 22 ++++++++++++++++++---
 lib/PublicInbox/LeiXSearch.pm | 20 +++++++++----------
 lib/PublicInbox/TestCommon.pm | 36 +++--------------------------------
 script/lei                    |  4 ++--
 12 files changed, 108 insertions(+), 89 deletions(-)
 create mode 100644 lib/PublicInbox/AutoReap.pm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/7] lei: use send() perlop for signals
  2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
@ 2021-10-14 13:16     ` Eric Wong
  2021-10-14 13:16     ` [PATCH 2/7] git: async_err shows retried requests properly Eric Wong
                       ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14 13:16 UTC (permalink / raw)
  To: meta

This may save us a small bit of startup time since there's
fewer args and opcodes should be smaller.
---
 script/lei | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/script/lei b/script/lei
index 8f6e8aacb86b..5cad19d77603 100755
--- a/script/lei
+++ b/script/lei
@@ -107,8 +107,8 @@ my $buf = join("\0", scalar(@ARGV), @ARGV);
 while (my ($k, $v) = each %ENV) { $buf .= "\0$k=$v" }
 $buf .= "\0\0";
 $send_cmd->($sock, [0, 1, 2, fileno($dh)], $buf, MSG_EOR) or die "sendmsg: $!";
-$SIG{TSTP} = sub { $send_cmd->($sock, [], 'STOP', MSG_EOR); kill 'STOP', $$ };
-$SIG{CONT} = sub { $send_cmd->($sock, [], 'CONT', MSG_EOR) };
+$SIG{TSTP} = sub { send($sock, 'STOP', MSG_EOR); kill 'STOP', $$ };
+$SIG{CONT} = sub { send($sock, 'CONT', MSG_EOR) };
 
 my $x_it_code = 0;
 while (1) {

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/7] git: async_err shows retried requests properly
  2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
  2021-10-14 13:16     ` [PATCH 1/7] lei: use send() perlop for signals Eric Wong
@ 2021-10-14 13:16     ` Eric Wong
  2021-10-14 13:16     ` [PATCH 3/7] git: ->fail invokes current callback Eric Wong
                       ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14 13:16 UTC (permalink / raw)
  To: meta

We make $req a reference upon retrying, but
"SCALAR(...)" in error messages isn't helpful, so
dereference the scalar ref.
---
 lib/PublicInbox/Git.pm | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 7c08be47bbe4..016dd2ae30e9 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -19,7 +19,7 @@ use Time::HiRes qw(stat);
 use PublicInbox::Spawn qw(popen_rd spawn);
 use PublicInbox::Tmpfile;
 use IO::Poll qw(POLLIN);
-use Carp qw(croak);
+use Carp qw(croak carp);
 use Digest::SHA ();
 use PublicInbox::DS qw(dwaitpid);
 our @EXPORT_OK = qw(git_unquote git_quote);
@@ -228,7 +228,7 @@ sub cat_async_step ($$) {
 	}
 	$self->{rbuf} = $rbuf if $$rbuf ne '';
 	eval { $cb->($bref, $oid, $type, $size, $arg) };
-	async_err($self, "E: cat $req ($oid) $@") if $@;
+	async_err($self, $req, $oid, $@, 'cat') if $@;
 }
 
 sub cat_async_wait ($) {
@@ -274,7 +274,7 @@ sub check_async_step ($$) {
 	}
 	$self->{rbuf_c} = $rbuf if $$rbuf ne '';
 	eval { $cb->($hex, $type, $size, $arg, $self) };
-	async_err($self, "E: check $req ($hex) $@") if $@;
+	async_err($self, $req, $hex, $@, 'check') if $@;
 }
 
 sub check_async_wait ($) {
@@ -342,6 +342,7 @@ sub async_abort ($) {
 			my $q = $self->{"inflight$c"};
 			while (@$q) {
 				my ($req, $cb, $arg) = splice(@$q, 0, 3);
+				$req = $$req if ref($req);
 				$req =~ s/ .*//; # drop git_dir for Gcf2Client
 				eval { $cb->(undef, $req, undef, undef, $arg) };
 				warn "E: (in abort) $req: $@" if $@;
@@ -359,10 +360,11 @@ sub fail { # may be augmented in subclasses
 	croak(ref($self) . ' ' . ($self->{git_dir} // '') . ": $msg");
 }
 
-sub async_err ($$) {
-	my ($self, $msg) = @_;
-	return warn($msg) if $async_warn;
-	$self->fail($msg);
+sub async_err ($$$$$) {
+	my ($self, $req, $oid, $err, $action) = @_;
+	$req = $$req if ref($req); # retried
+	my $msg = "E: $action $req ($oid): $err";
+	$async_warn ? carp($msg) : $self->fail($msg);
 }
 
 # $git->popen(qw(show f00)); # or

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/7] git: ->fail invokes current callback
  2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
  2021-10-14 13:16     ` [PATCH 1/7] lei: use send() perlop for signals Eric Wong
  2021-10-14 13:16     ` [PATCH 2/7] git: async_err shows retried requests properly Eric Wong
@ 2021-10-14 13:16     ` Eric Wong
  2021-10-14 13:16     ` [PATCH 4/7] git: cat-file --batch are their own pgrp Eric Wong
                       ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14 13:16 UTC (permalink / raw)
  To: meta

While we try to invoke all pending callbacks to force error
handling, the current callback wasn't getting invoked on
invoked on async_abort if my_read/my_readline failed.
---
 lib/PublicInbox/Git.pm | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 016dd2ae30e9..37342d7d10a4 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -179,8 +179,8 @@ sub my_readline ($$) {
 	}
 }
 
-sub cat_async_retry ($$$$$) {
-	my ($self, $inflight, $req, $cb, $arg) = @_;
+sub cat_async_retry ($$) {
+	my ($self, $inflight) = @_;
 
 	# {inflight} may be non-existent, but if it isn't we delete it
 	# here to prevent cleanup() from waiting:
@@ -189,12 +189,13 @@ sub cat_async_retry ($$$$$) {
 
 	$self->{inflight} = $inflight;
 	batch_prepare($self);
-	my $buf = "$req\n";
+	my $buf = '';
 	for (my $i = 0; $i < @$inflight; $i += 3) {
 		$buf .= "$inflight->[$i]\n";
 	}
 	print { $self->{out} } $buf or $self->fail("write error: $!");
-	unshift(@$inflight, \$req, $cb, $arg); # \$ref to indicate retried
+	my $req = shift @$inflight;
+	unshift(@$inflight, \$req); # \$ref to indicate retried
 
 	cat_async_step($self, $inflight); # take one step
 }
@@ -202,7 +203,7 @@ sub cat_async_retry ($$$$$) {
 sub cat_async_step ($$) {
 	my ($self, $inflight) = @_;
 	die 'BUG: inflight empty or odd' if scalar(@$inflight) < 3;
-	my ($req, $cb, $arg) = splice(@$inflight, 0, 3);
+	my ($req, $cb, $arg) = @$inflight[0, 1, 2];
 	my $rbuf = delete($self->{rbuf}) // \(my $new = '');
 	my ($bref, $oid, $type, $size);
 	my $head = my_readline($self->{in}, $rbuf);
@@ -217,8 +218,7 @@ sub cat_async_step ($$) {
 		# ref($req) indicates it's already been retried
 		# -gcf2 retries internally, so it never hits this path:
 		if (!ref($req) && !$in_cleanup && $self->alternates_changed) {
-			return cat_async_retry($self, $inflight,
-						$req, $cb, $arg);
+			return cat_async_retry($self, $inflight);
 		}
 		$type = 'missing';
 		$oid = ref($req) ? $$req : $req if $oid eq '';
@@ -227,6 +227,7 @@ sub cat_async_step ($$) {
 		$self->fail("bad result from async cat-file: $head$err");
 	}
 	$self->{rbuf} = $rbuf if $$rbuf ne '';
+	splice(@$inflight, 0, 3); # don't retry $cb on ->fail
 	eval { $cb->($bref, $oid, $type, $size, $arg) };
 	async_err($self, $req, $oid, $@, 'cat') if $@;
 }
@@ -259,7 +260,7 @@ sub cat_file {
 sub check_async_step ($$) {
 	my ($self, $inflight_c) = @_;
 	die 'BUG: inflight empty or odd' if scalar(@$inflight_c) < 3;
-	my ($req, $cb, $arg) = splice(@$inflight_c, 0, 3);
+	my ($req, $cb, $arg) = @$inflight_c[0, 1, 2];
 	my $rbuf = delete($self->{rbuf_c}) // \(my $new = '');
 	chomp(my $line = my_readline($self->{in_c}, $rbuf));
 	my ($hex, $type, $size) = split(/ /, $line);
@@ -273,6 +274,7 @@ sub check_async_step ($$) {
 		$self->fail(defined($ret) ? 'read EOF' : "read: $!") if !$ret;
 	}
 	$self->{rbuf_c} = $rbuf if $$rbuf ne '';
+	splice(@$inflight_c, 0, 3); # don't retry $cb on ->fail
 	eval { $cb->($hex, $type, $size, $arg, $self) };
 	async_err($self, $req, $hex, $@, 'check') if $@;
 }

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 4/7] git: cat-file --batch are their own pgrp
  2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
                       ` (2 preceding siblings ...)
  2021-10-14 13:16     ` [PATCH 3/7] git: ->fail invokes current callback Eric Wong
@ 2021-10-14 13:16     ` Eric Wong
  2021-10-14 13:16     ` [PATCH 5/7] lei: TSTP affects all curl and related subprocesses Eric Wong
                       ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14 13:16 UTC (permalink / raw)
  To: meta

We want these long-lived processes to die naturally when their
parent dies.  Hopefully this improves graceful shutdown for
-extindex because I'm interrupting a lot of reindexing...
---
 lib/PublicInbox/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 37342d7d10a4..e634ca55fd1f 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -114,7 +114,7 @@ sub _bidi_pipe {
 		return;
 	}
 	pipe(my ($out_r, $out_w)) or $self->fail("pipe failed: $!");
-	my $rdr = { 0 => $out_r };
+	my $rdr = { 0 => $out_r, pgid => 0 };
 	my $gd = $self->{git_dir};
 	if ($gd =~ s!/([^/]+/[^/]+)\z!/!) {
 		$rdr->{-C} = $gd;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 5/7] lei: TSTP affects all curl and related subprocesses
  2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
                       ` (3 preceding siblings ...)
  2021-10-14 13:16     ` [PATCH 4/7] git: cat-file --batch are their own pgrp Eric Wong
@ 2021-10-14 13:16     ` Eric Wong
  2021-10-14 13:16     ` [PATCH 6/7] lei up: actually rely on DESTROY for --alllll Eric Wong
  2021-10-14 13:16     ` [PATCH 7/7] lei up --all: send signals to workers, receive errors Eric Wong
  6 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14 13:16 UTC (permalink / raw)
  To: meta

By relying more on pgroups for remaining remaining processes,
this lets us pause all curl+tail subprocesses with a single
kill(2) to avoid cluttering stderr.

We won't bother pausing the pigz/gzip/bzip2/xz compressor
process not cat-file processes, though, since those don't write
to the terminal and they idle soon after the workers react to
SIGSTOP.

AutoReap is hoisted out from TestCommon.pm.  CLONE_SKIP
is gone since we won't be using Perl threads any time
soon (they're discouraged by the maintainers of Perl).
---
 MANIFEST                      |  1 +
 lib/PublicInbox/AutoReap.pm   | 34 +++++++++++++++++++++++++++++++++
 lib/PublicInbox/LEI.pm        |  7 +------
 lib/PublicInbox/LeiInput.pm   |  7 ++++---
 lib/PublicInbox/LeiMirror.pm  |  8 +++-----
 lib/PublicInbox/LeiRemote.pm  | 13 +++++--------
 lib/PublicInbox/LeiToMail.pm  |  2 +-
 lib/PublicInbox/LeiXSearch.pm | 20 +++++++++----------
 lib/PublicInbox/TestCommon.pm | 36 +++--------------------------------
 9 files changed, 61 insertions(+), 67 deletions(-)
 create mode 100644 lib/PublicInbox/AutoReap.pm

diff --git a/MANIFEST b/MANIFEST
index 122ceda0a761..b89513d5afb5 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -147,6 +147,7 @@ lib/PublicInbox/AddressPP.pm
 lib/PublicInbox/Admin.pm
 lib/PublicInbox/AdminEdit.pm
 lib/PublicInbox/AltId.pm
+lib/PublicInbox/AutoReap.pm
 lib/PublicInbox/Cgit.pm
 lib/PublicInbox/CmdIPC4.pm
 lib/PublicInbox/CompressNoop.pm
diff --git a/lib/PublicInbox/AutoReap.pm b/lib/PublicInbox/AutoReap.pm
new file mode 100644
index 000000000000..23ecce772186
--- /dev/null
+++ b/lib/PublicInbox/AutoReap.pm
@@ -0,0 +1,34 @@
+# Copyright (C) all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+
+# automatically kill + reap children when this goes out-of-scope
+package PublicInbox::AutoReap;
+use v5.10.1;
+use strict;
+
+sub new {
+	my (undef, $pid, $cb) = @_;
+	bless { pid => $pid, cb => $cb, owner => $$ }, __PACKAGE__
+}
+
+sub kill {
+	my ($self, $sig) = @_;
+	CORE::kill($sig // 'TERM', $self->{pid});
+}
+
+sub join {
+	my ($self, $sig) = @_;
+	my $pid = delete $self->{pid} or return;
+	$self->{cb}->() if defined $self->{cb};
+	CORE::kill($sig, $pid) if defined $sig;
+	my $ret = waitpid($pid, 0) // die "waitpid($pid): $!";
+	$ret == $pid or die "BUG: waitpid($pid) != $ret";
+}
+
+sub DESTROY {
+	my ($self) = @_;
+	return if $self->{owner} != $$;
+	$self->join('TERM');
+}
+
+1;
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 9620e2642213..d0905562f616 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -516,12 +516,6 @@ sub sigpipe_handler { # handles SIGPIPE from @WQ_KEYS workers
 	fail_handler($_[0], 13, delete $_[0]->{1});
 }
 
-# PublicInbox::OnDestroy callback for SIGINT to take out the entire pgid
-sub sigint_reap {
-	my ($pgid) = @_;
-	dwaitpid($pgid) if kill('-INT', $pgid);
-}
-
 sub fail ($$;$) {
 	my ($self, $buf, $exit_code) = @_;
 	local $current_lei = $self;
@@ -600,6 +594,7 @@ sub _lei_atfork_child {
 			$cb->(@_) unless PublicInbox::Eml::warn_ignore(@_)
 		};
 	}
+	$SIG{TERM} = sub { exit(128 + 15) };
 	$current_lei = $persist ? undef : $self; # for SIG{__WARN__}
 }
 
diff --git a/lib/PublicInbox/LeiInput.pm b/lib/PublicInbox/LeiInput.pm
index 6a90e7e1e756..dd40d83840c5 100644
--- a/lib/PublicInbox/LeiInput.pm
+++ b/lib/PublicInbox/LeiInput.pm
@@ -8,6 +8,7 @@ use v5.10.1;
 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
@@ -102,13 +103,13 @@ sub handle_http_input ($$@) {
 	push @$curl, '-s', @$curl_opt;
 	my $cmd = $curl->for_uri($lei, $uri);
 	$lei->qerr("# $cmd");
-	my $rdr = { 2 => $lei->{2}, pgid => 0 };
-	my ($fh, $pid) = popen_rd($cmd, undef, $rdr);
+	my ($fh, $pid) = popen_rd($cmd, undef, { 2 => $lei->{2} });
+	my $ar = PublicInbox::AutoReap->new($pid);
 	grep(/\A--compressed\z/, @$curl) or
 		$fh = IO::Uncompress::Gunzip->new($fh, MultiStream => 1);
 	eval { $self->input_fh('mboxrd', $fh, $url, @args) };
 	my $err = $@;
-	waitpid($pid, 0);
+	$ar->join;
 	$? || $err and
 		$lei->child_error($?, "@$cmd failed".$err ? " $err" : '');
 }
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index f1bc82e27205..a75c99c4987f 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -7,6 +7,7 @@ use strict;
 use v5.10.1;
 use parent qw(PublicInbox::IPC);
 use PublicInbox::Config;
+use PublicInbox::AutoReap;
 use IO::Uncompress::Gunzip qw(gunzip $GunzipError);
 use IO::Compress::Gzip qw(gzip $GzipError);
 use PublicInbox::Spawn qw(popen_rd spawn);
@@ -192,10 +193,8 @@ sub index_cloned_inbox {
 sub run_reap {
 	my ($lei, $cmd, $opt) = @_;
 	$lei->qerr("# @$cmd");
-	my $pid = spawn($cmd, undef, $opt);
-	my $reap = PublicInbox::OnDestroy->new($lei->can('sigint_reap'), $pid);
-	waitpid($pid, 0) == $pid or die "waitpid @$cmd: $!";
-	@$reap = (); # cancel reap
+	my $ar = PublicInbox::AutoReap->new(spawn($cmd, undef, $opt));
+	$ar->join;
 	my $ret = $?;
 	$? = 0; # don't let it influence normal exit
 	$ret;
@@ -459,7 +458,6 @@ sub start {
 sub ipc_atfork_child {
 	my ($self) = @_;
 	$self->{lei}->_lei_atfork_child;
-	$SIG{TERM} = sub { exit(128 + 15) }; # trigger OnDestroy $reap
 	$self->SUPER::ipc_atfork_child;
 }
 
diff --git a/lib/PublicInbox/LeiRemote.pm b/lib/PublicInbox/LeiRemote.pm
index 7782aa9dbfa1..54750062fd5f 100644
--- a/lib/PublicInbox/LeiRemote.pm
+++ b/lib/PublicInbox/LeiRemote.pm
@@ -9,10 +9,10 @@ package PublicInbox::LeiRemote;
 use v5.10.1;
 use strict;
 use IO::Uncompress::Gunzip;
-use PublicInbox::OnDestroy;
 use PublicInbox::MboxReader;
 use PublicInbox::Spawn qw(popen_rd);
 use PublicInbox::LeiCurl;
+use PublicInbox::AutoReap;
 use PublicInbox::ContentHash qw(git_sha);
 
 sub new {
@@ -47,17 +47,14 @@ 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 $rdr = { 2 => $lei->{2}, pgid => 0 };
-	my ($fh, $pid) = popen_rd($cmd, undef, $rdr);
-	my $reap = PublicInbox::OnDestroy->new($lei->can('sigint_reap'), $pid);
+	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 $err = waitpid($pid, 0) == $pid ? undef
-					: "BUG: waitpid($cmd): $!";
-	@$reap = (); # cancel OnDestroy
 	my $wait = $self->{lei}->{sto}->wq_do('done');
-	die $err if $err;
+	$ar->join;
+	$lei->child_error($?) if $?;
 	$self; # we are the mset (and $ibx, and $self)
 }
 
diff --git a/lib/PublicInbox/LeiToMail.pm b/lib/PublicInbox/LeiToMail.pm
index 5a220ba39735..9c748deaed16 100644
--- a/lib/PublicInbox/LeiToMail.pm
+++ b/lib/PublicInbox/LeiToMail.pm
@@ -157,7 +157,7 @@ sub _post_augment_mbox { # open a compressor process from top-level process
 	my $zsfx = $self->{zsfx} or return;
 	my $cmd = PublicInbox::MboxReader::zsfx2cmd($zsfx, undef, $lei);
 	my ($r, $w) = @{delete $lei->{zpipe}};
-	my $rdr = { 0 => $r, 1 => $lei->{1}, 2 => $lei->{2} };
+	my $rdr = { 0 => $r, 1 => $lei->{1}, 2 => $lei->{2}, pgid => 0 };
 	my $pid = spawn($cmd, undef, $rdr);
 	my $pp = gensym;
 	my $dup = bless { "pid.$pid" => $cmd }, ref($lei);
diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index 668d0b6e5df3..fba168613d96 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -15,6 +15,7 @@ use PublicInbox::Search qw(xap_terms);
 use PublicInbox::Spawn qw(popen_rd spawn which);
 use PublicInbox::MID qw(mids);
 use PublicInbox::Smsg;
+use PublicInbox::AutoReap;
 use PublicInbox::Eml;
 use PublicInbox::LEI;
 use Fcntl qw(SEEK_SET F_SETFL O_APPEND O_RDWR);
@@ -346,18 +347,17 @@ sub query_remote_mboxrd {
 	my @qform = (x => 'm');
 	push(@qform, t => 1) if $opt->{threads};
 	my $verbose = $opt->{verbose};
-	my ($reap_tail, $reap_curl);
+	my $reap_tail;
 	my $cerr = File::Temp->new(TEMPLATE => 'curl.err-XXXX', TMPDIR => 1);
 	fcntl($cerr, F_SETFL, O_APPEND|O_RDWR) or warn "set O_APPEND: $!";
-	my $rdr = { 2 => $cerr, pgid => 0 };
-	my $sigint_reap = $lei->can('sigint_reap');
+	my $rdr = { 2 => $cerr };
 	if ($verbose) {
 		# spawn a process to force line-buffering, otherwise curl
 		# will write 1 character at-a-time and parallel outputs
 		# mmmaaayyy llloookkk llliiikkkeee ttthhhiiisss
-		my $o = { 1 => $lei->{2}, 2 => $lei->{2}, pgid => 0 };
+		my $o = { 1 => $lei->{2}, 2 => $lei->{2} };
 		my $pid = spawn(['tail', '-f', $cerr->filename], undef, $o);
-		$reap_tail = PublicInbox::OnDestroy->new($sigint_reap, $pid);
+		$reap_tail = PublicInbox::AutoReap->new($pid);
 	}
 	my $curl = PublicInbox::LeiCurl->new($lei, $self->{curl}) or return;
 	push @$curl, '-s', '-d', '';
@@ -372,16 +372,13 @@ sub query_remote_mboxrd {
 		my $cmd = $curl->for_uri($lei, $uri);
 		$lei->qerr("# $cmd");
 		my ($fh, $pid) = popen_rd($cmd, undef, $rdr);
-		$reap_curl = PublicInbox::OnDestroy->new($sigint_reap, $pid);
+		my $reap_curl = PublicInbox::AutoReap->new($pid);
 		$fh = IO::Uncompress::Gunzip->new($fh, MultiStream => 1);
 		PublicInbox::MboxReader->mboxrd($fh, \&each_remote_eml, $self,
 						$lei, $each_smsg);
-		my $err = waitpid($pid, 0) == $pid ? undef
-						: "BUG: waitpid($cmd): $!";
-		@$reap_curl = (); # cancel OnDestroy
-		die $err if $err;
 		my $nr = $lei->{-nr_remote_eml};
 		my $wait = $lei->{sto}->wq_do('done') if $nr && $lei->{sto};
+		$reap_curl->join;
 		if ($? == 0) {
 			# don't update if no results, maybe MTA is down
 			$key && $nr and
@@ -389,7 +386,7 @@ sub query_remote_mboxrd {
 			mset_progress($lei, $lei->{-current_url}, $nr, $nr);
 			next;
 		}
-		$err = '';
+		my $err;
 		if (-s $cerr) {
 			seek($cerr, 0, SEEK_SET) //
 					warn "seek($cmd stderr): $!";
@@ -397,6 +394,7 @@ sub query_remote_mboxrd {
 					warn "read($cmd stderr): $!";
 			truncate($cerr, 0) // warn "truncate($cmd stderr): $!";
 		}
+		$err //= '';
 		next if (($? >> 8) == 22 && $err =~ /\b404\b/);
 		$uri->query_form(q => $qstr);
 		$lei->child_error($?, "E: <$uri> $err");
diff --git a/lib/PublicInbox/TestCommon.pm b/lib/PublicInbox/TestCommon.pm
index 57f1db952e49..835779996d56 100644
--- a/lib/PublicInbox/TestCommon.pm
+++ b/lib/PublicInbox/TestCommon.pm
@@ -6,6 +6,7 @@ package PublicInbox::TestCommon;
 use strict;
 use parent qw(Exporter);
 use v5.10.1;
+use PublicInbox::AutoReap;
 use Fcntl qw(FD_CLOEXEC F_SETFD F_GETFD :seek);
 use POSIX qw(dup2);
 use IO::Socket::INET;
@@ -429,7 +430,7 @@ sub tail_f (@) {
 	require PublicInbox::Spawn;
 	my $pid = PublicInbox::Spawn::spawn($cmd, undef, { 1 => 2 });
 	wait_for_tail($pid, scalar @_);
-	PublicInboxTestProcess->new($pid, \&wait_for_tail);
+	PublicInbox::AutoReap->new($pid, \&wait_for_tail);
 }
 
 sub start_script {
@@ -492,7 +493,7 @@ sub start_script {
 			die "FAIL: ",join(' ', $key, @argv), ": $!\n";
 		}
 	}
-	my $td = PublicInboxTestProcess->new($pid);
+	my $td = PublicInbox::AutoReap->new($pid);
 	$td->{-extra} = $tail;
 	$td;
 }
@@ -742,37 +743,6 @@ sub test_httpd ($$;$) {
 };
 
 
-package PublicInboxTestProcess;
-use strict;
-
-# prevent new threads from inheriting these objects
-sub CLONE_SKIP { 1 }
-
-sub new {
-	my ($cls, $pid, $cb) = @_;
-	bless { pid => $pid, cb => $cb, owner => $$ }, $cls;
-}
-
-sub kill {
-	my ($self, $sig) = @_;
-	CORE::kill($sig // 'TERM', $self->{pid});
-}
-
-sub join {
-	my ($self, $sig) = @_;
-	my $pid = delete $self->{pid} or return;
-	$self->{cb}->() if defined $self->{cb};
-	CORE::kill($sig, $pid) if defined $sig;
-	my $ret = waitpid($pid, 0) // die "waitpid($pid): $!";
-	$ret == $pid or die "waitpid($pid) != $ret";
-}
-
-sub DESTROY {
-	my ($self) = @_;
-	return if $self->{owner} != $$;
-	$self->join('TERM');
-}
-
 package PublicInbox::TestCommon::InboxWakeup;
 use strict;
 sub on_inbox_unlock { ${$_[0]}->($_[1]) }

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 6/7] lei up: actually rely on DESTROY for --alllll
  2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
                       ` (4 preceding siblings ...)
  2021-10-14 13:16     ` [PATCH 5/7] lei: TSTP affects all curl and related subprocesses Eric Wong
@ 2021-10-14 13:16     ` Eric Wong
  2021-10-14 13:16     ` [PATCH 7/7] lei up --all: send signals to workers, receive errors Eric Wong
  6 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14 13:16 UTC (permalink / raw)
  To: meta

We need to use DESTROY here to ensure we wait for workers, too;
not just the initial dispatch.

Fixes: cafbd77b3c82167d ("lei up: avoid excessively parallel --all")
---
 lib/PublicInbox/LeiUp.pm | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/LeiUp.pm b/lib/PublicInbox/LeiUp.pm
index 3011300dd836..719736e8597e 100644
--- a/lib/PublicInbox/LeiUp.pm
+++ b/lib/PublicInbox/LeiUp.pm
@@ -63,6 +63,7 @@ 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(
@@ -158,18 +159,22 @@ sub event_step { # runs via PublicInbox::DS::requeue
 	$l->{opt} = { %{$l->{opt}} }; # deep copy
 	delete $l->{opt}->{all};
 	$l->qerr("# updating $self->{out}");
-	$l->{up_op_p} = $self->{op_p}; # ($l => $lei => script/lei)
+	my $o = " (output: $self->{out})"; # add to all warnings
 	my $cb = $SIG{__WARN__} // \&CORE::warn;
-	my $o = " (output: $self->{out})";
 	local $SIG{__WARN__} = sub {
 		my @m = @_;
 		push(@m, $o) if !@m || $m[-1] !~ s/\n\z/$o\n/;
 		$cb->(@m);
 	};
+	$l->{-up1} = $self;
 	eval { $l->dispatch('up', $self->{out}) };
 	$lei->child_error(0, $@) if $@ || $l->{failed}; # lei->fail()
+}
 
-	# onto the next:
+sub DESTROY {
+	my ($self) = @_;
+	my $lei = $self->{lei}; # the original, from lei_up
+	return if $lei->{daemon_pid} != $$;
 	my $out = shift(@{$lei->{-upq}}) or return;
 	PublicInbox::DS::requeue(nxt($lei, $out, $self->{op_p}));
 }

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 7/7] lei up --all: send signals to workers, receive errors
  2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
                       ` (5 preceding siblings ...)
  2021-10-14 13:16     ` [PATCH 6/7] lei up: actually rely on DESTROY for --alllll Eric Wong
@ 2021-10-14 13:16     ` Eric Wong
  6 siblings, 0 replies; 12+ messages in thread
From: Eric Wong @ 2021-10-14 13:16 UTC (permalink / raw)
  To: meta

The redispatch mechanism wasn't routing signals and messages
between redispatched workers and script/lei properly.  We now
rely on PktOp to do bidirectional message forwarding and
carefully avoiding circular references by using PktOp.
---
 lib/PublicInbox/LEI.pm   |  7 ++++++-
 lib/PublicInbox/LeiUp.pm | 13 ++++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index d0905562f616..b6338377328f 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -573,6 +573,7 @@ sub _lei_atfork_child {
 		POSIX::setpgid(0, $$) // die "setpgid(0, $$): $!";
 	}
 	close($_) for (grep(defined, delete @$self{qw(3 old_1 au_done)}));
+	delete $self->{-socks};
 	if (my $op_c = delete $self->{pkt_op_c}) {
 		close(delete $op_c->{sock});
 	}
@@ -1144,7 +1145,9 @@ sub event_step {
 		if ($buf eq '') {
 			_drop_wq($self); # EOF, client disconnected
 			dclose($self);
-		} elsif ($buf =~ /\A(?:STOP|CONT)\z/) {
+			$buf = 'TERM';
+		}
+		if ($buf =~ /\A(?:STOP|CONT|TERM)\z/) {
 			my $sig = "-$buf";
 			for my $wq (grep(defined, @$self{@WQ_KEYS})) {
 				$wq->wq_kill($sig) or $wq->wq_kill_old($sig);
@@ -1152,6 +1155,8 @@ sub event_step {
 		} else {
 			die "unrecognized client signal: $buf";
 		}
+		my $s = $self->{-socks} // []; # lei up --all
+		@$s = grep { send($_, $buf, MSG_EOR) } @$s;
 	};
 	if (my $err = $@) {
 		eval { $self->fail($err) };
diff --git a/lib/PublicInbox/LeiUp.pm b/lib/PublicInbox/LeiUp.pm
index 719736e8597e..df65cb9b8474 100644
--- a/lib/PublicInbox/LeiUp.pm
+++ b/lib/PublicInbox/LeiUp.pm
@@ -166,7 +166,15 @@ sub event_step { # runs via PublicInbox::DS::requeue
 		push(@m, $o) if !@m || $m[-1] !~ s/\n\z/$o\n/;
 		$cb->(@m);
 	};
-	$l->{-up1} = $self;
+	$l->{-up1} = $self; # for LeiUp1->DESTROY
+	delete @$l{qw(-socks -event_init_done)};
+	my ($op_c, $op_p) = PublicInbox::PktOp->pair;
+	$self->{unref_on_destroy} = $op_c->{sock}; # to cleanup $lei->{-socks}
+	$lei->pkt_ops($op_c->{ops} //= {}); # errors from $l -> script/lei
+	push @{$lei->{-socks}}, $op_c->{sock}; # script/lei signals to $l
+	$l->{sock} = $op_p->{op_p}; # receive signals from op_c->{sock}
+	$op_c = $op_p = undef;
+
 	eval { $l->dispatch('up', $self->{out}) };
 	$lei->child_error(0, $@) if $@ || $l->{failed}; # lei->fail()
 }
@@ -175,6 +183,9 @@ sub DESTROY {
 	my ($self) = @_;
 	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;
 	my $out = shift(@{$lei->{-upq}}) or return;
 	PublicInbox::DS::requeue(nxt($lei, $out, $self->{op_p}));
 }

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-10-14 13:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14  4:32 [PATCH 0/3] clone+fetch stuff Eric Wong
2021-10-14  4:32 ` [PATCH 1/3] clone+fetch: respect umask for all downloaded files Eric Wong
2021-10-14  4:32 ` [PATCH 2/3] lei add-external --mirror: respect client umask Eric Wong
2021-10-14  4:32 ` [PATCH 3/3] lei: give workers their own process group Eric Wong
2021-10-14 13:16   ` [PATCH 0/7] lei: more process handling fixes Eric Wong
2021-10-14 13:16     ` [PATCH 1/7] lei: use send() perlop for signals Eric Wong
2021-10-14 13:16     ` [PATCH 2/7] git: async_err shows retried requests properly Eric Wong
2021-10-14 13:16     ` [PATCH 3/7] git: ->fail invokes current callback Eric Wong
2021-10-14 13:16     ` [PATCH 4/7] git: cat-file --batch are their own pgrp Eric Wong
2021-10-14 13:16     ` [PATCH 5/7] lei: TSTP affects all curl and related subprocesses Eric Wong
2021-10-14 13:16     ` [PATCH 6/7] lei up: actually rely on DESTROY for --alllll Eric Wong
2021-10-14 13:16     ` [PATCH 7/7] lei up --all: send signals to workers, receive errors Eric Wong

Code repositories for project(s) associated with this 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).