user/dev discussion of public-inbox itself
 help / color / mirror / code / Atom feed
From: Eric Wong <e@yhbt.net>
To: meta@public-inbox.org
Subject: [PATCH 07/82] imap: delay InboxIdle start, support refresh
Date: Wed, 10 Jun 2020 07:04:04 +0000	[thread overview]
Message-ID: <20200610070519.18252-8-e@yhbt.net> (raw)
In-Reply-To: <20200610070519.18252-1-e@yhbt.net>

InboxIdle should not be holding onto Inbox objects after the
Config object they came from expires, and Config objects may
expire on SIGHUP.

Old Inbox objects still persist due to IMAP clients holding onto
them, but that's a concern we'll deal with at another time, or
not at all, since all clients expire, eventually.

Regardless, stale inotify watch descriptors should not be left
hanging after SIGHUP refreshes.
---
 lib/PublicInbox/IMAP.pm      |  1 +
 lib/PublicInbox/IMAPD.pm     | 14 +++++----
 lib/PublicInbox/InboxIdle.pm | 36 ++++++++++++++++++----
 t/imapd.t                    | 58 ++++++++++++++++++++++++++++++++++--
 4 files changed, 96 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/IMAP.pm b/lib/PublicInbox/IMAP.pm
index 4a43185c512..c8592dc0329 100644
--- a/lib/PublicInbox/IMAP.pm
+++ b/lib/PublicInbox/IMAP.pm
@@ -160,6 +160,7 @@ sub cmd_idle ($$) {
 	# IDLE seems allowed by dovecot w/o a mailbox selected *shrug*
 	my $ibx = $self->{ibx} or return "$tag BAD no mailbox selected\r\n";
 	$ibx->subscribe_unlock(fileno($self->{sock}), $self);
+	$self->{imapd}->idler_start;
 	$self->{-idle_tag} = $tag;
 	$self->{-idle_max} = $ibx->mm->max // 0;
 	"+ idling\r\n"
diff --git a/lib/PublicInbox/IMAPD.pm b/lib/PublicInbox/IMAPD.pm
index 1922c16046a..05aa30e42a1 100644
--- a/lib/PublicInbox/IMAPD.pm
+++ b/lib/PublicInbox/IMAPD.pm
@@ -16,18 +16,22 @@ sub new {
 		out => \*STDOUT,
 		grouplist => [],
 		# accept_tls => { SSL_server => 1, ..., SSL_reuse_ctx => ... }
+		# pi_config => PublicInbox::Config
 		# idler => PublicInbox::InboxIdle
 	}, $class;
 }
 
 sub refresh_groups {
 	my ($self) = @_;
-	if (my $old_idler = delete $self->{idler}) {
-		$old_idler->close; # PublicInbox::DS::close
-	}
-	my $pi_config = PublicInbox::Config->new;
-	$self->{idler} = PublicInbox::InboxIdle->new($pi_config);
+	my $pi_config = $self->{pi_config} = PublicInbox::Config->new;
 	$self->SUPER::refresh_groups($pi_config);
+	if (my $idler = $self->{idler}) {
+		$idler->refresh($pi_config);
+	}
+}
+
+sub idler_start {
+	$_[0]->{idler} //= PublicInbox::InboxIdle->new($_[0]->{pi_config});
 }
 
 1;
diff --git a/lib/PublicInbox/InboxIdle.pm b/lib/PublicInbox/InboxIdle.pm
index 095a801c946..c19b8d186cd 100644
--- a/lib/PublicInbox/InboxIdle.pm
+++ b/lib/PublicInbox/InboxIdle.pm
@@ -4,7 +4,8 @@
 package PublicInbox::InboxIdle;
 use strict;
 use base qw(PublicInbox::DS);
-use fields qw(pi_config inot);
+use fields qw(pi_config inot pathmap);
+use Cwd qw(abs_path);
 use Symbol qw(gensym);
 use PublicInbox::Syscall qw(EPOLLIN EPOLLET);
 my $IN_CLOSE = 0x08 | 0x10; # match Linux inotify
@@ -19,13 +20,35 @@ if ($^O eq 'linux' && eval { require Linux::Inotify2; 1 }) {
 require PublicInbox::In2Tie if $ino_cls;
 
 sub in2_arm ($$) { # PublicInbox::Config::each_inbox callback
-	my ($ibx, $inot) = @_;
-	my $path = "$ibx->{inboxdir}/";
-	$path .= $ibx->version >= 2 ? 'inbox.lock' : 'ssoma.lock';
-	$inot->watch($path, $IN_CLOSE, sub { $ibx->on_unlock });
+	my ($ibx, $self) = @_;
+	my $dir = abs_path($ibx->{inboxdir});
+	if (!defined($dir)) {
+		warn "W: $ibx->{inboxdir} not watched: $!\n";
+		return;
+	}
+	my $inot = $self->{inot};
+	my $cur = $self->{pathmap}->{$dir} //= [];
+
+	# transfer old subscriptions to the current inbox, cancel the old watch
+	if (my $old_ibx = $cur->[0]) {
+		$ibx->{unlock_subs} and
+			die "BUG: $dir->{unlock_subs} should not exist";
+		$ibx->{unlock_subs} = $old_ibx->{unlock_subs};
+		$cur->[1]->cancel;
+	}
+	$cur->[0] = $ibx;
+
+	my $lock = "$dir/".($ibx->version >= 2 ? 'inbox.lock' : 'ssoma.lock');
+	$cur->[1] = $inot->watch($lock, $IN_CLOSE, sub { $ibx->on_unlock });
+
 	# TODO: detect deleted packs (and possibly other files)
 }
 
+sub refresh {
+	my ($self, $pi_config) = @_;
+	$pi_config->each_inbox(\&in2_arm, $self);
+}
+
 sub new {
 	my ($class, $pi_config) = @_;
 	my $self = fields::new($class);
@@ -42,7 +65,8 @@ sub new {
 		$inot = PublicInbox::FakeInotify->new;
 	}
 	$self->{inot} = $inot;
-	$pi_config->each_inbox(\&in2_arm, $inot);
+	$self->{pathmap} = {}; # inboxdir => [ ibx, watch1, watch2, watch3...]
+	refresh($self, $pi_config);
 	$self;
 }
 
diff --git a/t/imapd.t b/t/imapd.t
index 359c4c033b2..b0caa8f1779 100644
--- a/t/imapd.t
+++ b/t/imapd.t
@@ -6,6 +6,7 @@ use Test::More;
 use Time::HiRes ();
 use PublicInbox::TestCommon;
 use PublicInbox::Config;
+use PublicInbox::Spawn qw(which);
 require_mods(qw(DBD::SQLite Mail::IMAPClient Linux::Inotify2));
 my $level = '-Lbasic';
 SKIP: {
@@ -141,9 +142,12 @@ is_deeply([$mic->has_capability('COMPRESS')], ['DEFLATE'], 'deflate cap');
 ok($mic->compress, 'compress enabled');
 $compress_logout->($mic);
 
+my $have_inotify = eval { require Linux::Inotify2; 1 };
+
 my $pi_config = PublicInbox::Config->new;
 $pi_config->each_inbox(sub {
 	my ($ibx) = @_;
+	my $env = { ORIGINAL_RECIPIENT => $ibx->{-primary_address} };
 	my $name = $ibx->{name};
 	my $ng = $ibx->{newsgroup};
 	my $mic = Mail::IMAPClient->new(%mic_opt);
@@ -154,12 +158,62 @@ $pi_config->each_inbox(sub {
 	ok($mic->idle, "IDLE succeeds on $ng");
 
 	open(my $fh, '<', 't/data/message_embed.eml') or BAIL_OUT("open: $!");
-	my $env = { ORIGINAL_RECIPIENT => $ibx->{-primary_address} };
 	run_script(['-mda', '--no-precheck'], $env, { 0 => $fh }) or
 		BAIL_OUT('-mda delivery');
 	my $t0 = Time::HiRes::time();
 	ok(my @res = $mic->idle_data(11), "IDLE succeeds on $ng");
-	ok(grep(/\A\* [0-9] EXISTS\b/, @res), 'got EXISTS message');
+	is(grep(/\A\* [0-9] EXISTS\b/, @res), 1, 'got EXISTS message');
+	ok((Time::HiRes::time() - $t0) < 10, 'IDLE client notified');
+
+	my (@ino_info, $ino_fdinfo);
+	SKIP: {
+		skip 'no inotify support', 1 unless $have_inotify;
+		skip 'missing /proc/$PID/fd', 1 if !-d "/proc/$td->{pid}/fd";
+		my @ino = grep {
+			readlink($_) =~ /\binotify\b/
+		} glob("/proc/$td->{pid}/fd/*");
+		is(scalar(@ino), 1, 'only one inotify FD');
+		my $ino_fd = (split('/', $ino[0]))[-1];
+		$ino_fdinfo = "/proc/$td->{pid}/fdinfo/$ino_fd";
+		if (open my $fh, '<', $ino_fdinfo) {
+			local $/ = "\n";
+			@ino_info = grep(/^inotify wd:/, <$fh>);
+			ok(scalar(@ino_info), 'inotify has watches');
+		} else {
+			skip "$ino_fdinfo missing: $!", 1;
+		}
+	};
+
+	# ensure IDLE persists across HUP, w/o extra watches or FDs
+	$td->kill('HUP') or BAIL_OUT "failed to kill -imapd: $!";
+	SKIP: {
+		skip 'no inotify fdinfo (or support)', 2 if !@ino_info;
+		my (@tmp, %prev);
+		local $/ = "\n";
+		my $end = time + 5;
+		until (time > $end) {
+			select undef, undef, undef, 0.01;
+			open my $fh, '<', $ino_fdinfo or
+						BAIL_OUT "$ino_fdinfo: $!";
+			%prev = map { $_ => 1 } @ino_info;
+			@tmp = grep(/^inotify wd:/, <$fh>);
+			if (scalar(@tmp) == scalar(@ino_info)) {
+				delete @prev{@tmp};
+				last if scalar(keys(%prev)) == @ino_info;
+			}
+		}
+		is(scalar @tmp, scalar @ino_info,
+			'old inotify watches replaced');
+		is(scalar keys %prev, scalar @ino_info,
+			'no previous watches overlap');
+	};
+
+	open($fh, '<', 't/data/0001.patch') or BAIL_OUT("open: $!");
+	run_script(['-mda', '--no-precheck'], $env, { 0 => $fh }) or
+		BAIL_OUT('-mda delivery');
+	$t0 = Time::HiRes::time();
+	ok(@res = $mic->idle_data(11), "IDLE succeeds on $ng after HUP");
+	is(grep(/\A\* [0-9] EXISTS\b/, @res), 1, 'got EXISTS message');
 	ok((Time::HiRes::time() - $t0) < 10, 'IDLE client notified');
 });
 

  parent reply	other threads:[~2020-06-10  7:05 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-10  7:03 [PATCH 00/82] public-inbox-imapd: read-only IMAP server Eric Wong
2020-06-10  7:03 ` [PATCH 01/82] doc: add some IMAP standards Eric Wong
2020-06-10  7:03 ` [PATCH 02/82] nntpd: restrict allowed newsgroup names Eric Wong
2020-06-10  7:04 ` [PATCH 03/82] preliminary imap server implementation Eric Wong
2020-06-10  7:04 ` [PATCH 04/82] inboxidle: new class to detect inbox changes Eric Wong
2020-06-10  7:04 ` [PATCH 05/82] imap: support IDLE Eric Wong
2020-06-10  7:04 ` [PATCH 06/82] msgmap: split ->max into its own method Eric Wong
2020-06-10  7:04 ` Eric Wong [this message]
2020-06-10  7:04 ` [PATCH 08/82] imap: implement STATUS command Eric Wong
2020-06-10  7:04 ` [PATCH 09/82] imap: use Text::ParseWords::parse_line to handle quoted words Eric Wong
2020-06-10  7:04 ` [PATCH 10/82] imap: support LIST command Eric Wong
2020-06-10  7:04 ` [PATCH 11/82] t/imapd: support FakeInotify and KQNotify Eric Wong
2020-06-10  7:04 ` [PATCH 12/82] imap: support fetch for BODYSTRUCTURE and BODY Eric Wong
2020-06-10  7:04 ` [PATCH 13/82] eml: each_part: single part $idx is 1 Eric Wong
2020-06-10  7:04 ` [PATCH 14/82] imap: allow fetch of partial of BODY[...] and headers Eric Wong
2020-06-10  7:04 ` [PATCH 15/82] imap: always include `resp-text' in responses Eric Wong
2020-06-10  7:04 ` [PATCH 16/82] imap: split out unit tests and benchmarks Eric Wong
2020-06-10  7:04 ` [PATCH 17/82] imap: fix multi-message partial header fetches Eric Wong
2020-06-10  7:04 ` [PATCH 18/82] imap: simplify partial fetch structure Eric Wong
2020-06-10  7:04 ` [PATCH 19/82] imap: support sequence number FETCH Eric Wong
2020-06-10  7:04 ` [PATCH 20/82] imap: do not include ".PEEK" in responses Eric Wong
2020-06-10  7:04 ` [PATCH 21/82] imap: support the CLOSE command Eric Wong
2020-06-10  7:04 ` [PATCH 22/82] imap: speed up HEADER.FIELDS[.NOT] range fetches Eric Wong
2020-06-10  7:04 ` [PATCH 23/82] git: async: flatten the inflight array Eric Wong
2020-06-10  7:04 ` [PATCH 24/82] git: do our own read buffering for cat-file Eric Wong
2020-06-10  7:04 ` [PATCH 25/82] imap: use git-cat-file asynchronously Eric Wong
2020-06-10  7:04 ` [PATCH 26/82] git: idle rbuf for async Eric Wong
2020-06-10  7:04 ` [PATCH 27/82] imap: support LSUB command Eric Wong
2020-06-10  7:04 ` [PATCH 28/82] imap: FETCH: support comma-delimited ranges Eric Wong
2020-06-10  7:04 ` [PATCH 29/82] add imapd compression test Eric Wong
2020-06-10  7:04 ` [PATCH 30/82] testcommon: tcp_(server|connect): BAIL_OUT on failure Eric Wong
2020-06-10  7:04 ` [PATCH 31/82] *deflate: drop invalid comment about rbuf Eric Wong
2020-06-10  7:04 ` [PATCH 32/82] imap: fix pipelining with async git Eric Wong
2020-06-10  7:04 ` [PATCH 33/82] git: cat_async: provide requested OID + "missing" on missing blobs Eric Wong
2020-06-10  7:04 ` [PATCH 34/82] git: move async_cat reference to PublicInbox::Git Eric Wong
2020-06-10  7:04 ` [PATCH 35/82] git: async: automatic retry on alternates change Eric Wong
2020-06-10  7:04 ` [PATCH 36/82] imapclient: wrapper for Mail::IMAPClient Eric Wong
2020-06-10  7:04 ` [PATCH 37/82] xt: add imapd-validate and imapd-mbsync-oimap Eric Wong
2020-06-10  7:04 ` [PATCH 38/82] imap: support out-of-bounds ranges Eric Wong
2020-06-10  7:04 ` [PATCH 39/82] xt/perf-imap-list: time refresh_inboxlist Eric Wong
2020-06-10  7:04 ` [PATCH 40/82] imap: case-insensitive mailbox name comparisons Eric Wong
2020-06-10  7:04 ` [PATCH 41/82] imap: break giant inboxes into sub-inboxes of 50K messages Eric Wong
2020-06-10  7:04 ` [PATCH 42/82] imap: start doing iterative config reloading Eric Wong
2020-06-10  7:04 ` [PATCH 43/82] imap: require ".$UID_MIN-$UID_END" suffix Eric Wong
2020-06-10  7:04 ` [PATCH 44/82] imapd: ensure LIST is sorted alphabetically, for now Eric Wong
2020-06-10  7:04 ` [PATCH 45/82] imap: omit $UID_END from mailbox name, use index Eric Wong
2020-06-10  7:04 ` [PATCH 46/82] t/config.t: always compare against git bool behavior Eric Wong
2020-06-10  7:04 ` [PATCH 47/82] xt/*: show some tunable parameters Eric Wong
2020-06-10  7:04 ` [PATCH 48/82] imap: STATUS and LIST are case-insensitive, too Eric Wong
2020-06-10  7:04 ` [PATCH 49/82] imap: EXAMINE/STATUS: return correct counts Eric Wong
2020-06-10  7:04 ` [PATCH 50/82] imap: avoid uninitialized warnings on incomplete commands Eric Wong
2020-06-10  7:04 ` [PATCH 51/82] imap: start parsing out queries for SQLite and Xapian Eric Wong
2020-06-10  7:04 ` [PATCH 52/82] imap: SEARCH: clamp results to the 50K UID range Eric Wong
2020-06-10  7:04 ` [PATCH 53/82] imap: allow UID range search on timestamps Eric Wong
2020-06-10  7:04 ` [PATCH 54/82] over: get_art: use dbh->prepare_cached Eric Wong
2020-06-10  7:04 ` [PATCH 55/82] search: index byte size of a message for IMAP search Eric Wong
2020-06-10  7:04 ` [PATCH 56/82] search: index UID for IMAP search, too Eric Wong
2020-06-10  7:04 ` [PATCH 57/82] imap: remove dummies from sequence number FETCH Eric Wong
2020-06-10  7:04 ` [PATCH 58/82] imap: compile UID FETCH to opcodes Eric Wong
2020-06-10  7:04 ` [PATCH 59/82] imap: UID FETCH: optimize for smsg-only case Eric Wong
2020-06-10  7:04 ` [PATCH 60/82] imap: UID FETCH: optimize (UID FLAGS) harder Eric Wong
2020-06-10  7:04 ` [PATCH 61/82] imap: IDLE: avoid extraneous wakeups, keep-alive Eric Wong
2020-06-10  7:04 ` [PATCH 62/82] imap: 30 minute auto-logout timer Eric Wong
2020-06-10  7:05 ` [PATCH 63/82] imap: split ->logged_in attribute into a separate class Eric Wong
2020-06-10  7:05 ` [PATCH 64/82] searchidx: v1 (re)-index uses git asynchronously Eric Wong
2020-06-10  7:05 ` [PATCH 65/82] index: account for CRLF conversion when storing bytes Eric Wong
2020-06-10  7:05 ` [PATCH 66/82] imap: rely on smsg->{bytes} for RFC822.SIZE Eric Wong
2020-06-10  7:05 ` [PATCH 67/82] imap: UID FETCH requires at least one data item Eric Wong
2020-06-10  7:05 ` [PATCH 68/82] imap: LIST shows "INBOX" in all caps Eric Wong
2020-06-10  7:05 ` [PATCH 69/82] imap: support 8000 octet lines Eric Wong
2020-06-10  7:05 ` [PATCH 70/82] imap: reinstate some message sequence number support Eric Wong
2020-06-10  7:05 ` [PATCH 71/82] imap: cleanup ->{uid_base} usage Eric Wong
2020-06-10  7:05 ` [PATCH 72/82] imap: FETCH: more granular CRLF conversion Eric Wong
2020-06-10  7:05 ` [PATCH 73/82] imap: further speed up HEADER.FIELDS FETCH requests Eric Wong
2020-06-10  7:05 ` [PATCH 74/82] imap: FETCH: try to make fake MSNs sequentially Eric Wong
2020-06-10  7:05 ` [PATCH 75/82] imap: STATUS/EXAMINE: rely on SQLite overview Eric Wong
2020-06-10  7:05 ` [PATCH 76/82] imap: UID SEARCH: support multiple ranges Eric Wong
2020-06-10  7:05 ` [PATCH 77/82] imap: wire up Xapian, MSN SEARCH and multi sequence-sets Eric Wong
2020-06-10  7:05 ` [PATCH 78/82] imap: misc cleanups and notes Eric Wong
2020-06-10  7:05 ` [PATCH 79/82] imapd: don't bother sorting LIST output Eric Wong
2020-06-10  7:05 ` [PATCH 80/82] imap: remove non-UID SEARCH for now Eric Wong
2020-06-10  7:05 ` [PATCH 81/82] over: uid_range: remove LIMIT Eric Wong
2020-06-10  7:05 ` [PATCH 82/82] imap: FETCH: proper MSN => UID mapping for requests Eric Wong
2020-06-12 23:49 ` [PATCH 83/82] imap: introduce memory-efficient uo2m mapping 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: https://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=20200610070519.18252-8-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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/public-inbox.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).