[PATCH 0/3] misc NNTP improvements

[PATCH 0/3] misc NNTP improvements

[Eric Wong]

A couple of minor cleanups and shutdown improvement for NNTP.

Eric Wong (3):
      config: introduce each_inbox for iteration
      nntp: simplify update_idle_time
      nntp: respect 3 minute idle time for shutdown

 lib/PublicInbox/Config.pm | 18 ++++++++++++++----
 lib/PublicInbox/ExtMsg.pm | 32 +++++++++++++++++---------------
 lib/PublicInbox/NNTP.pm   | 20 ++++++++++++++------
 lib/PublicInbox/NNTPD.pm  | 21 +++++++++------------
 t/nntpd.t                 |  1 +
 5 files changed, 55 insertions(+), 37 deletions(-)

[PATCH 1/3] config: introduce each_inbox for iteration

[Eric Wong]

This fills in the internal lookup hashes and simplifies
callers.
---
 lib/PublicInbox/Config.pm | 18 ++++++++++++++----
 lib/PublicInbox/ExtMsg.pm | 32 +++++++++++++++++---------------
 lib/PublicInbox/NNTPD.pm  | 21 +++++++++------------
 3 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index ddb4f6b..d34d11a 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -50,11 +50,21 @@ sub lookup {
 	_fill($self, $pfx);
 }
 
-sub lookup_name {
+sub lookup_name ($$) {
 	my ($self, $name) = @_;
-	my $rv = $self->{-by_name}->{$name};
-	return $rv if $rv;
-	$rv = _fill($self, "publicinbox.$name") or return;
+	$self->{-by_name}->{$name} || _fill($self, "publicinbox.$name");
+}
+
+sub each_inbox {
+	my ($self, $cb) = @_;
+	my %seen;
+	foreach my $k (keys %$self) {
+		$k =~ /\Apublicinbox\.([A-Z0-9a-z-]+)\.mainrepo\z/ or next;
+		next if $seen{$1};
+		$seen{$1} = 1;
+		my $ibx = lookup_name($self, $1) or next;
+		$cb->($ibx);
+	}
 }
 
 sub lookup_newsgroup {
diff --git a/lib/PublicInbox/ExtMsg.pm b/lib/PublicInbox/ExtMsg.pm
index 4b9e025..73bd4b1 100644
--- a/lib/PublicInbox/ExtMsg.pm
+++ b/lib/PublicInbox/ExtMsg.pm
@@ -30,19 +30,16 @@ sub ext_msg {
 
 	eval { require PublicInbox::Search };
 	my $have_xap = $@ ? 0 : 1;
-	my (@nox, @ibx);
+	my (@nox, @ibx, @found);
 
-	foreach my $k (keys %$pi_config) {
-		$k =~ /\Apublicinbox\.([A-Z0-9a-z-]+)\.url\z/ or next;
-		my $name = $1;
-		next if $name eq $cur->{name};
-		my $other = $pi_config->lookup_name($name) or next;
-		next unless $other->base_url;
+	$pi_config->each_inbox(sub {
+		my ($other) = @_;
+		return if $other->{name} eq $cur->{name} || !$other->base_url;
 
 		my $s = $other->search;
 		if (!$s) {
 			push @nox, $other;
-			next;
+			return;
 		}
 
 		# try to find the URL with Xapian to avoid forking
@@ -50,17 +47,22 @@ sub ext_msg {
 		if ($@) {
 			# xapian not configured properly for this repo
 			push @nox, $other;
-			next;
+			return;
 		}
 
 		# maybe we found it!
-		return r302($other, $mid) if defined $doc_id;
+		if (defined $doc_id) {
+			push @found, $other;
+		} else {
+			# no point in trying the fork fallback if we
+			# know Xapian is up-to-date but missing the
+			# message in the current repo
+			push @ibx, $other;
+		}
+	});
 
-		# no point in trying the fork fallback if we
-		# know Xapian is up-to-date but missing the
-		# message in the current repo
-		push @ibx, $other;
-	}
+	# TODO: multiple hits
+	return r302($found[0], $mid) if @found;
 
 	# Xapian not installed or configured for some repos,
 	# do a full MID check:
diff --git a/lib/PublicInbox/NNTPD.pm b/lib/PublicInbox/NNTPD.pm
index 50d022b..a67811b 100644
--- a/lib/PublicInbox/NNTPD.pm
+++ b/lib/PublicInbox/NNTPD.pm
@@ -23,21 +23,18 @@ sub refresh_groups () {
 	my $pi_config = PublicInbox::Config->new;
 	my $new = {};
 	my @list;
-	foreach my $k (keys %$pi_config) {
-		$k =~ /\Apublicinbox\.([^\.]+)\.mainrepo\z/ or next;
-		my $name = $1;
-		my $git_dir = $pi_config->{$k};
-		my $ngname = $pi_config->{"publicinbox.$name.newsgroup"};
-		next unless defined $ngname;
-		next if ($ngname eq ''); # disabled
-		my $ng = $pi_config->lookup_newsgroup($ngname) or next;
-
-		# Only valid if msgmap and search works
-		if ($ng->nntp_usable) {
+	$pi_config->each_inbox(sub {
+		my ($ng) = @_;
+		my $ngname = $ng->{newsgroup} or next;
+		if (ref $ngname) {
+			warn 'multiple newsgroups not supported: '.
+				join(', ', @$ngname). "\n";
+		} elsif ($ng->nntp_usable) {
+			# Only valid if msgmap and search works
 			$new->{$ngname} = $ng;
 			push @list, $ng;
 		}
-	}
+	});
 	@list =	sort { $a->{newsgroup} cmp $b->{newsgroup} } @list;
 	$self->{grouplist} = \@list;
 	# this will destroy old groups that got deleted

[PATCH 2/3] nntp: simplify update_idle_time

[Eric Wong]

This ought to make things easier when we add TLS support.
---
 lib/PublicInbox/NNTP.pm | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 4b116a7..b07e184 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -60,9 +60,8 @@ sub next_tick () {
 
 sub update_idle_time ($) {
 	my ($self) = @_;
-	my $tmp = $self->{sock} or return;
-	$tmp = fileno($tmp);
-	defined $tmp and $EXPMAP->{$tmp} = [ now(), $self ];
+	my $fd = $self->{fd};
+	defined $fd and $EXPMAP->{$fd} = [ now(), $self ];
 }
 
 # reduce FD pressure by closing some "git cat-file --batch" processes

[PATCH 3/3] nntp: respect 3 minute idle time for shutdown

[Eric Wong]

This avoids breaking clients on graceful shutdown since
NNTP responses should usually be quick.
---
 lib/PublicInbox/NNTP.pm | 15 ++++++++++++---
 t/nntpd.t               |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index b07e184..56d0838 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -998,10 +998,19 @@ sub watch_read {
 	$rv;
 }
 
+sub not_idle_long ($$) {
+	my ($self, $now) = @_;
+	defined(my $fd = $self->{fd}) or return;
+	my $ary = $EXPMAP->{$fd} or return;
+	my $exp_at = $ary->[0] + $EXPTIME;
+	$exp_at > $now;
+}
+
 # for graceful shutdown in PublicInbox::Daemon:
-sub busy () {
-	my ($self) = @_;
-	($self->{rbuf} ne '' || $self->{long_res} || $self->{write_buf_size});
+sub busy {
+	my ($self, $now) = @_;
+	($self->{rbuf} ne '' || $self->{long_res} || $self->{write_buf_size} ||
+	 not_idle_long($self, $now));
 }
 
 1;
diff --git a/t/nntpd.t b/t/nntpd.t
index 5875b73..7192d78 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -219,6 +219,7 @@ EOF
 		is(scalar @r, 1, 'only one response line');
 	}
 
+	$n = $s = undef;
 	is($pid, waitpid($pid, 0), 'nntpd exited successfully');
 	my $eout = eval {
 		local $/;

[PATCH 4/3] nntpd: avoid exiting subroutine via next

[Eric Wong]

Fixes: 33cef7f24d3d ("config: introduce each_inbox for iteration")
---
 lib/PublicInbox/NNTPD.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/NNTPD.pm b/lib/PublicInbox/NNTPD.pm
index a67811b..eb43a2b 100644
--- a/lib/PublicInbox/NNTPD.pm
+++ b/lib/PublicInbox/NNTPD.pm
@@ -25,7 +25,7 @@ sub refresh_groups () {
 	my @list;
 	$pi_config->each_inbox(sub {
 		my ($ng) = @_;
-		my $ngname = $ng->{newsgroup} or next;
+		my $ngname = $ng->{newsgroup} or return;
 		if (ref $ngname) {
 			warn 'multiple newsgroups not supported: '.
 				join(', ', @$ngname). "\n";
-- 
EW