[PATCH 0/5] daemon/netd-related improvements

[PATCH 0/5] daemon/netd-related improvements

[Eric Wong]

A few minor quality-of-life improvements I've noticed while
running -netd on public-inbox.org

Eric Wong (5):
  daemon: use default address + well-known ports for scheme
  daemon: use per-listener SIG{__WARN__} callbacks
  daemon: cleanup internal data structures
  imap: mailboxes list across listeners
  imapd|nntpd: drop ->base_url preload

 lib/PublicInbox/Daemon.pm | 103 +++++++++++++++++++-------------------
 lib/PublicInbox/HTTP.pm   |   2 +-
 lib/PublicInbox/HTTPD.pm  |   1 +
 lib/PublicInbox/IMAP.pm   |   2 +-
 lib/PublicInbox/IMAPD.pm  |  50 +++++++++---------
 lib/PublicInbox/NNTP.pm   |   2 +-
 lib/PublicInbox/NNTPD.pm  |   1 -
 lib/PublicInbox/POP3.pm   |   1 +
 8 files changed, 84 insertions(+), 78 deletions(-)

[PATCH 1/5] daemon: use default address + well-known ports for scheme

[Eric Wong]

This ensures the "bound $URL" diagnostic message at startup
always shows the URL scheme handled if not relying on socket
inheritance.

This also avoids duplicate/unused data structures when binding
sockets ourselves, as bound socket names can expand from short
names to longer names (e.g. "0:119" => "0.0.0.0:119").
---
 lib/PublicInbox/Daemon.pm | 45 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index b6f4f9ed..0043d21e 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -192,20 +192,23 @@ EOF
 
 	foreach my $l (@cfg_listen) {
 		my $orig = $l;
-		my $scheme = '';
-		my $port;
-		if ($l =~ s!\A([^:]+)://!!) { $scheme = $1 }
+		my ($scheme, $port, $opt);
+
+		$l =~ s!\A([a-z0-9]+)://!! and $scheme = $1;
+		(!$scheme && ($default_listen // '') =~ m!\A([^:]+)://!) and
+			$scheme = $1;
 		if ($l =~ /\A(?:\[[^\]]+\]|[^:]+):([0-9]+)/) {
 			$port = $1 + 0;
-			my $s = $KNOWN_TLS{$port} // $KNOWN_STARTTLS{$port};
-			$scheme //= $s if defined $s;
-		} elsif (index($l, '/') != 0) { # unix socket
-			$port //= $SCHEME2PORT{$scheme} if $scheme;
-			$port // die "no port in listen=$l\n";
+			$scheme //= $KNOWN_TLS{$port} // $KNOWN_STARTTLS{$port};
+		}
+		$scheme or die "unable to determine URL scheme of $orig\n";
+		if (!defined($port) && index($l, '/') != 0) { # unix socket
+			$port = $SCHEME2PORT{$scheme} //
+				die "no port in listen=$orig\n";
 			$l =~ s!\A([^/]+)!$1:$port! or
 				die "unable to add port=$port to $l\n";
 		}
-		my $opt; # non-TLS options
+		$l =~ s!/\z!!; # chop one trailing slash
 		if ($l =~ s!/?\?(.+)\z!!) {
 			$opt = listener_opt($1);
 			$tls_opt{"$scheme://$l"} = accept_tls_opt($opt);
@@ -214,10 +217,10 @@ EOF
 		} elsif ($scheme =~ /\A(?:https|imaps|nntps|pop3s)\z/) {
 			die "$orig specified w/o cert=\n";
 		}
-		$scheme =~ /\A(?:http|imap|nntp|pop3)/ and
+		if ($listener_names->{$l}) { # already inherited
 			$xnetd->{$l} = load_mod($scheme, $opt, $l);
-
-		next if $listener_names->{$l}; # already inherited
+			next;
+		}
 		my (%o, $sock_pkg);
 		if (index($l, '/') == 0) {
 			$sock_pkg = 'IO::Socket::UNIX';
@@ -244,16 +247,16 @@ EOF
 		}
 		$o{Listen} = 1024;
 		my $prev = umask 0000;
-		my $s = eval { $sock_pkg->new(%o) };
-		warn "error binding $l: $! ($@)\n" unless $s;
+		my $s = eval { $sock_pkg->new(%o) } or
+			warn "error binding $l: $! ($@)\n";
 		umask $prev;
-		if ($s) {
-			$s->blocking(0);
-			my $k = sockname($s);
-			warn "# bound $scheme://$k\n";
-			$listener_names->{$k} = $s;
-			push @listeners, $s;
-		}
+		$s // next;
+		$s->blocking(0);
+		my $sockname = sockname($s);
+		warn "# bound $scheme://$sockname\n";
+		$xnetd->{$sockname} //= load_mod($scheme);
+		$listener_names->{$sockname} = $s;
+		push @listeners, $s;
 	}
 
 	# cert/key options in @cfg_listen takes precedence when inheriting,

[PATCH 2/5] daemon: use per-listener SIG{__WARN__} callbacks

[Eric Wong]

This allows "-l $ADDRESS?err=/path/to/err.log to isolate normal
warn() (and carp()) messages for a particular listen address to
track down errors more easily.
---
 lib/PublicInbox/Daemon.pm | 2 ++
 lib/PublicInbox/HTTP.pm   | 2 +-
 lib/PublicInbox/HTTPD.pm  | 1 +
 lib/PublicInbox/IMAP.pm   | 2 +-
 lib/PublicInbox/NNTP.pm   | 2 +-
 lib/PublicInbox/POP3.pm   | 1 +
 6 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index 0043d21e..bb140640 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -134,6 +134,8 @@ sub load_mod ($;$$) {
 		$tlsd->{$f} = $logs{$p} //= open_log_path(my $fh, $p);
 		warn "# $scheme://$addr $f=$p\n";
 	}
+	my $err = $tlsd->{err};
+	$tlsd->{warn_cb} = sub { print $err @_ }; # for local $SIG{__WARN__}
 	\%xn;
 }
 
diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index 3d4e3499..0dba425d 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -69,7 +69,7 @@ sub new ($$$) {
 
 sub event_step { # called by PublicInbox::DS
 	my ($self) = @_;
-
+	local $SIG{__WARN__} = $self->{srv_env}->{'pi-httpd.warn_cb'};
 	return unless $self->flush_write && $self->{sock};
 
 	# only read more requests if we've drained the write buffer,
diff --git a/lib/PublicInbox/HTTPD.pm b/lib/PublicInbox/HTTPD.pm
index e531ee70..bae7281b 100644
--- a/lib/PublicInbox/HTTPD.pm
+++ b/lib/PublicInbox/HTTPD.pm
@@ -47,6 +47,7 @@ sub env_for ($$$) {
 		# detect when to use async paths for slow blobs
 		'pi-httpd.async' => \&pi_httpd_async,
 		'pi-httpd.app' => $self->{app},
+		'pi-httpd.warn_cb' => $self->{warn_cb},
 	}
 }
 
diff --git a/lib/PublicInbox/IMAP.pm b/lib/PublicInbox/IMAP.pm
index 605c5e51..2be1b763 100644
--- a/lib/PublicInbox/IMAP.pm
+++ b/lib/PublicInbox/IMAP.pm
@@ -1186,7 +1186,7 @@ sub out ($$;@) {
 # callback used by PublicInbox::DS for any (e)poll (in/out/hup/err)
 sub event_step {
 	my ($self) = @_;
-
+	local $SIG{__WARN__} = $self->{imapd}->{warn_cb};
 	return unless $self->flush_write && $self->{sock} && !$self->{long_cb};
 
 	# only read more requests if we've drained the write buffer,
diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 524784cb..ef01f448 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -958,7 +958,7 @@ sub out ($$;@) {
 # callback used by PublicInbox::DS for any (e)poll (in/out/hup/err)
 sub event_step {
 	my ($self) = @_;
-
+	local $SIG{__WARN__} = $self->{nntpd}->{warn_cb};
 	return unless $self->flush_write && $self->{sock} && !$self->{long_cb};
 
 	# only read more requests if we've drained the write buffer,
diff --git a/lib/PublicInbox/POP3.pm b/lib/PublicInbox/POP3.pm
index 7469922b..53fb2e05 100644
--- a/lib/PublicInbox/POP3.pm
+++ b/lib/PublicInbox/POP3.pm
@@ -351,6 +351,7 @@ sub process_line ($$) {
 # callback used by PublicInbox::DS for any (e)poll (in/out/hup/err)
 sub event_step {
 	my ($self) = @_;
+	local $SIG{__WARN__} = $self->{pop3d}->{warn_cb};
 	return unless $self->flush_write && $self->{sock} && !$self->{long_cb};
 
 	# only read more requests if we've drained the write buffer,

[PATCH 3/5] daemon: cleanup internal data structures

[Eric Wong]

This avoids dangling {''} entries in $xnetd and
%tls_opt hashes.  Furthermore, we can safely undef
%tls_opt once it's associated with each $xnetd object.
---
 lib/PublicInbox/Daemon.pm | 66 ++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 35 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index bb140640..16bae231 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -179,10 +179,7 @@ EOF
 		die "--pid-file cannot end with '.oldbin'\n";
 	}
 	@listeners = inherit($listener_names);
-
-	# allow socket-activation users to set certs once and not
-	# have to configure each socket:
-	my @inherited_names = keys(%$listener_names) if defined($default_cert);
+	my @inherited_names = keys(%$listener_names);
 
 	# ignore daemonize when inheriting
 	$daemonize = undef if scalar @listeners;
@@ -191,20 +188,18 @@ EOF
 		$default_listen // die "no listeners specified\n";
 		push @cfg_listen, $default_listen
 	}
-
+	my ($default_scheme) = (($default_listen // '') =~ m!\A([^:]+)://!);
 	foreach my $l (@cfg_listen) {
 		my $orig = $l;
 		my ($scheme, $port, $opt);
-
 		$l =~ s!\A([a-z0-9]+)://!! and $scheme = $1;
-		(!$scheme && ($default_listen // '') =~ m!\A([^:]+)://!) and
-			$scheme = $1;
+		$scheme //= $default_scheme;
 		if ($l =~ /\A(?:\[[^\]]+\]|[^:]+):([0-9]+)/) {
 			$port = $1 + 0;
 			$scheme //= $KNOWN_TLS{$port} // $KNOWN_STARTTLS{$port};
 		}
-		$scheme or die "unable to determine URL scheme of $orig\n";
-		if (!defined($port) && index($l, '/') != 0) { # unix socket
+		$scheme // die "unable to determine URL scheme of $orig\n";
+		if (!defined($port) && index($l, '/') != 0) { # AF_UNIX socket
 			$port = $SCHEME2PORT{$scheme} //
 				die "no port in listen=$orig\n";
 			$l =~ s!\A([^/]+)!$1:$port! or
@@ -263,21 +258,28 @@ EOF
 
 	# cert/key options in @cfg_listen takes precedence when inheriting,
 	# but map well-known inherited ports if --listen isn't specified
-	# at all
-	for my $sockname (@inherited_names) {
-		$sockname =~ /:([0-9]+)\z/ or next;
-		if (my $scheme = $KNOWN_TLS{$1}) {
-			$xnetd->{$sockname} //= load_mod($scheme);
-			$tls_opt{"$scheme://$sockname"} ||= accept_tls_opt('');
-		} elsif (($scheme = $KNOWN_STARTTLS{$1})) {
-			$xnetd->{$sockname} //= load_mod($scheme);
-			$tls_opt{"$scheme://$sockname"} ||= accept_tls_opt('');
-			$tls_opt{''} ||= accept_tls_opt('');
+	# at all.  This allows socket-activation users to set certs once
+	# and not have to configure each socket:
+	if (defined $default_cert) {
+		my ($stls) = (($default_scheme // '') =~ /\A(pop3|nntp|imap)/);
+		for my $x (@inherited_names) {
+			$x =~ /:([0-9]+)\z/ or next; # no TLS for AF_UNIX
+			if (my $scheme = $KNOWN_TLS{$1}) {
+				$xnetd->{$x} //= load_mod($scheme);
+				$tls_opt{"$scheme://$x"} ||= accept_tls_opt('');
+			} elsif (($scheme = $KNOWN_STARTTLS{$1})) {
+				$xnetd->{$x} //= load_mod($scheme);
+				$tls_opt{"$scheme://$x"} ||= accept_tls_opt('');
+			} elsif (defined $stls) {
+				$tls_opt{"$stls://$x"} ||= accept_tls_opt('');
+			}
+		}
+	}
+	if (defined $default_scheme) {
+		for my $x (@inherited_names) {
+			$xnetd->{$x} //= load_mod($default_scheme);
 		}
 	}
-	my @d;
-	while (my ($k, $v) = each %tls_opt) { push(@d, $k) if !defined($v) }
-	delete @tls_opt{@d};
 	die "No listeners bound\n" unless @listeners;
 }
 
@@ -671,14 +673,14 @@ sub daemon_loop ($) {
 	};
 	my %post_accept;
 	while (my ($k, $ctx_opt) = each %tls_opt) {
-		my $l = $k;
-		$l =~ s!\A([^:]+)://!!;
-		my $scheme = $1 // '';
-		my $xn = $xnetd->{$l} // $xnetd->{''};
+		$ctx_opt // next;
+		my ($scheme, $l) = split(m!://!, $k, 2);
+		my $xn = $xnetd->{$l} // die "BUG: no xnetd for $k";
 		$xn->{tlsd}->{ssl_ctx_opt} //= $ctx_opt;
 		$scheme =~ m!\A(?:https|imaps|nntps|pop3s)! and
 			$post_accept{$l} = tls_cb(@$xn{qw(post_accept tlsd)});
 	}
+	undef %tls_opt;
 	my $sig = {
 		HUP => $refresh,
 		INT => \&worker_quit,
@@ -706,7 +708,7 @@ sub daemon_loop ($) {
 	@listeners = map {;
 		my $l = sockname($_);
 		my $tls_cb = $post_accept{$l};
-		my $xn = $xnetd->{$l} // $xnetd->{''};
+		my $xn = $xnetd->{$l} // die "BUG: no xnetd for $l";
 
 		# NNTPS, HTTPS, HTTP, IMAPS and POP3S are client-first traffic
 		# IMAP, NNTP and POP3 are server-first
@@ -720,13 +722,7 @@ sub daemon_loop ($) {
 
 sub run {
 	my ($default_listen) = @_;
-	my $xnetd = {};
-	if ($default_listen) {
-		$default_listen =~ /\A(http|imap|nntp|pop3)/ or
-			die "BUG: $default_listen";
-		$xnetd->{''} = load_mod($1);
-	}
-	daemon_prepare($default_listen, $xnetd);
+	daemon_prepare($default_listen, my $xnetd = {});
 	my $for_destroy = daemonize();
 
 	# localize GCF2C for tests:

[PATCH 4/5] imap: mailboxes list across listeners

[Eric Wong]

Since IMAP mailbox lists are tied to the PublicInbox::Config
object, we can share them the same way the config object is
shared when an -imapd or -netd instance has multiple listeners.

This ought to reduce memory use and startup time when binding
multiple sockets which share a common config file.
---
 lib/PublicInbox/IMAPD.pm | 49 ++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/lib/PublicInbox/IMAPD.pm b/lib/PublicInbox/IMAPD.pm
index dd0d2c53..ba6ad05d 100644
--- a/lib/PublicInbox/IMAPD.pm
+++ b/lib/PublicInbox/IMAPD.pm
@@ -23,7 +23,7 @@ sub new {
 	}, $class;
 }
 
-sub imapd_refresh_ibx { # pi_cfg->each_inbox cb
+sub _refresh_ibx { # pi_cfg->each_inbox cb
 	my ($ibx, $imapd, $cache, $dummies) = @_;
 	my $ngname = $ibx->{newsgroup} // return;
 
@@ -56,27 +56,32 @@ sub imapd_refresh_ibx { # pi_cfg->each_inbox cb
 sub refresh_groups {
 	my ($self, $sig) = @_;
 	my $pi_cfg = PublicInbox::Config->new;
-	my $mailboxes = $self->{mailboxes} = {};
-	my $cache = eval { $pi_cfg->ALL->misc->nntpd_cache_load } // {};
-	my $dummies = {};
-	$pi_cfg->each_inbox(\&imapd_refresh_ibx, $self, $cache, $dummies);
-	%$dummies = (%$dummies, %$mailboxes);
-	$mailboxes = $self->{mailboxes} = $dummies;
-	@{$self->{mailboxlist}} = map { $_->[2] }
-		sort { $a->[0] cmp $b->[0] || $a->[1] <=> $b->[1] }
-		map {
-			my $u = $_; # capitalize "INBOX" for user-familiarity
-			$u =~ s/\Ainbox(\.|\z)/INBOX$1/i;
-			if ($mailboxes->{$_} == $dummy) {
-				[ $u, -1,
-				  qq[* LIST (\\HasChildren) "." $u\r\n]]
-			} else {
-				$u =~ /\A(.+)\.([0-9]+)\z/ or
-					die "BUG: `$u' has no slice digit(s)";
-				[ $1, $2 + 0,
-				  qq[* LIST (\\HasNoChildren) "." $u\r\n] ]
-			}
-		} keys %$mailboxes;
+	$self->{mailboxes} = $pi_cfg->{-imap_mailboxes} // do {
+		my $mailboxes = $self->{mailboxes} = {};
+		my $cache = eval { $pi_cfg->ALL->misc->nntpd_cache_load } // {};
+		my $dummies = {};
+		$pi_cfg->each_inbox(\&_refresh_ibx, $self, $cache, $dummies);
+		%$mailboxes = (%$dummies, %$mailboxes);
+		@{$pi_cfg->{-imap_mailboxlist}} = map { $_->[2] }
+			sort { $a->[0] cmp $b->[0] || $a->[1] <=> $b->[1] }
+			map {
+				# capitalize "INBOX" for user-familiarity
+				my $u = $_;
+				$u =~ s/\Ainbox(\.|\z)/INBOX$1/i;
+				if ($mailboxes->{$_} == $dummy) {
+					[ $u, -1,
+					  qq[* LIST (\\HasChildren) "." $u\r\n]]
+				} else {
+					$u =~ /\A(.+)\.([0-9]+)\z/ or die
+"BUG: `$u' has no slice digit(s)";
+					[ $1, $2 + 0, '* LIST '.
+					  qq[(\\HasNoChildren) "." $u\r\n] ]
+				}
+			} keys %$mailboxes;
+		$pi_cfg->{-imap_mailboxes} = $mailboxes;
+	};
+	$self->{mailboxlist} = $pi_cfg->{-imap_mailboxlist} //
+			die 'BUG: no mailboxlist';
 	$self->{pi_cfg} = $pi_cfg;
 	if (my $idler = $self->{idler}) {
 		$idler->refresh($pi_cfg);

[PATCH 5/5] imapd|nntpd: drop ->base_url preload

[Eric Wong]

It was never useful for IMAP, and NNTP hasn't needed it since:
1bf653ad139bf7bb (nntp+www: drop List-* and Archived-At headers, 2020-12-10)
---
 lib/PublicInbox/IMAPD.pm | 1 -
 lib/PublicInbox/NNTPD.pm | 1 -
 2 files changed, 2 deletions(-)

diff --git a/lib/PublicInbox/IMAPD.pm b/lib/PublicInbox/IMAPD.pm
index ba6ad05d..78323e57 100644
--- a/lib/PublicInbox/IMAPD.pm
+++ b/lib/PublicInbox/IMAPD.pm
@@ -44,7 +44,6 @@ sub _refresh_ibx { # pi_cfg->each_inbox cb
 		PublicInbox::IMAP::ensure_slices_exist($imapd, $ibx);
 		# preload to avoid fragmentation:
 		$ibx->description;
-		$ibx->base_url;
 		# ensure dummies are selectable:
 		do {
 			$dummies->{$ngname} = $dummy;
diff --git a/lib/PublicInbox/NNTPD.pm b/lib/PublicInbox/NNTPD.pm
index 4f550bb0..4401a29b 100644
--- a/lib/PublicInbox/NNTPD.pm
+++ b/lib/PublicInbox/NNTPD.pm
@@ -45,7 +45,6 @@ sub refresh_groups {
 			# only valid if msgmap and over works
 			# preload to avoid fragmentation:
 			$ibx->description;
-			$ibx->base_url;
 		} else {
 			delete $groups->{$ngname};
 			# Note: don't be tempted to delete more for memory