imap: delay InboxIdle start, support refresh

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.

diff --git a/lib/PublicInbox/IMAP.pm b/lib/PublicInbox/IMAP.pm
index 4a43185c51262baffb9abbafce7eea758c2c3447..c8592dc03291f40fe1a921675850e7f4df7006c2 100644 (file)
--- 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 1922c16046a3119723e560eb18fdfc010bbc9bd2..05aa30e42a15acb30e288896016c3d1f5df3fa3d 100644 (file)
--- 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 095a801c946514099639f7e82db80e23e3a46331..c19b8d186cdcdfd88f13d1f38d11c2d835b5d850 100644 (file)
--- 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 359c4c033b28bfecf6fd8b1a39a46e76e13394f3..b0caa8f17796c1540767f84345ce542486b212a1 100644 (file)
--- 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');
 });