[PATCH 0/2] graceful shutdown tweaks

[PATCH 0/2] graceful shutdown tweaks

[Eric Wong]

We want to reduce the chance that an inadvertant signal
could kill a child we depend while we are gracefully shutting
down.

Tested via long-running "git clone --mirror" on a 800MB inbox.
Unfortunately, we will still have to tweak worker processes
a bit and have them avoid receiving signals from the master
and instead fake signals via pipes.

Eric Wong (2):
      spawn: try to keep signals blocked in spawned child
      daemon: be less misleading about graceful shutdown

 lib/PublicInbox/Daemon.pm      | 3 ++-
 lib/PublicInbox/HTTPD/Async.pm | 3 ---
 lib/PublicInbox/Spawn.pm       | 8 +++++---
 lib/PublicInbox/SpawnPP.pm     | 7 ++++++-
 t/spawn.t                      | 2 +-
 5 files changed, 14 insertions(+), 9 deletions(-)

[PATCH 1/2] spawn: try to keep signals blocked in spawned child

[Eric Wong]

While we only want to stop our daemons and gracefully destroy
subprocesses, it is common for 'Ctrl-C' from a terminal to kill
the entire pgroup.

Killing an entire pgroup nukes subprocesses like git-upload-pack
breaks graceful shutdown on long clones.  Make a best effort to
ensure git-upload-pack processes are not broken when somebody
signals an entire process group.

Followup-to: commit 37bf2db81bbbe114d7fc5a00e30d3d5a6fa74de5
("doc: systemd examples should only kill one process")
---
 lib/PublicInbox/Spawn.pm   | 8 +++++---
 lib/PublicInbox/SpawnPP.pm | 7 ++++++-
 t/spawn.t                  | 2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index 66dce33..8373030 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -104,9 +104,11 @@ int public_inbox_fork_exec(int in, int out, int err,
 		REDIR(out, 1);
 		REDIR(err, 2);
 		for (sig = 1; sig < NSIG; sig++)
-			signal(sig, SIG_DFL); /* ignore errorrs on signals */
-		ret = sigprocmask(SIG_SETMASK, &old, NULL);
-		if (ret != 0) xerr("sigprocmask failed in vfork child");
+			signal(sig, SIG_DFL); /* ignore errors on signals */
+		/*
+		 * don't bother unblocking, we don't want signals
+		 * to the group taking out a subprocess
+		 */
 		execve(filename, argv, envp);
 		xerr("execve failed");
 	}
diff --git a/lib/PublicInbox/SpawnPP.pm b/lib/PublicInbox/SpawnPP.pm
index fe95d12..36223e8 100644
--- a/lib/PublicInbox/SpawnPP.pm
+++ b/lib/PublicInbox/SpawnPP.pm
@@ -3,11 +3,15 @@
 package PublicInbox::SpawnPP;
 use strict;
 use warnings;
-use POSIX qw(dup2);
+use POSIX qw(dup2 :signal_h);
 
 # Pure Perl implementation for folks that do not use Inline::C
 sub public_inbox_fork_exec ($$$$$$) {
 	my ($in, $out, $err, $f, $cmd, $env) = @_;
+	my $old = POSIX::SigSet->new();
+	my $set = POSIX::SigSet->new();
+	$set->fillset or die "fillset failed: $!";
+	sigprocmask(SIG_SETMASK, $set, $old) or die "can't block signals: $!";
 	my $pid = fork;
 	if ($pid == 0) {
 		if ($in != 0) {
@@ -29,6 +33,7 @@ sub public_inbox_fork_exec ($$$$$$) {
 			die "exec $cmd->[0] failed: $!\n";
 		}
 	}
+	sigprocmask(SIG_SETMASK, $old) or die "can't unblock signals: $!";
 	$pid;
 }
 
diff --git a/t/spawn.t b/t/spawn.t
index 9e58f67..0f75646 100644
--- a/t/spawn.t
+++ b/t/spawn.t
@@ -87,7 +87,7 @@ use PublicInbox::Spawn qw(which spawn popen_rd);
 	is(kill(0, $pid), 1, 'child process is running');
 	ok(!defined(sysread($fh, my $buf, 1)) && $!{EAGAIN},
 	   'sysread returned quickly with EAGAIN');
-	is(kill(15, $pid), 1, 'child process killed early');
+	is(kill(9, $pid), 1, 'child process killed early');
 	is(waitpid($pid, 0), $pid, 'child process reapable');
 	isnt($?, 0, '$? set properly: '.$?);
 }

[PATCH 2/2] daemon: be less misleading about graceful shutdown

[Eric Wong]

We do not need to count the httpd.async object
against our running client count, that is tied to
the socket of the actual client.

This prevents misleading sysadmins about connected
clients during shutdown.
---
 lib/PublicInbox/Daemon.pm      | 3 ++-
 lib/PublicInbox/HTTPD/Async.pm | 3 ---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index b76b9ff..a25dd90 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -180,7 +180,8 @@ sub worker_quit {
 		my $n = 0;
 
 		foreach my $s (values %$dmap) {
-			if ($s->can('busy') && $s->busy) {
+			$s->can('busy') or next;
+			if ($s->busy) {
 				++$n;
 			} else {
 				# close as much as possible, early as possible
diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm
index fadf2d3..a936d9b 100644
--- a/lib/PublicInbox/HTTPD/Async.pm
+++ b/lib/PublicInbox/HTTPD/Async.pm
@@ -74,7 +74,4 @@ sub close {
 	PublicInbox::EvCleanup::asap($cleanup) if $cleanup;
 }
 
-# do not let ourselves be closed during graceful termination
-sub busy () { $_[0]->{cb} }
-
 1;