[PATCH 0/3] there is no END{}

[PATCH 0/3] there is no END{}

[Eric Wong]

END{} blocks make code harder to test and reuse in long-lived
processes.  Get rid of them in preparation for allowing daemons
to be restarted in tests without new processes.  This may make
our code more multiplicity-friendly in the future :>

Eric Wong (3):
  ds: move EvCleanup code into DS
  ds: move NNTP-only expiration code into DS
  daemon: use DESTROY for unlinking --pid-file

 MANIFEST                     |  1 -
 lib/PublicInbox/DS.pm        | 64 +++++++++++++++++++++++++++++++++---
 lib/PublicInbox/Daemon.pm    | 27 +++++++--------
 lib/PublicInbox/EvCleanup.pm | 30 -----------------
 lib/PublicInbox/HTTP.pm      |  1 -
 lib/PublicInbox/Inbox.pm     |  6 ++--
 lib/PublicInbox/NNTP.pm      | 49 +++------------------------
 t/httpd-unix.t               | 41 ++++++++++++-----------
 8 files changed, 100 insertions(+), 119 deletions(-)
 delete mode 100644 lib/PublicInbox/EvCleanup.pm

[PATCH 1/3] ds: move EvCleanup code into DS

[Eric Wong]

EvCleanup only existed since Danga::Socket was a separate
component, and cleanup code belongs with the event loop.
---
 MANIFEST                     |  1 -
 lib/PublicInbox/DS.pm        | 27 ++++++++++++++++++++++-----
 lib/PublicInbox/Daemon.pm    |  2 --
 lib/PublicInbox/EvCleanup.pm | 30 ------------------------------
 lib/PublicInbox/HTTP.pm      |  1 -
 lib/PublicInbox/Inbox.pm     |  6 +++---
 lib/PublicInbox/NNTP.pm      |  5 ++---
 7 files changed, 27 insertions(+), 45 deletions(-)
 delete mode 100644 lib/PublicInbox/EvCleanup.pm

diff --git a/MANIFEST b/MANIFEST
index 044bbfef..edbbbfad 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -92,7 +92,6 @@ lib/PublicInbox/DSKQXS.pm
 lib/PublicInbox/DSPoll.pm
 lib/PublicInbox/Daemon.pm
 lib/PublicInbox/Emergency.pm
-lib/PublicInbox/EvCleanup.pm
 lib/PublicInbox/ExtMsg.pm
 lib/PublicInbox/Feed.pm
 lib/PublicInbox/Filter/Base.pm
diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 301ec057..7eb0aada 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -41,7 +41,8 @@ require File::Spec;
 
 my $nextq; # queue for next_tick
 my $WaitPids; # list of [ pid, callback, callback_arg ]
-my $reap_timer;
+my $later_queue; # callbacks
+my ($later_timer, $reap_timer);
 our (
      %DescriptorMap,             # fd (num) -> PublicInbox::DS object
      $Epoll,                     # Global epoll fd (or DSKQXS ref)
@@ -71,7 +72,8 @@ sub Reset {
     %DescriptorMap = ();
     $nextq = [];
     $WaitPids = [];
-    $reap_timer = undef;
+    $later_queue = [];
+    $reap_timer = $later_timer = undef;
     @ToClose = ();
     $LoopTimeout = -1;  # no timeout by default
     @Timers = ();
@@ -250,9 +252,11 @@ sub reap_pids {
 # reentrant SIGCHLD handler (since reap_pids is not reentrant)
 sub enqueue_reap ($) { push @$nextq, \&reap_pids };
 
+sub running () { ($SIG{CHLD} // '') eq \&enqueue_reap }
+
 sub EpollEventLoop {
     local $in_loop = 1;
-    while (1) {
+    do {
         my @events;
         my $i;
         my $timeout = RunTimers();
@@ -266,8 +270,8 @@ sub EpollEventLoop {
             # in that event.
             $DescriptorMap{$events[$i]->[0]}->event_step;
         }
-        return unless PostEventLoop();
-    }
+    } while (PostEventLoop());
+    _run_later();
 }
 
 =head2 C<< CLASS->SetPostLoopCallback( CODEREF ) >>
@@ -640,6 +644,19 @@ sub dwaitpid ($$$) {
     }
 }
 
+sub _run_later () {
+    my $run = $later_queue;
+    $later_timer = undef;
+    $later_queue = [];
+    $_->() for @$run;
+}
+
+sub later ($) {
+    my ($cb) = @_;
+    push @$later_queue, $cb;
+    $later_timer //= AddTimer(undef, 60, \&_run_later);
+}
+
 package PublicInbox::DS::Timer;
 # [$abs_float_firetime, $coderef];
 sub cancel {
diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index c7a71ba0..c2c05b96 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -16,7 +16,6 @@ STDOUT->autoflush(1);
 STDERR->autoflush(1);
 use PublicInbox::DS qw(now);
 use PublicInbox::Syscall qw(SFD_NONBLOCK);
-require PublicInbox::EvCleanup;
 require PublicInbox::Listener;
 require PublicInbox::ParentPipe;
 require PublicInbox::Sigfd;
@@ -580,7 +579,6 @@ sub defer_accept ($$) {
 
 sub daemon_loop ($$$$) {
 	my ($refresh, $post_accept, $nntpd, $af_default) = @_;
-	PublicInbox::EvCleanup::enable(); # early for $refresh
 	my %post_accept;
 	while (my ($k, $v) = each %tls_opt) {
 		if ($k =~ s!\A(?:nntps|https)://!!) {
diff --git a/lib/PublicInbox/EvCleanup.pm b/lib/PublicInbox/EvCleanup.pm
deleted file mode 100644
index be6672ed..00000000
--- a/lib/PublicInbox/EvCleanup.pm
+++ /dev/null
@@ -1,30 +0,0 @@
-# Copyright (C) 2016-2019 all contributors <meta@public-inbox.org>
-# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
-
-# event cleanups (for PublicInbox::DS)
-package PublicInbox::EvCleanup;
-use strict;
-use warnings;
-require PublicInbox::DS;
-
-# this only runs under public-inbox-{httpd/nntpd}, not generic PSGI servers
-my $ENABLED;
-sub enabled { $ENABLED }
-sub enable { $ENABLED = 1 }
-my $laterq = [ [], undef ];
-
-sub _run_later () {
-	my $run = $laterq->[0];
-	$laterq->[0] = [];
-	$laterq->[1] = undef;
-	$_->() foreach @$run;
-}
-
-sub later ($) {
-	my ($cb) = @_;
-	push @{$laterq->[0]}, $cb;
-	$laterq->[1] ||= PublicInbox::DS->AddTimer(60, *_run_later);
-}
-
-END { _run_later() }
-1;
diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index 009b5ff0..53d50836 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -18,7 +18,6 @@ use Plack::HTTPParser qw(parse_http_request); # XS or pure Perl
 use HTTP::Status qw(status_message);
 use HTTP::Date qw(time2str);
 use IO::Handle;
-require PublicInbox::EvCleanup;
 use PublicInbox::DS qw(msg_more);
 use PublicInbox::Syscall qw(EPOLLIN EPOLLONESHOT);
 use PublicInbox::Tmpfile;
diff --git a/lib/PublicInbox/Inbox.pm b/lib/PublicInbox/Inbox.pm
index 5feb2154..43c6a428 100644
--- a/lib/PublicInbox/Inbox.pm
+++ b/lib/PublicInbox/Inbox.pm
@@ -50,9 +50,9 @@ sub cleanup_task () {
 }
 
 sub cleanup_possible () {
-	# no need to require EvCleanup, here, if it were enabled another
+	# no need to require DS, here, if it were enabled another
 	# module would've require'd it, already
-	eval { PublicInbox::EvCleanup::enabled() } or return 0;
+	eval { PublicInbox::DS::running() } or return 0;
 
 	eval {
 		require Devel::Peek; # needs separate package in Fedora
@@ -65,7 +65,7 @@ sub _cleanup_later ($) {
 	my ($self) = @_;
 	$cleanup_avail = cleanup_possible() if $cleanup_avail < 0;
 	return if $cleanup_avail != 1;
-	$cleanup_timer ||= PublicInbox::EvCleanup::later(*cleanup_task);
+	$cleanup_timer ||= PublicInbox::DS::later(*cleanup_task);
 	$CLEANUP->{"$self"} = $self;
 }
 
diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index c0496852..c9487114 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -11,7 +11,6 @@ use PublicInbox::Search;
 use PublicInbox::Msgmap;
 use PublicInbox::MID qw(mid_escape);
 use PublicInbox::Git;
-require PublicInbox::EvCleanup;
 use Email::Simple;
 use POSIX qw(strftime);
 use PublicInbox::DS qw(now);
@@ -70,7 +69,7 @@ sub expire_old () {
 		}
 	}
 	$EXPMAP = \%new;
-	$expt = scalar(keys %new) ? PublicInbox::EvCleanup::later(*expire_old)
+	$expt = scalar(keys %new) ? PublicInbox::DS::later(*expire_old)
 	                          : undef;
 }
 
@@ -94,7 +93,7 @@ sub new ($$$) {
 		greet($self);
 	}
 	update_idle_time($self);
-	$expt ||= PublicInbox::EvCleanup::later(*expire_old);
+	$expt ||= PublicInbox::DS::later(*expire_old);
 	$self;
 }
 

[PATCH 2/3] ds: move NNTP-only expiration code into DS

[Eric Wong]

We'll be supporting idle timeout for the HTTP code in the
future to deal directly with Internet-exposed clients w/o
Varnish or nginx.
---
 lib/PublicInbox/DS.pm   | 41 +++++++++++++++++++++++++++++++++--
 lib/PublicInbox/NNTP.pm | 48 +++++------------------------------------
 2 files changed, 44 insertions(+), 45 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 7eb0aada..856884bb 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -42,7 +42,9 @@ require File::Spec;
 my $nextq; # queue for next_tick
 my $WaitPids; # list of [ pid, callback, callback_arg ]
 my $later_queue; # callbacks
-my ($later_timer, $reap_timer);
+my $EXPMAP; # fd -> [ idle_time, $self ]
+our $EXPTIME = 180; # 3 minutes
+my ($later_timer, $reap_timer, $exp_timer);
 our (
      %DescriptorMap,             # fd (num) -> PublicInbox::DS object
      $Epoll,                     # Global epoll fd (or DSKQXS ref)
@@ -73,7 +75,8 @@ sub Reset {
     $nextq = [];
     $WaitPids = [];
     $later_queue = [];
-    $reap_timer = $later_timer = undef;
+    $EXPMAP = {};
+    $reap_timer = $later_timer = $exp_timer = undef;
     @ToClose = ();
     $LoopTimeout = -1;  # no timeout by default
     @Timers = ();
@@ -657,6 +660,40 @@ sub later ($) {
     $later_timer //= AddTimer(undef, 60, \&_run_later);
 }
 
+sub expire_old () {
+    my $now = now();
+    my $exp = $EXPTIME;
+    my $old = $now - $exp;
+    my %new;
+    while (my ($fd, $v) = each %$EXPMAP) {
+        my ($idle_time, $ds_obj) = @$v;
+        if ($idle_time < $old) {
+            if (!$ds_obj->shutdn) {
+                $new{$fd} = $v;
+            }
+        } else {
+            $new{$fd} = $v;
+        }
+    }
+    $EXPMAP = \%new;
+    $exp_timer = scalar(keys %new) ? later(\&expire_old) : undef;
+}
+
+sub update_idle_time {
+    my ($self) = @_;
+    my $sock = $self->{sock} or return;
+    $EXPMAP->{fileno($sock)} = [ now(), $self ];
+    $exp_timer //= later(\&expire_old);
+}
+
+sub not_idle_long {
+    my ($self, $now) = @_;
+    my $sock = $self->{sock} or return;
+    my $ary = $EXPMAP->{fileno($sock)} or return;
+    my $exp_at = $ary->[0] + $EXPTIME;
+    $exp_at > $now;
+}
+
 package PublicInbox::DS::Timer;
 # [$abs_float_firetime, $coderef];
 sub cancel {
diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index c9487114..6cd2b84c 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -43,35 +43,6 @@ HDR\r
 OVER\r
 
 my $have_deflate;
-my $EXPMAP; # fd -> [ idle_time, $self ]
-my $expt;
-our $EXPTIME = 180; # 3 minutes
-
-sub update_idle_time ($) {
-	my ($self) = @_;
-	my $sock = $self->{sock} or return;
-	$EXPMAP->{fileno($sock)} = [ now(), $self ];
-}
-
-sub expire_old () {
-	my $now = now();
-	my $exp = $EXPTIME;
-	my $old = $now - $exp;
-	my %new;
-	while (my ($fd, $v) = each %$EXPMAP) {
-		my ($idle_time, $nntp) = @$v;
-		if ($idle_time < $old) {
-			if (!$nntp->shutdn) {
-				$new{$fd} = $v;
-			}
-		} else {
-			$new{$fd} = $v;
-		}
-	}
-	$EXPMAP = \%new;
-	$expt = scalar(keys %new) ? PublicInbox::DS::later(*expire_old)
-	                          : undef;
-}
 
 sub greet ($) { $_[0]->write($_[0]->{nntpd}->{greet}) };
 
@@ -92,8 +63,7 @@ sub new ($$$) {
 	} else {
 		greet($self);
 	}
-	update_idle_time($self);
-	$expt ||= PublicInbox::DS::later(*expire_old);
+	$self->update_idle_time;
 	$self;
 }
 
@@ -650,7 +620,7 @@ sub long_response ($$) {
 			out($self, " deferred[$fd] aborted - %0.6f", $diff);
 			$self->close;
 		} elsif ($more) { # $self->{wbuf}:
-			update_idle_time($self);
+			$self->update_idle_time;
 
 			# COMPRESS users all share the same DEFLATE context.
 			# Flush it here to ensure clients don't see
@@ -983,7 +953,7 @@ sub event_step {
 
 	return unless $self->flush_write && $self->{sock};
 
-	update_idle_time($self);
+	$self->update_idle_time;
 	# only read more requests if we've drained the write buffer,
 	# otherwise we can be buffering infinitely w/o backpressure
 
@@ -1008,25 +978,17 @@ sub event_step {
 	my $len = bytes::length($$rbuf);
 	return $self->close if ($len >= LINE_MAX);
 	$self->rbuf_idle($rbuf);
-	update_idle_time($self);
+	$self->update_idle_time;
 
 	# maybe there's more pipelined data, or we'll have
 	# to register it for socket-readiness notifications
 	$self->requeue unless $self->{wbuf};
 }
 
-sub not_idle_long ($$) {
-	my ($self, $now) = @_;
-	my $sock = $self->{sock} or return;
-	my $ary = $EXPMAP->{fileno($sock)} or return;
-	my $exp_at = $ary->[0] + $EXPTIME;
-	$exp_at > $now;
-}
-
 # for graceful shutdown in PublicInbox::Daemon:
 sub busy {
 	my ($self, $now) = @_;
-	($self->{rbuf} || $self->{wbuf} || not_idle_long($self, $now));
+	($self->{rbuf} || $self->{wbuf} || $self->not_idle_long($now));
 }
 
 # this is an import to prevent "perl -c" from complaining about fields

[PATCH 3/3] daemon: use DESTROY for unlinking --pid-file

[Eric Wong]

This gets rid of the last "END{}" block in our code and cleans
up a (temporary) circular reference.

Furthermore, ensure the cleanup code still works in all
configurations by adding tests and testing both the -W1
(default, 1 worker) and -W0 (no workers) code paths.
---
 lib/PublicInbox/Daemon.pm | 25 ++++++++++++------------
 t/httpd-unix.t            | 41 ++++++++++++++++++++-------------------
 2 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index c2c05b96..dd9e7780 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -28,10 +28,8 @@ my %pids;
 my %listener_names; # sockname => IO::Handle
 my %tls_opt; # scheme://sockname => args for IO::Socket::SSL->start_SSL
 my $reexec_pid;
-my $cleanup;
 my ($uid, $gid);
 my ($default_cert, $default_key);
-END { $cleanup->() if $cleanup };
 my %KNOWN_TLS = ( 443 => 'https', 563 => 'nntps' );
 my %KNOWN_STARTTLS = ( 119 => 'nntp' );
 
@@ -245,14 +243,11 @@ sub daemonize () {
 		die "could not fork: $!\n" unless defined $pid;
 		exit if $pid;
 	}
-	if (defined $pid_file) {
-		write_pid($pid_file);
-		my $unlink_pid = $$;
-		$cleanup = sub {
-			$cleanup = undef; # avoid cyclic reference
-			unlink_pid_file_safe_ish($unlink_pid, $pid_file);
-		};
-	}
+	return unless defined $pid_file;
+
+	write_pid($pid_file);
+	# for ->DESTROY:
+	bless { pid => $$, pid_file => $pid_file }, __PACKAGE__;
 }
 
 sub worker_quit { # $_[0] = signal name or number (unused)
@@ -631,16 +626,16 @@ sub daemon_loop ($$$$) {
 		PublicInbox::DS->SetLoopTimeout(1000);
 	}
 	PublicInbox::DS->EventLoop;
-	$parent_pipe = undef;
 }
 
-
 sub run ($$$;$) {
 	my ($default, $refresh, $post_accept, $nntpd) = @_;
 	daemon_prepare($default);
 	my $af_default = $default =~ /:8080\z/ ? 'httpready' : undef;
-	daemonize();
+	my $for_destroy = daemonize();
 	daemon_loop($refresh, $post_accept, $nntpd, $af_default);
+	PublicInbox::DS->Reset;
+	# ->DESTROY runs when $for_destroy goes out-of-scope
 }
 
 sub do_chown ($) {
@@ -656,4 +651,8 @@ sub write_pid ($) {
 	do_chown($path);
 }
 
+sub DESTROY {
+	unlink_pid_file_safe_ish($_[0]->{pid}, $_[0]->{pid_file});
+}
+
 1;
diff --git a/t/httpd-unix.t b/t/httpd-unix.t
index ceec127c..2c8f8d6b 100644
--- a/t/httpd-unix.t
+++ b/t/httpd-unix.t
@@ -22,7 +22,6 @@ my $td;
 
 my $spawn_httpd = sub {
 	my (@args) = @_;
-	push @args, '-W0';
 	my $cmd = [ '-httpd', @args, "--stdout=$out", "--stderr=$err", $psgi ];
 	$td = start_script($cmd);
 };
@@ -36,7 +35,7 @@ my $spawn_httpd = sub {
 }
 
 ok(!-S $unix, 'UNIX socket does not exist, yet');
-$spawn_httpd->("-l$unix");
+$spawn_httpd->("-l$unix", '-W0');
 my %o = (Peer => $unix, Type => SOCK_STREAM);
 for (1..1000) {
 	last if -S $unix && IO::Socket::UNIX->new(%o);
@@ -87,26 +86,28 @@ check_sock($unix);
 
 SKIP: {
 	eval 'require Net::Server::Daemonize';
-	skip('Net::Server missing for pid-file/daemonization test', 10) if $@;
+	skip('Net::Server missing for pid-file/daemonization test', 20) if $@;
+	my $pid_file = "$tmpdir/pid";
+	for my $w (qw(-W0 -W1)) {
+		# wait for daemonization
+		$spawn_httpd->("-l$unix", '-D', '-P', $pid_file, $w);
+		$td->join;
+		is($?, 0, "daemonized $w process");
+		check_sock($unix);
 
-	# wait for daemonization
-	$spawn_httpd->("-l$unix", '-D', '-P', "$tmpdir/pid");
-	$td->join;
-	is($?, 0, 'daemonized process OK');
-	check_sock($unix);
-
-	ok(-f "$tmpdir/pid", 'pid file written');
-	open my $fh, '<', "$tmpdir/pid" or die "open failed: $!";
-	local $/ = "\n";
-	my $rpid = <$fh>;
-	chomp $rpid;
-	like($rpid, qr/\A\d+\z/s, 'pid file looks like a pid');
-	is(kill('TERM', $rpid), 1, 'signalled daemonized process');
-	for (1..100) {
-		kill(0, $rpid) or last;
-		select undef, undef, undef, 0.02;
+		ok(-f $pid_file, "$w pid file written");
+		open my $fh, '<', "$tmpdir/pid" or die "open failed: $!";
+		my $rpid = do { local $/; <$fh> };
+		chomp $rpid;
+		like($rpid, qr/\A\d+\z/s, "$w pid file looks like a pid");
+		is(kill('TERM', $rpid), 1, "signaled daemonized $w process");
+		for (1..100) {
+			kill(0, $rpid) or last;
+			select undef, undef, undef, 0.02;
+		}
+		is(kill(0, $rpid), 0, "daemonized $w process exited");
+		ok(!-e $pid_file, "$w pid file unlinked at exit");
 	}
-	is(kill(0, $rpid), 0, 'daemonized process exited')
 }
 
 done_testing();