[PATCH 00/11] ds: various cleanups and fixes

[PATCH 00/11] ds: various cleanups and fixes

[Eric Wong]

Start taking advantage of autovivification to simplify our code
and reduce allocations.  Also, I believe it's my first time
using the return value of "push".

Eric Wong (11):
  ds: new: avoid redundant check, make clobbering fatal
  ds: add_timer: rename from AddTimer, remove a parameter
  ds: add an in_loop() function for Inbox.pm use
  ds: remove Timer->cancel and Timer class+bless
  ds: PostEventLoop: guard ToClose against DESTROY side-effects
  ds|http|nntp: simplify {wbuf} population
  ds: rely on autovivification for nextq
  ds: rely on autovivication for waitpid bits
  ds: rely on autovivification for $later_queue
  ds: flatten $EXPMAP, delete entries on close
  sigfd: simplify loop and improve documentation

 lib/PublicInbox/DS.pm    | 200 +++++++++++++++++----------------------
 lib/PublicInbox/HTTP.pm  |   6 +-
 lib/PublicInbox/Inbox.pm |   2 +-
 lib/PublicInbox/NNTP.pm  |  11 +--
 lib/PublicInbox/Sigfd.pm |  13 +--
 t/ds-leak.t              |   2 +-
 6 files changed, 105 insertions(+), 129 deletions(-)

[PATCH 01/11] ds: new: avoid redundant check, make clobbering fatal

[Eric Wong]

"fileno(undef)" already dies under "use strict", so there's no
need to check for it ourselves.  As far as "fileno($closed_io)"
or "fileno($fake_io)" goes, we'll let epoll_ctl detect the
error, instead.

Our design should make DescriptorMap entries impossible to clobber,
so make it fatal via confess in case it does happen, because
inadvertantly clobbering a FD would be very bad.  While we're at
it, remove a redundant return statement and rely on implicit
returns.
---
 lib/PublicInbox/DS.pm | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 09dc3992..00b5dca7 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -337,9 +337,6 @@ sub new {
     $self->{sock} = $sock;
     my $fd = fileno($sock);
 
-    Carp::cluck("undef sock and/or fd in PublicInbox::DS->new.  sock=" . ($sock || "") . ", fd=" . ($fd || ""))
-        unless $sock && $fd;
-
     _InitPoller();
 
     if (epoll_ctl($Epoll, EPOLL_CTL_ADD, $fd, $ev)) {
@@ -349,11 +346,10 @@ sub new {
         }
         die "couldn't add epoll watch for $fd: $!\n";
     }
-    Carp::cluck("PublicInbox::DS::new blowing away existing descriptor map for fd=$fd ($DescriptorMap{$fd})")
-        if $DescriptorMap{$fd};
+    confess("DescriptorMap{$fd} defined ($DescriptorMap{$fd})")
+        if defined($DescriptorMap{$fd});
 
     $DescriptorMap{$fd} = $self;
-    return $self;
 }
 
 

[PATCH 02/11] ds: add_timer: rename from AddTimer, remove a parameter

[Eric Wong]

The class parameter is pointless, especially for an internal
sub which only has one external caller in a test.  Add a sub
prototype while we're at it to get some compile time checking.
---
 lib/PublicInbox/DS.pm | 13 +++++++------
 t/ds-leak.t           |  2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 00b5dca7..217799bb 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -101,16 +101,17 @@ sub SetLoopTimeout {
     return $LoopTimeout = $_[1] + 0;
 }
 
-=head2 C<< CLASS->AddTimer( $seconds, $coderef ) >>
+=head2 C<< PublicInbox::DS::add_timer( $seconds, $coderef ) >>
 
 Add a timer to occur $seconds from now. $seconds may be fractional, but timers
 are not guaranteed to fire at the exact time you ask for.
 
-Returns a timer object which you can call C<< $timer->cancel >> on if you need to.
+Returns a timer object which you can call C<< $timer->cancel >> on if you need
+to.
 
 =cut
-sub AddTimer {
-    my ($class, $secs, $coderef) = @_;
+sub add_timer ($$) {
+    my ($secs, $coderef) = @_;
 
     my $fire_time = now() + $secs;
 
@@ -247,7 +248,7 @@ sub reap_pids {
     }
     if (@$WaitPids) {
         # we may not be donea, and we may miss our
-        $reap_timer = AddTimer(undef, 1, \&reap_pids);
+        $reap_timer = add_timer(1, \&reap_pids);
     }
 }
 
@@ -653,7 +654,7 @@ sub _run_later () {
 sub later ($) {
     my ($cb) = @_;
     push @$later_queue, $cb;
-    $later_timer //= AddTimer(undef, 60, \&_run_later);
+    $later_timer //= add_timer(60, \&_run_later);
 }
 
 sub expire_old () {
diff --git a/t/ds-leak.t b/t/ds-leak.t
index 34ffc125..a4a56e0d 100644
--- a/t/ds-leak.t
+++ b/t/ds-leak.t
@@ -20,7 +20,7 @@ if ('close-on-exec for epoll and kqueue') {
 	my ($r, $w);
 	pipe($r, $w) or die "pipe: $!";
 
-	PublicInbox::DS->AddTimer(0, sub { $pid = spawn([qw(sleep 10)]) });
+	PublicInbox::DS::add_timer(0, sub { $pid = spawn([qw(sleep 10)]) });
 	PublicInbox::DS->EventLoop;
 	ok($pid, 'subprocess spawned');
 

[PATCH 03/11] ds: add an in_loop() function for Inbox.pm use

[Eric Wong]

Inbox.pm accessing the $in_loop variable directly raises
warnings when Inbox is loaded without DS.
---
 lib/PublicInbox/DS.pm    | 2 ++
 lib/PublicInbox/Inbox.pm | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 217799bb..3b9193b6 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -255,6 +255,8 @@ sub reap_pids {
 # reentrant SIGCHLD handler (since reap_pids is not reentrant)
 sub enqueue_reap ($) { push @$nextq, \&reap_pids };
 
+sub in_loop () { $in_loop }
+
 sub EpollEventLoop {
     local $in_loop = 1;
     do {
diff --git a/lib/PublicInbox/Inbox.pm b/lib/PublicInbox/Inbox.pm
index a3cdcbc0..0c59a8b1 100644
--- a/lib/PublicInbox/Inbox.pm
+++ b/lib/PublicInbox/Inbox.pm
@@ -52,7 +52,7 @@ sub cleanup_task () {
 sub cleanup_possible () {
 	# no need to require DS, here, if it were enabled another
 	# module would've require'd it, already
-	eval { $PublicInbox::DS::in_loop } or return 0;
+	eval { PublicInbox::DS::in_loop() } or return 0;
 
 	eval {
 		require Devel::Peek; # needs separate package in Fedora

[PATCH 04/11] ds: remove Timer->cancel and Timer class+bless

[Eric Wong]

It doesn't seem needed at the moment, and we can re-add it
in the future if needed.
---
 lib/PublicInbox/DS.pm | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 3b9193b6..57c945f5 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -106,16 +106,13 @@ sub SetLoopTimeout {
 Add a timer to occur $seconds from now. $seconds may be fractional, but timers
 are not guaranteed to fire at the exact time you ask for.
 
-Returns a timer object which you can call C<< $timer->cancel >> on if you need
-to.
-
 =cut
 sub add_timer ($$) {
     my ($secs, $coderef) = @_;
 
     my $fire_time = now() + $secs;
 
-    my $timer = bless [$fire_time, $coderef], "PublicInbox::DS::Timer";
+    my $timer = [$fire_time, $coderef];
 
     if (!@Timers || $fire_time >= $Timers[-1][0]) {
         push @Timers, $timer;
@@ -693,12 +690,6 @@ sub not_idle_long {
     $exp_at > $now;
 }
 
-package PublicInbox::DS::Timer;
-# [$abs_float_firetime, $coderef];
-sub cancel {
-    $_[0][1] = undef;
-}
-
 1;
 
 =head1 AUTHORS (Danga::Socket)

[PATCH 05/11] ds: guard ToClose against DESTROY side-effects

[Eric Wong]

This does not affect our current code, but theoretically a
DESTROY callback could call PublicInbox::DS::close to enqueue
elements into the ToClose array.  So take a similar strategy as
we do with other queues (e.g. $nextq) by swapping references to
arrays, rather than operating on the array itself.

Since close operations are relatively rare, we can rely on
auto-vivification via "push" ops to create the array on an
as-needed basis.

Since we're in the area, clean up the PostLoopCallback
invocation to use the ternary operator rather than a confusing
(to me) combination of statements.

Finally, add a prototype to strengthen compile-time checking,
and move it in front of our only caller to make use of
the prototype.
---
 lib/PublicInbox/DS.pm | 52 ++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 28 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 57c945f5..52b11386 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -44,11 +44,11 @@ my $later_queue; # callbacks
 my $EXPMAP; # fd -> [ idle_time, $self ]
 our $EXPTIME = 180; # 3 minutes
 my ($later_timer, $reap_timer, $exp_timer);
+my $ToClose; # sockets to close when event loop is done
 our (
      %DescriptorMap,             # fd (num) -> PublicInbox::DS object
      $Epoll,                     # Global epoll fd (or DSKQXS ref)
      $_io,                       # IO::Handle for Epoll
-     @ToClose,                   # sockets to close when event loop is done
 
      $PostLoopCallback,          # subref to call at the end of each loop, if defined (global)
 
@@ -75,8 +75,7 @@ sub Reset {
     $WaitPids = [];
     $later_queue = [];
     $EXPMAP = {};
-    $reap_timer = $later_timer = $exp_timer = undef;
-    @ToClose = ();
+    $ToClose = $reap_timer = $later_timer = $exp_timer = undef;
     $LoopTimeout = -1;  # no timeout by default
     @Timers = ();
 
@@ -197,7 +196,7 @@ sub next_tick () {
 sub RunTimers {
     next_tick();
 
-    return ((@$nextq || @ToClose) ? 0 : $LoopTimeout) unless @Timers;
+    return ((@$nextq || $ToClose) ? 0 : $LoopTimeout) unless @Timers;
 
     my $now = now();
 
@@ -208,7 +207,7 @@ sub RunTimers {
     }
 
     # timers may enqueue into nextq:
-    return 0 if (@$nextq || @ToClose);
+    return 0 if (@$nextq || $ToClose);
 
     return $LoopTimeout unless @Timers;
 
@@ -254,6 +253,25 @@ sub enqueue_reap ($) { push @$nextq, \&reap_pids };
 
 sub in_loop () { $in_loop }
 
+# Internal function: run the post-event callback, send read events
+# for pushed-back data, and close pending connections.  returns 1
+# if event loop should continue, or 0 to shut it all down.
+sub PostEventLoop () {
+	# now we can close sockets that wanted to close during our event
+	# processing.  (we didn't want to close them during the loop, as we
+	# didn't want fd numbers being reused and confused during the event
+	# loop)
+	if (my $close_now = $ToClose) {
+		$ToClose = undef; # will be autovivified on push
+		# ->DESTROY methods may populate ToClose
+		delete($DescriptorMap{fileno($_)}) for @$close_now;
+		# let refcounting drop everything in $close_now at once
+	}
+
+	# by default we keep running, unless a postloop callback cancels it
+	$PostLoopCallback ? $PostLoopCallback->(\%DescriptorMap) : 1;
+}
+
 sub EpollEventLoop {
     local $in_loop = 1;
     do {
@@ -292,28 +310,6 @@ sub SetPostLoopCallback {
     $PostLoopCallback = (defined $ref && ref $ref eq 'CODE') ? $ref : undef;
 }
 
-# Internal function: run the post-event callback, send read events
-# for pushed-back data, and close pending connections.  returns 1
-# if event loop should continue, or 0 to shut it all down.
-sub PostEventLoop {
-    # now we can close sockets that wanted to close during our event processing.
-    # (we didn't want to close them during the loop, as we didn't want fd numbers
-    #  being reused and confused during the event loop)
-    delete($DescriptorMap{fileno($_)}) for @ToClose;
-    @ToClose = (); # let refcounting drop everything all at once
-
-    # by default we keep running, unless a postloop callback (either per-object
-    # or global) cancels it
-    my $keep_running = 1;
-
-    # now we're at the very end, call callback if defined
-    if (defined $PostLoopCallback) {
-        $keep_running &&= $PostLoopCallback->(\%DescriptorMap);
-    }
-
-    return $keep_running;
-}
-
 #####################################################################
 ### PublicInbox::DS-the-object code
 #####################################################################
@@ -390,7 +386,7 @@ sub close {
 
     # defer closing the actual socket until the event loop is done
     # processing this round of events.  (otherwise we might reuse fds)
-    push @ToClose, $sock;
+    push @$ToClose, $sock; # autovivifies $ToClose
 
     return 0;
 }

[PATCH 06/11] ds|http|nntp: simplify {wbuf} population

[Eric Wong]

We can rely on autovification to turn `undef' value of {wbuf}
into an arrayref.

Furthermore, "push" returns the (new) size of the array since at
least Perl 5.0 (I didn't look further back), so we can use that
return value instead of calling "scalar" again.
---
 lib/PublicInbox/DS.pm   | 11 +++++------
 lib/PublicInbox/HTTP.pm |  6 +++---
 lib/PublicInbox/NNTP.pm | 11 +++++------
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 52b11386..7a4aa7cc 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -31,7 +31,7 @@ use PublicInbox::Tmpfile;
 
 use fields ('sock',              # underlying socket
             'rbuf',              # scalarref, usually undef
-            'wbuf',              # arrayref of coderefs or GLOB refs
+            'wbuf', # arrayref of coderefs or GLOB refs (autovivified)
             'wbuf_off',  # offset into first element of wbuf to start writing at
             );
 
@@ -555,7 +555,7 @@ sub write {
 
         # wbuf may be an empty array if we're being called inside
         # ->flush_write via CODE bref:
-        push @{$self->{wbuf} ||= []}, $tmpio;
+        push @{$self->{wbuf}}, $tmpio; # autovivifies
         return 0;
     }
 }
@@ -575,8 +575,7 @@ sub msg_more ($$) {
             return 1 if $nlen == 0; # all done!
             # queue up the unwritten substring:
             my $tmpio = tmpio($self, \($_[1]), $n) or return 0;
-            $self->{wbuf} //= $wbuf //= [];
-            push @$wbuf, $tmpio;
+            push @{$self->{wbuf}}, $tmpio; # autovivifies
             epwait($sock, EPOLLOUT|EPOLLONESHOT);
             return 0;
         }
@@ -599,7 +598,7 @@ sub accept_tls_step ($) {
     return 1 if $sock->accept_SSL;
     return $self->close if $! != EAGAIN;
     epwait($sock, PublicInbox::TLS::epollbit() | EPOLLONESHOT);
-    unshift @{$self->{wbuf} ||= []}, \&accept_tls_step;
+    unshift(@{$self->{wbuf}}, \&accept_tls_step); # autovivifies
     0;
 }
 
@@ -610,7 +609,7 @@ sub shutdn_tls_step ($) {
     return $self->close if $sock->stop_SSL(SSL_fast_shutdown => 1);
     return $self->close if $! != EAGAIN;
     epwait($sock, PublicInbox::TLS::epollbit() | EPOLLONESHOT);
-    unshift @{$self->{wbuf} ||= []}, \&shutdn_tls_step;
+    unshift(@{$self->{wbuf}}, \&shutdn_tls_step); # autovivifies
     0;
 }
 
diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index a6ec1d0d..5fecf292 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -280,12 +280,12 @@ sub getline_pull {
 		}
 
 		if ($self->{sock}) {
-			my $wbuf = $self->{wbuf} //= [];
-			push @$wbuf, \&getline_pull;
+			# autovivify wbuf
+			my $new_size = push(@{$self->{wbuf}}, \&getline_pull);
 
 			# wbuf may be populated by {chunked,identity}_write()
 			# above, no need to rearm if so:
-			$self->requeue if scalar(@$wbuf) == 1;
+			$self->requeue if $new_size == 1;
 			return; # likely
 		}
 	} elsif ($@) {
diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 9f0dfaaa..35729f00 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -616,20 +616,19 @@ sub long_step {
 		# each other's data
 		$self->zflush;
 
-		# no recursion, schedule another call ASAP
-		# but only after all pending writes are done
-		my $wbuf = $self->{wbuf} ||= [];
-		push @$wbuf, \&long_step;
+		# no recursion, schedule another call ASAP, but only after
+		# all pending writes are done.  autovivify wbuf:
+		my $new_size = push(@{$self->{wbuf}}, \&long_step);
 
 		# wbuf may be populated by $cb, no need to rearm if so:
-		$self->requeue if scalar(@$wbuf) == 1;
+		$self->requeue if $new_size == 1;
 	} else { # all done!
 		delete $self->{long_cb};
 		res($self, '.');
 		my $elapsed = now() - $t0;
 		my $fd = fileno($self->{sock});
 		out($self, " deferred[$fd] done - %0.6f", $elapsed);
-		my $wbuf = $self->{wbuf};
+		my $wbuf = $self->{wbuf}; # do NOT autovivify
 		$self->requeue unless $wbuf && @$wbuf;
 	}
 }

[PATCH 07/11] ds: rely on autovivification for nextq

[Eric Wong]

Another place we can delay creating arrays until needed.
---
 lib/PublicInbox/DS.pm | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 7a4aa7cc..ac9065f2 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -71,11 +71,10 @@ Reset all state
 =cut
 sub Reset {
     %DescriptorMap = ();
-    $nextq = [];
     $WaitPids = [];
     $later_queue = [];
     $EXPMAP = {};
-    $ToClose = $reap_timer = $later_timer = $exp_timer = undef;
+    $nextq = $ToClose = $reap_timer = $later_timer = $exp_timer = undef;
     $LoopTimeout = -1;  # no timeout by default
     @Timers = ();
 
@@ -179,8 +178,8 @@ sub FirstTimeEventLoop {
 sub now () { clock_gettime(CLOCK_MONOTONIC) }
 
 sub next_tick () {
-    my $q = $nextq;
-    $nextq = [];
+    my $q = $nextq or return;
+    $nextq = undef;
     for (@$q) {
         # we avoid "ref" on blessed refs to workaround a Perl 5.16.3 leak:
         # https://rt.perl.org/Public/Bug/Display.html?id=114340
@@ -196,7 +195,7 @@ sub next_tick () {
 sub RunTimers {
     next_tick();
 
-    return ((@$nextq || $ToClose) ? 0 : $LoopTimeout) unless @Timers;
+    return (($nextq || $ToClose) ? 0 : $LoopTimeout) unless @Timers;
 
     my $now = now();
 
@@ -207,7 +206,7 @@ sub RunTimers {
     }
 
     # timers may enqueue into nextq:
-    return 0 if (@$nextq || $ToClose);
+    return 0 if ($nextq || $ToClose);
 
     return $LoopTimeout unless @Timers;
 
@@ -249,7 +248,7 @@ sub reap_pids {
 }
 
 # reentrant SIGCHLD handler (since reap_pids is not reentrant)
-sub enqueue_reap ($) { push @$nextq, \&reap_pids };
+sub enqueue_reap ($) { push @$nextq, \&reap_pids }; # autovivifies
 
 sub in_loop () { $in_loop }
 
@@ -353,7 +352,7 @@ sub new {
 ### I N S T A N C E   M E T H O D S
 #####################################################################
 
-sub requeue ($) { push @$nextq, $_[0] }
+sub requeue ($) { push @$nextq, $_[0] } # autovivifies
 
 =head2 C<< $obj->close >>
 

[PATCH 08/11] ds: rely on autovivication for waitpid bits

[Eric Wong]

No need to create an arrayref until we need it, and fix up a
comment while we're in the area.  Some aesthetic changes while
we're at it:

- Rename $WaitPids to $wait_pids to make it clear this is
  unique to our implementation and not in Danga::Socket.

- rewrite dwaitpid() to reduce indentation level
---
 lib/PublicInbox/DS.pm | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index ac9065f2..a78037f6 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -39,7 +39,7 @@ use Errno qw(EAGAIN EINVAL);
 use Carp qw(confess carp);
 
 my $nextq; # queue for next_tick
-my $WaitPids; # list of [ pid, callback, callback_arg ]
+my $wait_pids; # list of [ pid, callback, callback_arg ]
 my $later_queue; # callbacks
 my $EXPMAP; # fd -> [ idle_time, $self ]
 our $EXPTIME = 180; # 3 minutes
@@ -71,7 +71,7 @@ Reset all state
 =cut
 sub Reset {
     %DescriptorMap = ();
-    $WaitPids = [];
+    $wait_pids = undef;
     $later_queue = [];
     $EXPMAP = {};
     $nextq = $ToClose = $reap_timer = $later_timer = $exp_timer = undef;
@@ -226,25 +226,23 @@ sub RunTimers {
 }
 
 # We can't use waitpid(-1) safely here since it can hit ``, system(),
-# and other things.  So we scan the $WaitPids list, which is hopefully
-# not too big.
+# and other things.  So we scan the $wait_pids list, which is hopefully
+# not too big.  We keep $wait_pids small by not calling dwaitpid()
+# until we've hit EOF when reading the stdout of the child.
 sub reap_pids {
-    my $tmp = $WaitPids;
-    $WaitPids = [];
-    $reap_timer = undef;
+    my $tmp = $wait_pids or return;
+    $wait_pids = $reap_timer = undef;
     foreach my $ary (@$tmp) {
         my ($pid, $cb, $arg) = @$ary;
         my $ret = waitpid($pid, WNOHANG);
         if ($ret == 0) {
-            push @$WaitPids, $ary;
+            push @$wait_pids, $ary; # autovivifies @$wait_pids
         } elsif ($cb) {
             eval { $cb->($arg, $pid) };
         }
     }
-    if (@$WaitPids) {
-        # we may not be donea, and we may miss our
-        $reap_timer = add_timer(1, \&reap_pids);
-    }
+    # we may not be done, yet, and could've missed/masked a SIGCHLD:
+    $reap_timer = add_timer(1, \&reap_pids) if $wait_pids;
 }
 
 # reentrant SIGCHLD handler (since reap_pids is not reentrant)
@@ -626,15 +624,11 @@ sub shutdn ($) {
 
 # must be called with eval, PublicInbox::DS may not be loaded (see t/qspawn.t)
 sub dwaitpid ($$$) {
-    my ($pid, $cb, $arg) = @_;
-    if ($in_loop) {
-        push @$WaitPids, [ $pid, $cb, $arg ];
+	die "Not in EventLoop\n" unless $in_loop;
+	push @$wait_pids, [ @_ ]; # [ $pid, $cb, $arg ]
 
-        # We could've just missed our SIGCHLD, cover it, here:
-        requeue(\&reap_pids);
-    } else {
-        die "Not in EventLoop\n";
-    }
+	# We could've just missed our SIGCHLD, cover it, here:
+	requeue(\&reap_pids);
 }
 
 sub _run_later () {

[PATCH 09/11] ds: rely on autovivification for $later_queue

[Eric Wong]

No reason to have an empty arrayref lying around when not
everybody needs it.

Re-indent the later-related subs since we're changing a
bunch of lines, anyways.
---
 lib/PublicInbox/DS.pm | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index a78037f6..fc3c82cc 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -40,7 +40,7 @@ use Carp qw(confess carp);
 
 my $nextq; # queue for next_tick
 my $wait_pids; # list of [ pid, callback, callback_arg ]
-my $later_queue; # callbacks
+my $later_queue; # list of callbacks to run at some later interval
 my $EXPMAP; # fd -> [ idle_time, $self ]
 our $EXPTIME = 180; # 3 minutes
 my ($later_timer, $reap_timer, $exp_timer);
@@ -71,8 +71,7 @@ Reset all state
 =cut
 sub Reset {
     %DescriptorMap = ();
-    $wait_pids = undef;
-    $later_queue = [];
+    $wait_pids = $later_queue = undef;
     $EXPMAP = {};
     $nextq = $ToClose = $reap_timer = $later_timer = $exp_timer = undef;
     $LoopTimeout = -1;  # no timeout by default
@@ -632,16 +631,14 @@ sub dwaitpid ($$$) {
 }
 
 sub _run_later () {
-    my $run = $later_queue;
-    $later_timer = undef;
-    $later_queue = [];
-    $_->() for @$run;
+	my $run = $later_queue or return;
+	$later_timer = $later_queue = undef;
+	$_->() for @$run;
 }
 
 sub later ($) {
-    my ($cb) = @_;
-    push @$later_queue, $cb;
-    $later_timer //= add_timer(60, \&_run_later);
+	push @$later_queue, $_[0]; # autovivifies @$later_queue
+	$later_timer //= add_timer(60, \&_run_later);
 }
 
 sub expire_old () {

[PATCH 10/11] ds: flatten $EXPMAP, delete entries on close

[Eric Wong]

We can reduce the amount of small arrayrefs in memory
by flattening $EXPMAP.  This forces us to properly clean
up references during deferred close handling, so NNTP
(and soon HTTP) connections no longer linger until expiry.
---
 lib/PublicInbox/DS.pm | 57 ++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index fc3c82cc..c434464b 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -41,7 +41,7 @@ use Carp qw(confess carp);
 my $nextq; # queue for next_tick
 my $wait_pids; # list of [ pid, callback, callback_arg ]
 my $later_queue; # list of callbacks to run at some later interval
-my $EXPMAP; # fd -> [ idle_time, $self ]
+my $EXPMAP; # fd -> idle_time
 our $EXPTIME = 180; # 3 minutes
 my ($later_timer, $reap_timer, $exp_timer);
 my $ToClose; # sockets to close when event loop is done
@@ -259,9 +259,13 @@ sub PostEventLoop () {
 	# loop)
 	if (my $close_now = $ToClose) {
 		$ToClose = undef; # will be autovivified on push
+		@$close_now = map { fileno($_) } @$close_now;
+
+		# order matters, destroy expiry times, first:
+		delete @$EXPMAP{@$close_now};
+
 		# ->DESTROY methods may populate ToClose
-		delete($DescriptorMap{fileno($_)}) for @$close_now;
-		# let refcounting drop everything in $close_now at once
+		delete @DescriptorMap{@$close_now};
 	}
 
 	# by default we keep running, unless a postloop callback cancels it
@@ -642,37 +646,34 @@ sub 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;
+	my $now = now();
+	my $exp = $EXPTIME;
+	my $old = $now - $exp;
+	my %new;
+	while (my ($fd, $idle_at) = each %$EXPMAP) {
+		if ($idle_at < $old) {
+			my $ds_obj = $DescriptorMap{$fd};
+			$new{$fd} = $idle_at if !$ds_obj->shutdn;
+		} else {
+			$new{$fd} = $idle_at;
+		}
+	}
+	$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);
+	my ($self) = @_;
+	my $sock = $self->{sock} or return;
+	$EXPMAP->{fileno($sock)} = now();
+	$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;
+	my ($self, $now) = @_;
+	my $sock = $self->{sock} or return;
+	my $idle_at = $EXPMAP->{fileno($sock)} or return;
+	($idle_at + $EXPTIME) > $now;
 }
 
 1;

[PATCH 11/11] sigfd: simplify loop and improve documentation

[Eric Wong]

We can use the return value of sysread to bound our loop instead
of repeatedly shortening the string.  Furthermore add some
comments which can be easily checked against the signalfd(2)
manpage.
---
 lib/PublicInbox/Sigfd.pm | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/Sigfd.pm b/lib/PublicInbox/Sigfd.pm
index ec5d7145..15dedb10 100644
--- a/lib/PublicInbox/Sigfd.pm
+++ b/lib/PublicInbox/Sigfd.pm
@@ -42,14 +42,15 @@ sub new {
 # PublicInbox::Daemon in master main loop (blocking)
 sub wait_once ($) {
 	my ($self) = @_;
+	# 128 == sizeof(struct signalfd_siginfo)
 	my $r = sysread($self->{sock}, my $buf, 128 * 64);
 	if (defined($r)) {
-		while (1) {
-			my $sig = unpack('L', $buf);
-			my $cb = $self->{sig}->{$sig};
-			$cb->($sig) if $cb ne 'IGNORE';
-			return $r if length($buf) == 128;
-			$buf = substr($buf, 128);
+		my $nr = $r / 128 - 1; # $nr may be -1
+		for my $off (0..$nr) {
+			# the first uint32_t of signalfd_siginfo: ssi_signo
+			my $signo = unpack('L', substr($buf, 128 * $off, 4));
+			my $cb = $self->{sig}->{$signo};
+			$cb->($signo) if $cb ne 'IGNORE';
 		}
 	}
 	$r;