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 B01491FA01 for ; Sat, 16 Oct 2021 01:01:03 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 03/12] imapd+nntpd: drop timer-based expiration Date: Sat, 16 Oct 2021 01:00:54 +0000 Message-Id: <20211016010103.30825-4-e@80x24.org> In-Reply-To: <20211016010103.30825-1-e@80x24.org> References: <20211016010103.30825-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: It's needlessly complex and O(n), so it doesn't scale well to a high number of clients nor is it easy-to-scale with the data structures available to us in pure Perl. In any case, I see no evidence of either -imapd nor -nntpd experiencing high connection loads on public-facing sites. -httpd has never had its own timer-based expiration, either. Fwiw, public-inbox.org itself has been running a public-facing HTTP/HTTPS server with no userspace idle client expiration for the past 8 years or with no ill effect. Clients can come and go as they wish, and SO_KEEPALIVE takes care of truly broken connections if they're gone for ~2 hours. Internet connections drop all time, so it should be harmless to drop connections w/o warning since both NNTP and IMAP protocols have well-defined semantics for determining if a message was truncated (as does HTTP/1.1+). --- lib/PublicInbox/DS.pm | 37 +------------------------------------ lib/PublicInbox/Daemon.pm | 8 +++----- lib/PublicInbox/HTTP.pm | 5 ++--- lib/PublicInbox/IMAP.pm | 17 ++++------------- lib/PublicInbox/NNTP.pm | 12 +++--------- 5 files changed, 13 insertions(+), 66 deletions(-) diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm index 9cca02d7ffe9..bf8c4466218a 100644 --- a/lib/PublicInbox/DS.pm +++ b/lib/PublicInbox/DS.pm @@ -37,9 +37,7 @@ our @EXPORT_OK = qw(now msg_more dwaitpid add_timer add_uniq_timer); my %Stack; my $nextq; # queue for next_tick my $wait_pids; # list of [ pid, callback, callback_arg ] -my $EXPMAP; # fd -> idle_time -our $EXPTIME = 180; # 3 minutes -my ($reap_armed); +my $reap_armed; my $ToClose; # sockets to close when event loop is done our ( %DescriptorMap, # fd (num) -> PublicInbox::DS object @@ -76,7 +74,6 @@ sub Reset { # we may be iterating inside one of these on our stack my @q = delete @Stack{keys %Stack}; for my $q (@q) { @$q = () } - $EXPMAP = undef; $wait_pids = $nextq = $ToClose = undef; $ep_io = undef; # closes real $Epoll FD $Epoll = undef; # may call DSKQXS::DESTROY @@ -251,9 +248,6 @@ sub PostEventLoop () { $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{@$close_now}; } @@ -655,35 +649,6 @@ sub dwaitpid ($;$$) { } } -sub expire_old () { - my $cur = $EXPMAP or return; - $EXPMAP = undef; - my $old = now() - $EXPTIME; - while (my ($fd, $idle_at) = each %$cur) { - if ($idle_at < $old) { - my $ds_obj = $DescriptorMap{$fd}; - $EXPMAP->{$fd} = $idle_at if !$ds_obj->shutdn; - } else { - $EXPMAP->{$fd} = $idle_at; - } - } - add_uniq_timer('expire', 60, \&expire_old) if $EXPMAP; -} - -sub update_idle_time { - my ($self) = @_; - my $sock = $self->{sock} or return; - $EXPMAP->{fileno($sock)} = now(); - add_uniq_timer('expire', 60, \&expire_old); -} - -sub not_idle_long { - my ($self, $now) = @_; - my $sock = $self->{sock} or return; - my $idle_at = $EXPMAP->{fileno($sock)} or return; - ($idle_at + $EXPTIME) > $now; -} - 1; =head1 AUTHORS (Danga::Socket) diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm index 90b77412f96a..505235864c0b 100644 --- a/lib/PublicInbox/Daemon.pm +++ b/lib/PublicInbox/Daemon.pm @@ -271,13 +271,11 @@ sub worker_quit { # $_[0] = signal name or number (unused) my ($dmap, undef) = @_; my $n = 0; my $now = now(); - - foreach my $s (values %$dmap) { + for my $s (values %$dmap) { $s->can('busy') or next; - if ($s->busy($now)) { + if ($s->busy) { ++$n; - } else { - # close as much as possible, early as possible + } else { # close as much as possible, early as possible $s->close; } } diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm index 8a89dd73b031..8057481d1ce5 100644 --- a/lib/PublicInbox/HTTP.pm +++ b/lib/PublicInbox/HTTP.pm @@ -459,10 +459,9 @@ sub close { $self->SUPER::close; # PublicInbox::DS::close } -# for graceful shutdown in PublicInbox::Daemon: -sub busy () { +sub busy { # for graceful shutdown in PublicInbox::Daemon: my ($self) = @_; - ($self->{rbuf} || exists($self->{env}) || $self->{wbuf}); + defined($self->{rbuf}) || exists($self->{env}) || defined($self->{wbuf}) } # runs $cb on the next iteration of the event loop at earliest diff --git a/lib/PublicInbox/IMAP.pm b/lib/PublicInbox/IMAP.pm index bc34f4feea1e..41bcf9af295d 100644 --- a/lib/PublicInbox/IMAP.pm +++ b/lib/PublicInbox/IMAP.pm @@ -99,9 +99,6 @@ undef %FETCH_NEED; my $valid_range = '[0-9]+|[0-9]+:[0-9]+|[0-9]+:\*'; $valid_range = qr/\A(?:$valid_range)(?:,(?:$valid_range))*\z/; -# RFC 3501 5.4. Autologout Timer needs to be >= 30min -$PublicInbox::DS::EXPTIME = 60 * 30; - sub greet ($) { my ($self) = @_; my $capa = capa($self); @@ -124,7 +121,6 @@ sub new ($$$) { } else { greet($self); } - $self->update_idle_time; $self; } @@ -323,7 +319,6 @@ sub idle_tick_all { $IDLERS = undef; for my $i (values %$old) { next if ($i->{wbuf} || !exists($i->{-idle_tag})); - $i->update_idle_time or next; $IDLERS->{fileno($i->{sock})} = $i; $i->write(\"* OK Still here\r\n"); } @@ -1226,8 +1221,6 @@ sub long_step { out($self, " deferred[$fd] aborted - %0.6f", $elapsed); $self->close; } elsif ($more) { # $self->{wbuf}: - $self->update_idle_time; - # control passed to ibx_async_cat if $more == \undef requeue_once($self) if !ref($more); } else { # all done! @@ -1269,7 +1262,6 @@ sub event_step { return unless $self->flush_write && $self->{sock} && !$self->{long_cb}; - $self->update_idle_time; # only read more requests if we've drained the write buffer, # otherwise we can be buffering infinitely w/o backpressure @@ -1295,7 +1287,6 @@ sub event_step { return $self->close if $r < 0; $self->rbuf_idle($rbuf); - $self->update_idle_time; # maybe there's more pipelined data, or we'll have # to register it for socket-readiness notifications @@ -1334,14 +1325,14 @@ sub cmd_starttls ($$) { undef; } -# for graceful shutdown in PublicInbox::Daemon: -sub busy { - my ($self, $now) = @_; +sub busy { # for graceful shutdown in PublicInbox::Daemon: + my ($self) = @_; if (defined($self->{-idle_tag})) { $self->write(\"* BYE server shutting down\r\n"); return; # not busy anymore } - ($self->{rbuf} || $self->{wbuf} || $self->not_idle_long($now)); + defined($self->{rbuf}) || defined($self->{wbuf}) || + !$self->write(\"* BYE server shutting down\r\n"); } sub close { diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm index aa0193687dd6..b36722d72745 100644 --- a/lib/PublicInbox/NNTP.pm +++ b/lib/PublicInbox/NNTP.pm @@ -65,7 +65,6 @@ sub new ($$$) { } else { greet($self); } - $self->update_idle_time; $self; } @@ -651,8 +650,6 @@ sub long_step { out($self, " deferred[$fd] aborted - %0.6f", $elapsed); $self->close; } elsif ($more) { # $self->{wbuf}: - $self->update_idle_time; - # COMPRESS users all share the same DEFLATE context. # Flush it here to ensure clients don't see # each other's data @@ -1050,7 +1047,6 @@ sub event_step { return unless $self->flush_write && $self->{sock} && !$self->{long_cb}; - $self->update_idle_time; # only read more requests if we've drained the write buffer, # otherwise we can be buffering infinitely w/o backpressure @@ -1072,17 +1068,15 @@ sub event_step { out($self, "[$fd] %s - %0.6f$pending", $line, now() - $t0); return $self->close if $r < 0; $self->rbuf_idle($rbuf); - $self->update_idle_time; # maybe there's more pipelined data, or we'll have # to register it for socket-readiness notifications $self->requeue unless $pending; } -# for graceful shutdown in PublicInbox::Daemon: -sub busy { - my ($self, $now) = @_; - ($self->{rbuf} || $self->{wbuf} || $self->not_idle_long($now)); +sub busy { # for graceful shutdown in PublicInbox::Daemon: + my ($self) = @_; + defined($self->{rbuf}) || defined($self->{wbuf}) } 1;