[PATCH 0/6] ds: object lifetime shortening

[PATCH 0/6] ds: object lifetime shortening

[Eric Wong]

1 and 2 do the bulk of the work
3 and 4 are things I noticed when working on 2
5 is a fix while preparing for 6
6 is something I've wanted to do for a long time.

Anyways, the code reductions always make me happy.

Eric Wong (6):
  ds: next_tick: shorten object lifetimes
  ds: do not defer close
  ds: move maxevents further down the stack
  watch: simplify DirIdle object cleanup
  pop3: use SSL_shutdown(3ssl) if appropriate
  ds: make ->close behave like CORE::close

 lib/PublicInbox/CidxComm.pm |   2 +-
 lib/PublicInbox/CidxLogP.pm |   2 +-
 lib/PublicInbox/DS.pm       | 143 ++++++++++++------------------------
 lib/PublicInbox/DSKQXS.pm   |   3 +-
 lib/PublicInbox/DSPoll.pm   |   2 +-
 lib/PublicInbox/Epoll.pm    |   5 +-
 lib/PublicInbox/POP3.pm     |   2 +-
 lib/PublicInbox/Select.pm   |   2 +-
 lib/PublicInbox/Watch.pm    |  11 +--
 t/ds-poll.t                 |  14 ++--
 t/epoll.t                   |   4 +-
 11 files changed, 71 insertions(+), 119 deletions(-)

[PATCH 1/6] ds: next_tick: shorten object lifetimes

[Eric Wong]

Drop reference counts ASAP in case it saves us some memory
sooner rather than later.  This ought to give us more predictable
resource use and ensure OnDestroy callbacks fire sooner.
There's no need to use `local' to clobber the arrayref anymore,
either.

AFAIK, this doesn't fix any known bug, but more predictability
will make it easier to debug things going forward.
---
 lib/PublicInbox/DS.pm | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 6041c6b5..708fe14d 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -136,16 +136,12 @@ sub _InitPoller () {
 sub now () { clock_gettime(CLOCK_MONOTONIC) }
 
 sub next_tick () {
-	local $cur_runq = $nextq or return;
+	$cur_runq = $nextq or return;
 	$nextq = undef;
-	for my $obj (@$cur_runq) {
+	while (my $obj = shift @$cur_runq) {
 		# avoid "ref" on blessed refs to workaround a Perl 5.16.3 leak:
 		# https://rt.perl.org/Public/Bug/Display.html?id=114340
-		if (blessed($obj)) {
-			$obj->event_step;
-		} else {
-			$obj->();
-		}
+		blessed($obj) ? $obj->event_step : $obj->();
 	}
 }
 

[PATCH 2/6] ds: do not defer close

[Eric Wong]

We can map all integer FDs to Perl objects once ->ep_wait returns,
so there's no need to play tricks elsewhere to ensure FDs can
be mapped to objects within the same event loop iteration.
---
 lib/PublicInbox/DS.pm | 67 ++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 45 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 708fe14d..b30e9db6 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -39,7 +39,7 @@ our @EXPORT_OK = qw(now msg_more awaitpid add_timer add_uniq_timer);
 
 my $nextq; # queue for next_tick
 my $reap_armed;
-my $ToClose; # sockets to close when event loop is done
+my @active; # FDs (or objs) returned by epoll/kqueue
 our (%AWAIT_PIDS, # pid => [ $callback, @args ]
 	$cur_runq, # only set inside next_tick
      %DescriptorMap,             # fd (num) -> PublicInbox::DS object
@@ -81,10 +81,11 @@ sub Reset {
 
 		# we may be called from an *atfork_child inside next_tick:
 		@$cur_runq = () if $cur_runq;
-		$nextq = $ToClose = undef; # may call ep_del
+		@active = ();
+		$nextq = undef; # may call ep_del
 		%AWAIT_PIDS = ();
 	} while (@Timers || $nextq || keys(%AWAIT_PIDS) ||
-		$ToClose || keys(%DescriptorMap) ||
+		@active || keys(%DescriptorMap) ||
 		@post_loop_do || keys(%UniqTimer) ||
 		scalar(@{$cur_runq // []})); # do not vivify cur_runq
 
@@ -149,7 +150,7 @@ sub next_tick () {
 sub RunTimers {
 	next_tick();
 
-	return (($nextq || $ToClose) ? 0 : $loop_timeout) unless @Timers;
+	return ($nextq ? 0 : $loop_timeout) unless @Timers;
 
 	my $now = now();
 
@@ -161,7 +162,7 @@ sub RunTimers {
 	}
 
 	# timers may enqueue into nextq:
-	return 0 if ($nextq || $ToClose);
+	return 0 if $nextq;
 
 	return $loop_timeout unless @Timers;
 
@@ -236,18 +237,6 @@ sub close_non_busy () {
 # 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
-		@$close_now = map { fileno($_) } @$close_now;
-
-		# ->DESTROY methods may populate ToClose
-		delete @DescriptorMap{@$close_now};
-	}
-
 	# by default we keep running, unless a postloop callback cancels it
 	@post_loop_do ? $post_loop_do[0]->(@post_loop_do[1..$#post_loop_do]) : 1
 }
@@ -295,21 +284,16 @@ sub event_loop (;$$) {
 	}
 	$_[0] = $sigfd = $sig = undef; # $_[0] == sig
 	local $in_loop = 1;
-	my @events;
 	do {
 		my $timeout = RunTimers();
 
-		# get up to 1000 events
-		$Poller->ep_wait(1000, $timeout, \@events);
-		for my $fd (@events) {
-			# it's possible epoll_wait returned many events,
-			# including some at the end that ones in the front
-			# triggered unregister-interest actions.  if we can't
-			# find the %sock entry, it's because we're no longer
-			# interested in that event.
-
-			# guard stack-not-refcounted w/ Carp + @DB::args
-			my $obj = $DescriptorMap{$fd};
+		# get up to 1000 FDs representing events
+		$Poller->ep_wait(1000, $timeout, \@active);
+
+		# map all FDs to their associated Perl object
+		@active = @DescriptorMap{@active};
+
+		while (my $obj = shift @active) {
 			$obj->event_step;
 		}
 	} while (PostEventLoop());
@@ -390,24 +374,17 @@ sub close {
     # preventing the object from being destroyed
     delete $self->{wbuf};
 
-    # if we're using epoll, we have to remove this from our epoll fd so we stop getting
-    # notifications about it
-    $Poller->ep_del($sock) and croak("EPOLL_CTL_DEL($self/$sock): $!");
-
-    # we explicitly don't delete from DescriptorMap here until we
-    # actually close the socket, as we might be in the middle of
-    # processing an epoll_wait/etc that returned hundreds of fds, one
-    # of which is not yet processed and is what we're closing.  if we
-    # keep it in DescriptorMap, then the event harnesses can just
-    # looked at $pob->{sock} == undef and ignore it.  but if it's an
-    # un-accounted for fd, then it (understandably) freak out a bit
-    # and emit warnings, thinking their state got off.
+    delete $DescriptorMap{fileno($sock)};
 
-    # defer closing the actual socket until the event loop is done
-    # processing this round of events.  (otherwise we might reuse fds)
-    push @$ToClose, $sock; # autovivifies $ToClose
+    # if we're using epoll, we have to remove this from our epoll fd so we
+    # stop getting notifications about it
+    $Poller->ep_del($sock) and croak("EPOLL_CTL_DEL($self/$sock): $!");
+    # let refcount drop to zero..
 
-    return 0;
+    # FIXME this is the opposite of CORE::close return value
+    # TODO: callers need to be updated to expect true on success like
+    # CORE::close (and false otherwise)
+    0;
 }
 
 # portable, non-thread-safe sendfile emulation (no pread, yet)

[PATCH 3/6] ds: move maxevents further down the stack

[Eric Wong]

The epoll implementation is the only one which respects the
limit (kevent would, but IO::KQueue does not).  In any case,
I'm not a fan of the maxevents=1000 historical default since
it leads to fairness problems with shared non-blocking listeners
across multiple daemon workers.
---
 lib/PublicInbox/DS.pm     |  4 ++--
 lib/PublicInbox/DSKQXS.pm |  3 ++-
 lib/PublicInbox/DSPoll.pm |  2 +-
 lib/PublicInbox/Epoll.pm  |  5 ++++-
 lib/PublicInbox/Select.pm |  2 +-
 t/ds-poll.t               | 14 +++++++-------
 t/epoll.t                 |  4 ++--
 7 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index b30e9db6..9e1f66c2 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -287,8 +287,8 @@ sub event_loop (;$$) {
 	do {
 		my $timeout = RunTimers();
 
-		# get up to 1000 FDs representing events
-		$Poller->ep_wait(1000, $timeout, \@active);
+		# grab whatever FDs are ready
+		$Poller->ep_wait($timeout, \@active);
 
 		# map all FDs to their associated Perl object
 		@active = @DescriptorMap{@active};
diff --git a/lib/PublicInbox/DSKQXS.pm b/lib/PublicInbox/DSKQXS.pm
index 8ef8ffb6..f84c196a 100644
--- a/lib/PublicInbox/DSKQXS.pm
+++ b/lib/PublicInbox/DSKQXS.pm
@@ -117,7 +117,8 @@ sub ep_del {
 }
 
 sub ep_wait {
-	my ($self, $maxevents, $timeout_msec, $events) = @_;
+	my ($self, $timeout_msec, $events) = @_;
+	# n.b.: IO::KQueue is hard-coded to return up to 1000 events
 	@$events = eval { $self->{kq}->kevent($timeout_msec) };
 	if (my $err = $@) {
 		# workaround https://rt.cpan.org/Ticket/Display.html?id=116615
diff --git a/lib/PublicInbox/DSPoll.pm b/lib/PublicInbox/DSPoll.pm
index 0446df4c..b947f756 100644
--- a/lib/PublicInbox/DSPoll.pm
+++ b/lib/PublicInbox/DSPoll.pm
@@ -18,7 +18,7 @@ use Errno ();
 sub new { bless {}, __PACKAGE__ } # fd => events
 
 sub ep_wait {
-	my ($self, $maxevents, $timeout_msec, $events) = @_;
+	my ($self, $timeout_msec, $events) = @_;
 	my (@pset, $n, $fd, $revents, $nval);
 	while (my ($fd, $events) = each %$self) {
 		my $pevents = $events & EPOLLIN ? POLLIN : 0;
diff --git a/lib/PublicInbox/Epoll.pm b/lib/PublicInbox/Epoll.pm
index d55c8535..7e0aa6e7 100644
--- a/lib/PublicInbox/Epoll.pm
+++ b/lib/PublicInbox/Epoll.pm
@@ -18,6 +18,9 @@ sub new {
 sub ep_add { epoll_ctl(fileno(${$_[0]}), EPOLL_CTL_ADD, fileno($_[1]), $_[2]) }
 sub ep_mod { epoll_ctl(fileno(${$_[0]}), EPOLL_CTL_MOD, fileno($_[1]), $_[2]) }
 sub ep_del { epoll_ctl(fileno(${$_[0]}), EPOLL_CTL_DEL, fileno($_[1]), 0) }
-sub ep_wait { epoll_wait(fileno(${$_[0]}), @_[1, 2, 3]) }
+
+# n.b. maxevents=1000 is the historical default.  maxevents=1 (yes, one)
+# is more fair under load with multiple worker processes sharing one listener
+sub ep_wait { epoll_wait(fileno(${$_[0]}), 1000, @_[1, 2]) }
 
 1;
diff --git a/lib/PublicInbox/Select.pm b/lib/PublicInbox/Select.pm
index 5817c9ef..5cb7aff3 100644
--- a/lib/PublicInbox/Select.pm
+++ b/lib/PublicInbox/Select.pm
@@ -12,7 +12,7 @@ use PublicInbox::Syscall qw(EPOLLONESHOT EPOLLIN EPOLLOUT);
 sub new { bless {}, __PACKAGE__ } # fd => events
 
 sub ep_wait {
-	my ($self, $maxevents, $msec, $events) = @_;
+	my ($self, $msec, $events) = @_;
 	my ($rvec, $wvec) = ('', ''); # we don't use EPOLLERR
 	while (my ($fd, $ev) = each %$self) {
 		vec($rvec, $fd, 1) = 1 if $ev & EPOLLIN;
diff --git a/t/ds-poll.t b/t/ds-poll.t
index 7a7e2bcf..321534bd 100644
--- a/t/ds-poll.t
+++ b/t/ds-poll.t
@@ -16,33 +16,33 @@ pipe($r, $w);
 pipe($x, $y);
 is($p->ep_add($r, EPOLLIN), 0, 'add EPOLLIN');
 my $events = [];
-$p->ep_wait(9, 0, $events);
+$p->ep_wait(0, $events);
 is_deeply($events, [], 'no events set');
 is($p->ep_add($w, EPOLLOUT|EPOLLONESHOT), 0, 'add EPOLLOUT|EPOLLONESHOT');
-$p->ep_wait(9, -1, $events);
+$p->ep_wait(-1, $events);
 is(scalar(@$events), 1, 'got POLLOUT event');
 is($events->[0], fileno($w), '$w ready');
 
-$p->ep_wait(9, 0, $events);
+$p->ep_wait(0, $events);
 is(scalar(@$events), 0, 'nothing ready after oneshot');
 is_deeply($events, [], 'no events set after oneshot');
 
 syswrite($w, '1') == 1 or die;
 for my $t (0..1) {
-	$p->ep_wait(9, $t, $events);
+	$p->ep_wait($t, $events);
 	is($events->[0], fileno($r), "level-trigger POLLIN ready #$t");
 	is(scalar(@$events), 1, "only event ready #$t");
 }
 syswrite($y, '1') == 1 or die;
 is($p->ep_add($x, EPOLLIN|EPOLLONESHOT), 0, 'EPOLLIN|EPOLLONESHOT add');
-$p->ep_wait(9, -1, $events);
+$p->ep_wait(-1, $events);
 is(scalar @$events, 2, 'epoll_wait has 2 ready');
 my @fds = sort @$events;
 my @exp = sort((fileno($r), fileno($x)));
 is_deeply(\@fds, \@exp, 'got both ready FDs');
 
 is($p->ep_del($r, 0), 0, 'EPOLL_CTL_DEL OK');
-$p->ep_wait(9, 0, $events);
+$p->ep_wait(0, $events);
 is(scalar @$events, 0, 'nothing ready after EPOLL_CTL_DEL');
 
 is($p->ep_add($r, EPOLLIN), 0, 're-add');
@@ -54,7 +54,7 @@ SKIP: {
 	my @w;
 	eval {
 		local $SIG{__WARN__} = sub { push @w, @_ };
-		$p->ep_wait(9, 0, $events);
+		$p->ep_wait(0, $events);
 	};
 	ok($@, 'error detected from bad FD');
 	ok($!{EBADF}, 'EBADF errno set');
diff --git a/t/epoll.t b/t/epoll.t
index 54dc6f47..ab9ac22a 100644
--- a/t/epoll.t
+++ b/t/epoll.t
@@ -12,11 +12,11 @@ pipe(my $r, my $w);
 is($ep->ep_add($w, EPOLLOUT), 0, 'epoll_ctl pipe EPOLLOUT');
 
 my @events;
-$ep->ep_wait(100, 10000, \@events);
+$ep->ep_wait(10000, \@events);
 is(scalar(@events), 1, 'got one event');
 is($events[0], fileno($w), 'got expected FD');
 close $w;
-$ep->ep_wait(100, 0, \@events);
+$ep->ep_wait(0, \@events);
 is(scalar(@events), 0, 'epoll_wait timeout');
 
 done_testing;

[PATCH 4/6] watch: simplify DirIdle object cleanup

[Eric Wong]

There's no need to waste time nor reach into DS internals to
map FDs to Perl objects, here.  LEI.pm has never had to deal
with integer FDs for DirIdle, either.
---
 lib/PublicInbox/Watch.pm | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/Watch.pm b/lib/PublicInbox/Watch.pm
index 41b77dc1..c2b1312a 100644
--- a/lib/PublicInbox/Watch.pm
+++ b/lib/PublicInbox/Watch.pm
@@ -256,9 +256,8 @@ sub quit { # may be called in IMAP/NNTP children
 	%{$self->{opendirs}} = ();
 	_done_for_now($self);
 	quit_done($self);
-	if (defined(my $fd = delete $self->{dir_idle_fd})) {
-		my $di = $PublicInbox::DS::DescriptorMap{$fd};
-		$di->close if $di && $di->can('add_watches');
+	if (my $dir_idle = delete $self->{dir_idle}) {
+		$dir_idle->close if $dir_idle;
 	}
 	if (my $idle_mic = delete $self->{idle_mic}) { # IMAP child
 		return unless $idle_mic->IsConnected && $idle_mic->Socket;
@@ -283,8 +282,7 @@ sub watch_fs_init ($) {
 	};
 	require PublicInbox::DirIdle;
 	# inotify_create + EPOLL_CTL_ADD
-	my $dir_idle = PublicInbox::DirIdle->new($cb);
-	$self->{dir_idle_fd} = fileno($dir_idle->{sock}) if $dir_idle->{sock};
+	my $dir_idle = $self->{dir_idle} = PublicInbox::DirIdle->new($cb);
 	$dir_idle->add_watches([keys %{$self->{mdmap}}]);
 }
 
@@ -383,8 +381,7 @@ sub watch_imap_idle_1 ($$$) {
 
 sub watch_atfork_child ($) {
 	my ($self) = @_;
-	delete $self->{pids};
-	delete $self->{opendirs};
+	delete @$self{qw(dir_idle pids opendirs)};
 	my $sig = delete $self->{sig};
 	$sig->{CHLD} = $sig->{HUP} = $sig->{USR1} = 'DEFAULT';
 	# TERM/QUIT/INT call ->quit, which works in both parent+child

[PATCH 5/6] pop3: use SSL_shutdown(3ssl) if appropriate

[Eric Wong]

This allows us support SSL session caching + reuse in the future.
---
 lib/PublicInbox/POP3.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/POP3.pm b/lib/PublicInbox/POP3.pm
index 6d24b17c..06772069 100644
--- a/lib/PublicInbox/POP3.pm
+++ b/lib/PublicInbox/POP3.pm
@@ -370,7 +370,7 @@ UPDATE users SET last_seen = ? WHERE user_id = ?
 		$self->{pop3d}->unlock_mailbox($self);
 	}
 	$self->write(\"+OK public-inbox POP3 server signing off\r\n");
-	$self->close;
+	$self->shutdn;
 	undef;
 }
 

[PATCH 6/6] ds: make ->close behave like CORE::close

[Eric Wong]

Matching existing Perl IO semantics seems like a good idea to
reduce confusion in the future.

We'll also fix some outdated comments and update indentation
to match the rest of our code base since we're far detached from
Danga::Socket at this point.
---
 lib/PublicInbox/CidxComm.pm |  2 +-
 lib/PublicInbox/CidxLogP.pm |  2 +-
 lib/PublicInbox/DS.pm       | 84 ++++++++++++++-----------------------
 3 files changed, 33 insertions(+), 55 deletions(-)

diff --git a/lib/PublicInbox/CidxComm.pm b/lib/PublicInbox/CidxComm.pm
index fb7be0aa..c7ab3c10 100644
--- a/lib/PublicInbox/CidxComm.pm
+++ b/lib/PublicInbox/CidxComm.pm
@@ -21,7 +21,7 @@ sub new {
 sub event_step {
 	my ($self) = @_;
 	my $rd = $self->{sock} // return warn('BUG?: no {sock}');
-	$self->close; # PublicInbox::DS::close, deferred, so $sock is usable
+	$self->close; # EPOLL_CTL_DEL
 	delete($self->{cidx})->cidx_read_comm($rd);
 }
 
diff --git a/lib/PublicInbox/CidxLogP.pm b/lib/PublicInbox/CidxLogP.pm
index ac4c1b37..5ea675bf 100644
--- a/lib/PublicInbox/CidxLogP.pm
+++ b/lib/PublicInbox/CidxLogP.pm
@@ -21,7 +21,7 @@ sub new {
 sub event_step {
 	my ($self) = @_;
 	my $rd = $self->{sock} // return warn('BUG?: no {sock}');
-	$self->close; # PublicInbox::DS::close, deferred, so $sock is usable
+	$self->close; # EPOLL_CTL_DEL
 	delete($self->{cidx})->cidx_read_log_p($self, $rd);
 }
 
diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 9e1f66c2..ca1f0f81 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -354,37 +354,21 @@ sub greet {
 	$self;
 }
 
-#####################################################################
-### I N S T A N C E   M E T H O D S
-#####################################################################
-
 sub requeue ($) { push @$nextq, $_[0] } # autovivifies
 
-=head2 C<< $obj->close >>
-
-Close the socket.
-
-=cut
+# drop the IO::Handle ref, true if successful, false if not (or already dropped)
+# (this is closer to CORE::close than Danga::Socket::close)
 sub close {
-    my ($self) = @_;
-    my $sock = delete $self->{sock} or return;
-
-    # we need to flush our write buffer, as there may
-    # be self-referential closures (sub { $client->close })
-    # preventing the object from being destroyed
-    delete $self->{wbuf};
-
-    delete $DescriptorMap{fileno($sock)};
+	my ($self) = @_;
+	my $sock = delete $self->{sock} or return;
 
-    # if we're using epoll, we have to remove this from our epoll fd so we
-    # stop getting notifications about it
-    $Poller->ep_del($sock) and croak("EPOLL_CTL_DEL($self/$sock): $!");
-    # let refcount drop to zero..
+	# we need to clear our write buffer, as there may
+	# be self-referential closures (sub { $client->close })
+	# preventing the object from being destroyed
+	delete $self->{wbuf};
+	delete $DescriptorMap{fileno($sock)};
 
-    # FIXME this is the opposite of CORE::close return value
-    # TODO: callers need to be updated to expect true on success like
-    # CORE::close (and false otherwise)
-    0;
+	!$Poller->ep_del($sock); # stop getting notifications
 }
 
 # portable, non-thread-safe sendfile emulation (no pread, yet)
@@ -430,8 +414,7 @@ next_buf:
                         shift @$wbuf;
                         goto next_buf;
                     }
-                } elsif ($! == EAGAIN) {
-                    my $ev = epbit($sock, EPOLLOUT) or return $self->close;
+                } elsif ($! == EAGAIN && (my $ev = epbit($sock, EPOLLOUT))) {
                     epwait($sock, $ev | EPOLLONESHOT);
                     return 0;
                 } else {
@@ -461,28 +444,28 @@ sub rbuf_idle ($$) {
     }
 }
 
+# returns true if bytes are read, false otherwise
 sub do_read ($$$;$) {
-    my ($self, $rbuf, $len, $off) = @_;
-    my $r = sysread(my $sock = $self->{sock}, $$rbuf, $len, $off // 0);
-    return ($r == 0 ? $self->close : $r) if defined $r;
-    # common for clients to break connections without warning,
-    # would be too noisy to log here:
-    if ($! == EAGAIN) {
-        my $ev = epbit($sock, EPOLLIN) or return $self->close;
-        epwait($sock, $ev | EPOLLONESHOT);
-        rbuf_idle($self, $rbuf);
-        0;
-    } else {
-        $self->close;
-    }
+	my ($self, $rbuf, $len, $off) = @_;
+	my ($ev, $r, $s);
+	$r = sysread($s = $self->{sock}, $$rbuf, $len, $off // 0) and return $r;
+
+	if (!defined($r) && $! == EAGAIN && ($ev = epbit($s, EPOLLIN))) {
+		epwait($s, $ev | EPOLLONESHOT);
+		rbuf_idle($self, $rbuf);
+	} else {
+		$self->close;
+	}
+	undef;
 }
 
 # drop the socket if we hit unrecoverable errors on our system which
 # require BOFH attention: ENOSPC, EFBIG, EIO, EMFILE, ENFILE...
 sub drop {
-    my $self = shift;
-    carp(@_);
-    $self->close;
+	my $self = shift;
+	carp(@_);
+	$self->close;
+	undef;
 }
 
 sub tmpio ($$$) {
@@ -603,7 +586,7 @@ sub accept_tls_step ($) {
     0;
 }
 
-# return true if complete, false if incomplete (or failure)
+# return value irrelevant
 sub shutdn_tls_step ($) {
     my ($self) = @_;
     my $sock = $self->{sock} or return;
@@ -612,19 +595,14 @@ sub shutdn_tls_step ($) {
     my $ev = PublicInbox::TLS::epollbit() or return $self->close;
     epwait($sock, $ev | EPOLLONESHOT);
     unshift(@{$self->{wbuf}}, \&shutdn_tls_step); # autovivifies
-    0;
 }
 
 # don't bother with shutdown($sock, 2), we don't fork+exec w/o CLOEXEC
 # or fork w/o exec, so no inadvertent socket sharing
 sub shutdn ($) {
-    my ($self) = @_;
-    my $sock = $self->{sock} or return;
-    if ($sock->can('stop_SSL')) {
-        shutdn_tls_step($self);
-    } else {
-	$self->close;
-    }
+	my ($self) = @_;
+	my $sock = $self->{sock} or return;
+	$sock->can('stop_SSL') ? shutdn_tls_step($self) : $self->close;
 }
 
 sub dflush {} # overridden by DSdeflate