user/dev discussion of public-inbox itself
 help / color / Atom feed
From: Eric Wong <e@yhbt.net>
To: meta@public-inbox.org
Subject: [PATCH 05/11] ds: guard ToClose against DESTROY side-effects
Date: Sun, 12 Jan 2020 21:17:50 +0000
Message-ID: <20200112211756.23100-6-e@yhbt.net> (raw)
In-Reply-To: <20200112211756.23100-1-e@yhbt.net>

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;
 }

  parent reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-12 21:17 [PATCH 00/11] ds: various cleanups and fixes Eric Wong
2020-01-12 21:17 ` [PATCH 01/11] ds: new: avoid redundant check, make clobbering fatal Eric Wong
2020-01-12 21:17 ` [PATCH 02/11] ds: add_timer: rename from AddTimer, remove a parameter Eric Wong
2020-01-12 21:17 ` [PATCH 03/11] ds: add an in_loop() function for Inbox.pm use Eric Wong
2020-01-12 21:17 ` [PATCH 04/11] ds: remove Timer->cancel and Timer class+bless Eric Wong
2020-01-12 21:17 ` Eric Wong [this message]
2020-01-12 21:17 ` [PATCH 06/11] ds|http|nntp: simplify {wbuf} population Eric Wong
2020-01-12 21:17 ` [PATCH 07/11] ds: rely on autovivification for nextq Eric Wong
2020-01-12 21:17 ` [PATCH 08/11] ds: rely on autovivication for waitpid bits Eric Wong
2020-01-12 21:17 ` [PATCH 09/11] ds: rely on autovivification for $later_queue Eric Wong
2020-01-12 21:17 ` [PATCH 10/11] ds: flatten $EXPMAP, delete entries on close Eric Wong
2020-01-12 21:17 ` [PATCH 11/11] sigfd: simplify loop and improve documentation Eric Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://public-inbox.org/README

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200112211756.23100-6-e@yhbt.net \
    --to=e@yhbt.net \
    --cc=meta@public-inbox.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

user/dev discussion of public-inbox itself

Archives are clonable:
	git clone --mirror http://public-inbox.org/meta
	git clone --mirror http://czquwvybam4bgbro.onion/meta
	git clone --mirror http://hjrcffqmbrq6wope.onion/meta
	git clone --mirror http://ou63pmih66umazou.onion/meta

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.mail.public-inbox.meta
	nntp://ou63pmih66umazou.onion/inbox.comp.mail.public-inbox.meta
	nntp://czquwvybam4bgbro.onion/inbox.comp.mail.public-inbox.meta
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.mail.public-inbox.meta
	nntp://news.gmane.io/gmane.mail.public-inbox.general

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git