about summary refs log tree commit homepage
path: root/lib/PublicInbox/Xapcmd.pm
diff options
context:
space:
mode:
authorEric Wong <e@80x24.org>2019-11-14 01:12:11 +0000
committerEric Wong <e@80x24.org>2019-11-14 06:43:18 +0000
commit6466b21e0776fdd88648730e7248d6887d380224 (patch)
treea0cad403b0fbae219906ec04886ae7f938684c93 /lib/PublicInbox/Xapcmd.pm
parent45fb16664775746dc4b3f7a55743dbe622a3c5ae (diff)
downloadpublic-inbox-6466b21e0776fdd88648730e7248d6887d380224.tar.gz
InboxWritable caching the result of ->importer leads to a
circular references with returned (V2Writable|Import) object
holds onto the calling InboxWritable object.

With public-inbox-watch, this leads to a memory leak if a user
is reloading via SIGHUP after a message is imported (it would
only become noticeable with SIGHUPs after every message imported).

I would not expect anybody to to notice this in real-world
usage.  I only noticed this since I was making -xcpdb suitable
for long-lived process use (e.g. "mod_perl style") and a flock
remained unreleased on v1 inboxes after resharding.

WatchMaildir (used by -watch) already handles caching of the
importer object itself, and all of our other real-world uses of
->importer are short-lived or designed for batch scripts, so
there's no need to cache the importer result internally.
Diffstat (limited to 'lib/PublicInbox/Xapcmd.pm')
-rw-r--r--lib/PublicInbox/Xapcmd.pm25
1 files changed, 10 insertions, 15 deletions
diff --git a/lib/PublicInbox/Xapcmd.pm b/lib/PublicInbox/Xapcmd.pm
index 95d472b0..c807bf10 100644
--- a/lib/PublicInbox/Xapcmd.pm
+++ b/lib/PublicInbox/Xapcmd.pm
@@ -15,12 +15,10 @@ use File::Basename qw(dirname);
 our $XAPIAN_COMPACT = $ENV{XAPIAN_COMPACT} || 'xapian-compact';
 our @COMPACT_OPT = qw(jobs|j=i quiet|q blocksize|b=s no-full|n fuller|F);
 
-sub commit_changes ($$$) {
-        my ($ibx, $tmp, $opt) = @_;
+sub commit_changes ($$$$) {
+        my ($ibx, $im, $tmp, $opt) = @_;
         my $reshard = $opt->{reshard};
         my $reindex = $opt->{reindex};
-        my $im = $ibx->importer(0);
-        $im->lock_acquire if !$opt->{-coarse_lock};
 
         $SIG{INT} or die 'BUG: $SIG{INT} not handled';
         my @old_shard;
@@ -78,10 +76,7 @@ sub commit_changes ($$$) {
                         }
                 }
 
-                PublicInbox::Admin::index_inbox($ibx, $opt);
-                # implicit lock_release
-        } else {
-                $im->lock_release;
+                PublicInbox::Admin::index_inbox($ibx, $im, $opt);
         }
 }
 
@@ -98,8 +93,8 @@ sub runnable_or_die ($) {
         which($exe) or die "$exe not found in PATH\n";
 }
 
-sub prepare_reindex ($$) {
-        my ($ibx, $reindex) = @_;
+sub prepare_reindex ($$$) {
+        my ($ibx, $im, $reindex) = @_;
         if ($ibx->{version} == 1) {
                 my $dir = $ibx->search->xdir(1);
                 my $xdb = Search::Xapian::Database->new($dir);
@@ -107,9 +102,8 @@ sub prepare_reindex ($$) {
                         $reindex->{from} = $lc;
                 }
         } else { # v2
-                my $v2w = $ibx->importer(0);
                 my $max;
-                $v2w->git_dir_latest(\$max) or return;
+                $im->git_dir_latest(\$max) or return;
                 my $from = $reindex->{from};
                 my $mm = $ibx->mm;
                 my $v = PublicInbox::Search::SCHEMA_VERSION();
@@ -229,20 +223,21 @@ sub run {
                         $tmp->{$_} ||= undef for @$src;
                 }
         }
-        my $im = $ibx->importer(0);
         my $max = $opt->{jobs} || scalar(@q);
         $ibx->with_umask(sub {
+                my $im = $ibx->importer(0);
                 $im->lock_acquire;
 
                 # fine-grained locking if we prepare for reindex
                 if (!$opt->{-coarse_lock}) {
-                        prepare_reindex($ibx, $reindex);
+                        prepare_reindex($ibx, $im, $reindex);
                         $im->lock_release;
                 }
 
                 delete($ibx->{$_}) for (qw(mm over search)); # cleanup
                 process_queue(\@q, $cb, $max, $opt);
-                commit_changes($ibx, $tmp, $opt);
+                $im->lock_acquire if !$opt->{-coarse_lock};
+                commit_changes($ibx, $im, $tmp, $opt);
         });
 }