From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id B704E1FD54 for ; Sun, 12 Jan 2020 21:17:57 +0000 (UTC) From: Eric Wong 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> In-Reply-To: <20200112211756.23100-1-e@yhbt.net> References: <20200112211756.23100-1-e@yhbt.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: 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; }