What started off as a trivial, "something-to-do-before-bedtime" change several days ago (15/15) ended up forcing me to fix various hard-to-reproduce race conditions around Maildirs. Eventually, `schedtool -a 0x1 -e ...' helped me reproduce some races more easily (but still not with 100% reliability). t/lei-{watch,auto-watch,export-kw}.t should all be more reliable, now. There's also several other minor fixes I found along the way... Eric Wong (15): t/lei-{auto-watch,export-kw}: extra diagnostics on failure t/lei-import-maildir: rename fix (SR -> RS) t/lei-p2q: extra diagnostics lei/store: check for any unexpected process death lei note-event: drop unnecessary eval guard lei note-event: wq_io_do => wq_do lei_search: try harder to associate "lei index"-ed messages watch: check for {quit} before IDLE watch: remove redundant signal mask manipulation doc: lei-overview: add CAVEATS section lei note-event: clear_src on ENOENT dir_idle: treat IN_MOVED_FROM as a gone event lei: no Perl FileHandle for `undef' w/ ECONNRESET lei_mail_sync: mv_src: use transaction, check UNIQUE lei: use RENAME_NOREPLACE on Linux 3.15+ Documentation/lei-overview.pod | 5 ++++ MANIFEST | 1 + devel/syscall-list | 8 +++++- lib/PublicInbox/DirIdle.pm | 3 +- lib/PublicInbox/FakeInotify.pm | 3 ++ lib/PublicInbox/LEI.pm | 3 +- lib/PublicInbox/LeiExportKw.pm | 19 ++++--------- lib/PublicInbox/LeiMailSync.pm | 8 ++++-- lib/PublicInbox/LeiNoteEvent.pm | 13 +++++---- lib/PublicInbox/LeiSearch.pm | 13 ++++++++- lib/PublicInbox/LeiStore.pm | 16 +++++++---- lib/PublicInbox/LeiToMail.pm | 7 ++--- lib/PublicInbox/Syscall.pm | 49 +++++++++++++++++++++++++++++++-- lib/PublicInbox/Watch.pm | 13 +++------ t/lei-auto-watch.t | 3 +- t/lei-export-kw.t | 39 ++++++++++++++++++-------- t/lei-import-maildir.t | 2 +- t/lei-p2q.t | 2 +- t/rename_noreplace.t | 26 +++++++++++++++++ 19 files changed, 173 insertions(+), 60 deletions(-) create mode 100644 t/rename_noreplace.t
Maybe these will help track down some failures and make diagnosing bugs easier. "lei export-kw" should also become optional, even, so allow disabling it easily in the test. --- t/lei-auto-watch.t | 3 ++- t/lei-export-kw.t | 39 ++++++++++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/t/lei-auto-watch.t b/t/lei-auto-watch.t index e5e132eb3bfd..d5661ae5d618 100644 --- a/t/lei-auto-watch.t +++ b/t/lei-auto-watch.t @@ -41,7 +41,8 @@ test_lei(sub { $ins = json_utf8->decode($lei_out); $exp = { "maildir:$x" => [ map { basename($_) } glob("$x/*/*") ], "maildir:$y" => [ map { basename($_) } glob("$y/*/*") ] }; - is_deeply($ins->{'mail-sync'}, $exp, 'mail_sync matches FS'); + is_deeply($ins->{'mail-sync'}, $exp, 'mail_sync matches FS') or + diag explain($ins); }); done_testing; diff --git a/t/lei-export-kw.t b/t/lei-export-kw.t index 1fe940bb6d89..55730e87c050 100644 --- a/t/lei-export-kw.t +++ b/t/lei-export-kw.t @@ -7,28 +7,45 @@ use File::Path qw(make_path); require_mods(qw(lei -imapd Mail::IMAPClient)); my ($tmpdir, $for_destroy) = tmpdir; my $expect = eml_load('t/data/0001.patch'); +my $do_export_kw = 1; +my $wait_for = sub { + my ($f) = @_; + lei_ok qw(export-kw --all=local) if $do_export_kw; + my $x = $f; + $x =~ s!\Q$tmpdir\E/!\$TMPDIR/!; + for (0..10) { + last if -f $f; + diag "tick #$_ $x"; + tick(0.1); + } + ok(-f $f, "$x exists") or xbail; +}; + test_lei({ tmpdir => $tmpdir }, sub { my $home = $ENV{HOME}; my $md = "$home/md"; + my $f; make_path("$md/new", "$md/cur", "$md/tmp"); cp('t/data/0001.patch', "$md/new/y") or xbail "cp $md $!"; cp('t/data/message_embed.eml', "$md/cur/x:2,S") or xbail "cp $md $!"; - lei_ok qw(index -q), $md; + lei_ok qw(index), $md; lei_ok qw(tag t/data/0001.patch +kw:seen); - lei_ok qw(export-kw --all=local); - ok(!-e "$md/new/y", 'original gone'); - is_deeply(eml_load("$md/cur/y:2,S"), $expect, - "`seen' kw exported"); + $wait_for->($f = "$md/cur/y:2,S"); + ok(!-e "$md/new/y", 'original gone') or + diag explain([glob("$md/*/*")]); + is_deeply(eml_load($f), $expect, "`seen' kw exported"); lei_ok qw(tag t/data/0001.patch +kw:answered); - lei_ok qw(export-kw --all=local); - ok(!-e "$md/cur/y:2,S", 'seen-only file gone'); - is_deeply(eml_load("$md/cur/y:2,RS"), $expect, "`R' added"); + $wait_for->($f = "$md/cur/y:2,RS"); + ok(!-e "$md/cur/y:2,S", 'seen-only file gone') or + diag explain([glob("$md/*/*")]); + is_deeply(eml_load($f), $expect, "`R' added"); lei_ok qw(tag t/data/0001.patch -kw:answered -kw:seen); - lei_ok qw(export-kw --mode=set --all=local); - ok(!-e "$md/cur/y:2,RS", 'seen+answered file gone'); - is_deeply(eml_load("$md/cur/y:2,"), $expect, 'no keywords left'); + $wait_for->($f = "$md/cur/y:2,"); + ok(!-e "$md/cur/y:2,RS", 'seen+answered file gone') or + diag explain([glob("$md/*/*")]); + is_deeply(eml_load($f), $expect, 'no keywords left'); }); done_testing;
While it doesn't matter to us, the Maildir spec specifies characters are to be sorted in alphabetical order. --- t/lei-import-maildir.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/lei-import-maildir.t b/t/lei-import-maildir.t index c81e7805fe7f..1e7eddd571d8 100644 --- a/t/lei-import-maildir.t +++ b/t/lei-import-maildir.t @@ -52,7 +52,7 @@ test_lei(sub { my $r2 = json_utf8->decode($lei_out); is_deeply($r2, $res, 'idempotent import') or diag explain($imp_err, $res); - rename("$md/cur/x:2,S", "$md/cur/x:2,SR") or BAIL_OUT "rename: $!"; + rename("$md/cur/x:2,S", "$md/cur/x:2,RS") or BAIL_OUT "rename: $!"; lei_ok('import', "maildir:$md", \'import Maildir after +answered'); lei_ok(qw(q -d none s:boolean), \'lei q after +answered'); $res = json_utf8->decode($lei_out);
I got one mysterious test failure here, once, and can't seem to reproduce it... --- t/lei-p2q.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/lei-p2q.t b/t/lei-p2q.t index 495d81de78a5..bf40a43be466 100644 --- a/t/lei-p2q.t +++ b/t/lei-p2q.t @@ -7,7 +7,7 @@ require_mods(qw(json DBD::SQLite Search::Xapian)); test_lei(sub { ok(!lei(qw(p2q this-better-cause-format-patch-to-fail)), - 'p2q fails on bogus arg'); + 'p2q fails on bogus arg') or diag $lei_err; like($lei_err, qr/format-patch.*failed/, 'notes format-patch failure'); lei_ok(qw(p2q -w dfpost t/data/0001.patch)); is($lei_out, "dfpost:6e006fd73b1d\n", 'pathname') or diag $lei_err;
The lei/store process should only exit from EOF on the socket, so make sure we note any unintended signals --- lib/PublicInbox/LeiStore.pm | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm index 821045701dfe..16e7d302dc2f 100644 --- a/lib/PublicInbox/LeiStore.pm +++ b/lib/PublicInbox/LeiStore.pm @@ -571,6 +571,12 @@ sub recv_and_run { $self->SUPER::recv_and_run(@args); } +sub _sto_atexit { # dwaitpid callback + my ($args, $pid) = @_; + my $self = $args->[0]; + warn "lei/store PID:$pid died \$?=$?\n" if $?; +} + sub write_prepare { my ($self, $lei) = @_; $lei // die 'BUG: $lei not passed'; @@ -587,7 +593,7 @@ sub write_prepare { -err_wr => $w, to_close => [ $r ], }); - $self->wq_wait_async; # outlives $lei + $self->wq_wait_async(\&_sto_atexit); # outlives $lei require PublicInbox::LeiStoreErr; PublicInbox::LeiStoreErr->new($r, $lei); }
We don't want to lose the failure message in case note-event fails. --- lib/PublicInbox/LeiNoteEvent.pm | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/PublicInbox/LeiNoteEvent.pm b/lib/PublicInbox/LeiNoteEvent.pm index 1749c98f8312..0709754f6d2a 100644 --- a/lib/PublicInbox/LeiNoteEvent.pm +++ b/lib/PublicInbox/LeiNoteEvent.pm @@ -72,8 +72,7 @@ sub lei_note_event { my $lms = $lei->lms or return; $lms->lms_write_prepare if $new_cur eq ''; # for ->clear_src below $lei->{opt}->{quiet} = 1; - eval { $lms->arg2folder($lei, [ $folder ]) }; - return if $@; + $lms->arg2folder($lei, [ $folder ]); my $state = $cfg->get_1("watch.$folder.state") // 'tag-rw'; return if $state eq 'pause'; return $lms->clear_src($folder, \$bn) if $new_cur eq '';
No need to pass extra arrayref args, here. --- lib/PublicInbox/LeiNoteEvent.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/PublicInbox/LeiNoteEvent.pm b/lib/PublicInbox/LeiNoteEvent.pm index 0709754f6d2a..3472e73070d6 100644 --- a/lib/PublicInbox/LeiNoteEvent.pm +++ b/lib/PublicInbox/LeiNoteEvent.pm @@ -97,7 +97,7 @@ sub lei_note_event { return if index($fl, 'T') >= 0; my $kw = PublicInbox::MdirReader::flags2kw($fl); my $vmd = { kw => $kw, sync_info => [ $folder, \$bn ] }; - $self->wq_io_do('maildir_event', [], $fn, $vmd, $state); + $self->wq_do('maildir_event', $fn, $vmd, $state); } # else: TODO: imap }
Allow checking for keyword changes if we have an known OID, even if the blob isn't currently reachable. --- lib/PublicInbox/LeiSearch.pm | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/PublicInbox/LeiSearch.pm b/lib/PublicInbox/LeiSearch.pm index 3e046b2146c6..1fb38da1d7aa 100644 --- a/lib/PublicInbox/LeiSearch.pm +++ b/lib/PublicInbox/LeiSearch.pm @@ -7,7 +7,7 @@ use strict; use v5.10.1; use parent qw(PublicInbox::ExtSearch); # PublicInbox::Search->reopen use PublicInbox::Search qw(xap_terms); -use PublicInbox::ContentHash qw(content_digest content_hash); +use PublicInbox::ContentHash qw(content_digest content_hash git_sha); use PublicInbox::MID qw(mids mids_for_index); use Carp qw(croak); @@ -118,6 +118,13 @@ sub xoids_for { } } $git->async_wait_all; + + # it could be an 'lei index'-ed file that just got renamed + if (scalar(keys %$xoids) < ($min // 1) && defined($self->{topdir})) { + my $hex = git_sha(1, $eml)->hexdigest; + my @n = $overs[0]->blob_exists($hex); + for (@n) { $xoids->{$hex} //= $_ } + } scalar(keys %$xoids) ? $xoids : undef; } @@ -129,6 +136,10 @@ sub kw_changed { my $xoids = xoids_for($self, $eml) // return; $docids //= []; @$docids = sort { $a <=> $b } values %$xoids; + if (!@$docids && $self->over) { + my $bin = git_sha(1, $eml)->digest; + @$docids = $self->over->oidbin_exists($bin); + } } for my $id (@$docids) { $cur_kw = eval { msg_keywords($self, $id) } and last;
This may make it less likely for watch-dependent tests to get stuck. Unfortunately, due to the synchronous API of Mail::IMAPClient, ->idle is still susceptible to missing signals. --- lib/PublicInbox/Watch.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/PublicInbox/Watch.pm b/lib/PublicInbox/Watch.pm index b48d9cccc3e2..e80bbdec16fd 100644 --- a/lib/PublicInbox/Watch.pm +++ b/lib/PublicInbox/Watch.pm @@ -330,6 +330,7 @@ sub imap_idle_once ($$$$) { my $end = now() + $intvl; warn "I: $uri idling for ${intvl}s\n"; local $0 = "IDLE $0"; + return if $self->{quit}; unless ($mic->idle) { return if $self->{quit}; return "E: IDLE failed on $uri: $!";
The top-level daemon process already blocks all signals, so there's no reason to block them around fork() calls. --- lib/PublicInbox/Watch.pm | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/PublicInbox/Watch.pm b/lib/PublicInbox/Watch.pm index e80bbdec16fd..3f6fe21b726f 100644 --- a/lib/PublicInbox/Watch.pm +++ b/lib/PublicInbox/Watch.pm @@ -391,11 +391,7 @@ sub watch_atfork_child ($) { PublicInbox::DS::sig_setmask($self->{oldset}); } -sub watch_atfork_parent ($) { - my ($self) = @_; - _done_for_now($self); - PublicInbox::DS::block_signals(); -} +sub watch_atfork_parent ($) { _done_for_now($_[0]) } sub imap_idle_requeue { # DS::add_timer callback my ($self, $uri_intvl) = @_; @@ -449,13 +445,12 @@ sub event_step { return if $self->{quit}; my $idle_todo = $self->{idle_todo}; if ($idle_todo && @$idle_todo) { - my $oldset = watch_atfork_parent($self); + watch_atfork_parent($self); eval { while (my $uri_intvl = shift(@$idle_todo)) { imap_idle_fork($self, $uri_intvl); } }; - PublicInbox::DS::sig_setmask($oldset); die $@ if $@; } fs_scan_step($self) if $self->{mdre}; @@ -492,7 +487,7 @@ sub poll_fetch_fork { # DS::add_timer callback my ($self, $intvl, $uris) = @_; return if $self->{quit}; pipe(my ($r, $w)) or die "pipe: $!"; - my $oldset = watch_atfork_parent($self); + watch_atfork_parent($self); my $seed = rand(0xffffffff); my $pid = fork; if (defined($pid) && $pid == 0) { @@ -508,7 +503,6 @@ sub poll_fetch_fork { # DS::add_timer callback close $w; _exit(0); } - PublicInbox::DS::sig_setmask($oldset); die "fork: $!" unless defined $pid; $self->{poll_pids}->{$pid} = [ $intvl, $uris ]; PublicInbox::EOFpipe->new($r, \&reap, [$pid, \&poll_fetch_reap, $self]);
IMAP and NNTP client performance absolutely sucks compared to what the read-only daemons are capable of... --- Documentation/lei-overview.pod | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/lei-overview.pod b/Documentation/lei-overview.pod index bb2fe50f7cd9..99fd6ef72174 100644 --- a/Documentation/lei-overview.pod +++ b/Documentation/lei-overview.pod @@ -137,6 +137,11 @@ Since lei runs as a daemon, L<lei-daemon-kill(1)> is required to kill the daemon so it can load new code. It will be restarted with the next invocation of any lei command. +=head1 CAVEATS + +IMAP and NNTP client performance is poor on high-latency connections. +It will hopefully be fixed in 2022. + =head1 CONTACT Feedback welcome via plain-text mail to L<mailto:meta@public-inbox.org>
When a file goes away, try to make sure we don't waste time trying to access or store it. --- lib/PublicInbox/LeiNoteEvent.pm | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/PublicInbox/LeiNoteEvent.pm b/lib/PublicInbox/LeiNoteEvent.pm index 3472e73070d6..22d6ffac9feb 100644 --- a/lib/PublicInbox/LeiNoteEvent.pm +++ b/lib/PublicInbox/LeiNoteEvent.pm @@ -8,6 +8,7 @@ use strict; use v5.10.1; use parent qw(PublicInbox::IPC); use PublicInbox::DS; +use Errno qw(ENOENT); our $to_flush; # { cfgpath => $lei } @@ -59,8 +60,11 @@ sub eml_event ($$$$) { sub maildir_event { # via wq_io_do my ($self, $fn, $vmd, $state) = @_; - my $eml = PublicInbox::InboxWritable::eml_from_path($fn) // return; - eml_event($self, $eml, $vmd, $state); + if (my $eml = PublicInbox::InboxWritable::eml_from_path($fn)) { + eml_event($self, $eml, $vmd, $state); + } elsif ($! == ENOENT) { + $self->{lms}->clear_src(@{$vmd->{sync_info}}); + } # else: eml_from_path already warns } sub lei_note_event {
Whether an MUA uses rename(2) or link(2)+unlink(2) combination should not matter to us. We should be able to handle both cases. --- lib/PublicInbox/DirIdle.pm | 3 ++- lib/PublicInbox/FakeInotify.pm | 3 +++ lib/PublicInbox/LEI.pm | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/PublicInbox/DirIdle.pm b/lib/PublicInbox/DirIdle.pm index 270d3829bc3e..9206da9cb710 100644 --- a/lib/PublicInbox/DirIdle.pm +++ b/lib/PublicInbox/DirIdle.pm @@ -14,7 +14,8 @@ if ($^O eq 'linux' && eval { require Linux::Inotify2; 1 }) { Linux::Inotify2::IN_CREATE(); $MAIL_GONE = Linux::Inotify2::IN_DELETE() | Linux::Inotify2::IN_DELETE_SELF() | - Linux::Inotify2::IN_MOVE_SELF(); + Linux::Inotify2::IN_MOVE_SELF() | + Linux::Inotify2::IN_MOVED_FROM(); $ino_cls = 'Linux::Inotify2'; # Perl 5.22+ is needed for fileno(DIRHANDLE) support: } elsif ($^V ge v5.22 && eval { require PublicInbox::KQNotify }) { diff --git a/lib/PublicInbox/FakeInotify.pm b/lib/PublicInbox/FakeInotify.pm index 641bc5bdc7c6..6d2696019d83 100644 --- a/lib/PublicInbox/FakeInotify.pm +++ b/lib/PublicInbox/FakeInotify.pm @@ -10,6 +10,7 @@ use parent qw(Exporter); use Time::HiRes qw(stat); use PublicInbox::DS qw(add_timer); sub IN_MODIFY () { 0x02 } # match Linux inotify +# my $IN_MOVED_FROM 0x00000040 /* File was moved from X. */ # my $IN_MOVED_TO = 0x80; # my $IN_CREATE = 0x100; sub MOVED_TO_OR_CREATE () { 0x80 | 0x100 } @@ -136,6 +137,7 @@ use strict; sub fullname { ${$_[0]} } sub IN_DELETE { 0 } +sub IN_MOVED_FROM { 0 } sub IN_DELETE_SELF { 0 } package PublicInbox::FakeInotify::GoneEvent; @@ -143,6 +145,7 @@ use strict; our @ISA = qw(PublicInbox::FakeInotify::Event); sub IN_DELETE { 1 } +sub IN_MOVED_FROM { 0 } package PublicInbox::FakeInotify::SelfGoneEvent; use strict; diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm index c1f28f7b31bf..b68e526bf365 100644 --- a/lib/PublicInbox/LEI.pm +++ b/lib/PublicInbox/LEI.pm @@ -1198,7 +1198,7 @@ sub dir_idle_handler ($) { # PublicInbox::DirIdle callback my $fn = $ev->fullname; if ($fn =~ m!\A(.+)/(new|cur)/([^/]+)\z!) { # Maildir file my ($mdir, $nc, $bn) = ($1, $2, $3); - $nc = '' if $ev->IN_DELETE; + $nc = '' if $ev->IN_DELETE || $ev->IN_MOVED_FROM; for my $f (keys %{$MDIR2CFGPATH->{$mdir} // {}}) { my $cfg = $PATH2CFG{$f} // next; eval {
Error reporting for recv_cmd4 methods is a bit wonky. --- lib/PublicInbox/LEI.pm | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm index b68e526bf365..43baeeb3d51c 100644 --- a/lib/PublicInbox/LEI.pm +++ b/lib/PublicInbox/LEI.pm @@ -1129,6 +1129,7 @@ sub event_step { if (scalar(@fds) == 1 && !defined($fds[0])) { return if $! == EAGAIN; die "recvmsg: $!" if $! != ECONNRESET; + @fds = (); # for open loop below: } for (@fds) { open my $rfh, '+<&=', $_ } if ($buf eq '') {
We need a transaction across two SQL statements so readers (which don't use flock) will see the result as atomic. This may help against some occasional test failures I'm seeing from t/lei-auto-watch.t and t/lei-watch.t, or make the problem more apparent. --- lib/PublicInbox/LeiMailSync.pm | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm index e70cb5de2b6b..124eb96948c5 100644 --- a/lib/PublicInbox/LeiMailSync.pm +++ b/lib/PublicInbox/LeiMailSync.pm @@ -182,16 +182,20 @@ sub mv_src { my ($self, $folder, $oidbin, $id, $newbn) = @_; my $lk = $self->lock_for_scope; my $fid = $self->{fmap}->{$folder} //= fid_for($self, $folder, 1); + $self->{dbh}->begin_work; my $sth = $self->{dbh}->prepare_cached(<<''); UPDATE blob2name SET name = ? WHERE fid = ? AND oidbin = ? AND name = ? - my $nr = $sth->execute($newbn, $fid, $oidbin, $$id); - if ($nr == 0) { # may race with a clear_src, ensure new value exists + # eval since unique constraint may fail due to race + my $nr = eval { $sth->execute($newbn, $fid, $oidbin, $$id) }; + if (!defined($nr) || $nr == 0) { # $nr may be `0E0' + # may race with a clear_src, ensure new value exists $sth = $self->{dbh}->prepare_cached(<<''); INSERT OR IGNORE INTO blob2name (oidbin, fid, name) VALUES (?, ?, ?) $sth->execute($oidbin, $fid, $newbn); } + $self->{dbh}->commit; } # read-only, iterates every oidbin + UID or name for a given folder
One syscall is better than two for atomicity in Maildirs. This means there's no window where another process can see both the old and new file at the same time (link && unlink), nor a window where we might inadvertantly clobber an existing file if we were to do `stat && rename'. --- MANIFEST | 1 + devel/syscall-list | 8 +++++- lib/PublicInbox/LeiExportKw.pm | 19 +++++-------- lib/PublicInbox/LeiStore.pm | 8 +++--- lib/PublicInbox/LeiToMail.pm | 7 +++-- lib/PublicInbox/Syscall.pm | 49 +++++++++++++++++++++++++++++++--- t/rename_noreplace.t | 26 ++++++++++++++++++ 7 files changed, 92 insertions(+), 26 deletions(-) create mode 100644 t/rename_noreplace.t diff --git a/MANIFEST b/MANIFEST index af1522d71bd1..9fd979ef02fb 100644 --- a/MANIFEST +++ b/MANIFEST @@ -528,6 +528,7 @@ t/psgi_v2.t t/purge.t t/qspawn.t t/reindex-time-range.t +t/rename_noreplace.t t/replace.t t/reply.t t/run.perl diff --git a/devel/syscall-list b/devel/syscall-list index b33401d98ce4..3d55df1fc1d7 100755 --- a/devel/syscall-list +++ b/devel/syscall-list @@ -1,4 +1,4 @@ -# Copyright 2021 all contributors <meta@public-inbox.org> +# Copyright all contributors <meta@public-inbox.org> # License: AGPL-3.0+ <http://www.gnu.org/licenses/agpl-3.0.txt> # Dump syscall numbers under Linux and any other kernel which # promises stable syscall numbers. This is to maintain @@ -9,7 +9,10 @@ eval 'exec perl -S $0 ${1+"$@"}' # no shebang if 0; # running under some shell use strict; +use v5.10.1; use File::Temp 0.19; +use POSIX qw(uname); +say '$machine='.(POSIX::uname())[-1]; my $cc = $ENV{CC} // 'cc'; my @cflags = split(/\s+/, $ENV{CFLAGS} // '-Wall'); my $str = do { local $/; <DATA> }; @@ -43,6 +46,9 @@ int main(void) D(SYS_inotify_add_watch); D(SYS_inotify_rm_watch); D(SYS_prctl); +#ifdef SYS_renameat2 + D(SYS_renameat2); +#endif #endif /* Linux, any other OSes with stable syscalls? */ printf("size_t=%zu off_t=%zu\n", sizeof(size_t), sizeof(off_t)); return 0; diff --git a/lib/PublicInbox/LeiExportKw.pm b/lib/PublicInbox/LeiExportKw.pm index 0b65c2762633..ceeef7f21d54 100644 --- a/lib/PublicInbox/LeiExportKw.pm +++ b/lib/PublicInbox/LeiExportKw.pm @@ -7,6 +7,7 @@ use strict; use v5.10.1; use parent qw(PublicInbox::IPC PublicInbox::LeiInput); use Errno qw(EEXIST ENOENT); +use PublicInbox::Syscall qw(rename_noreplace); sub export_kw_md { # LeiMailSync->each_src callback my ($oidbin, $id, $self, $mdir) = @_; @@ -30,30 +31,22 @@ sub export_kw_md { # LeiMailSync->each_src callback my $lei = $self->{lei}; for my $d (@try) { my $src = "$mdir/$d/$$id"; - - # we use link(2) + unlink(2) since rename(2) may - # inadvertently clobber if the "uniquefilename" part wasn't - # actually unique. - if (link($src, $dst)) { # success - # unlink(2) may ENOENT from parallel invocation, - # ignore it, but not other serious errors - if (!unlink($src) and $! != ENOENT) { - $lei->child_error(1, "E: unlink($src): $!"); - } + if (rename_noreplace($src, $dst)) { # success $self->{lms}->mv_src("maildir:$mdir", $oidbin, $id, $bn); - return; # success anyways if link(2) worked + return; # success } elsif ($! == EEXIST) { # lost race with lei/store? return; } elsif ($! != ENOENT) { - $lei->child_error(1, "E: link($src -> $dst): $!"); + $lei->child_error(1, + "E: rename_noreplace($src -> $dst): $!"); } # else loop @try } my $e = $!; # both tries failed my $oidhex = unpack('H*', $oidbin); my $src = "$mdir/{".join(',', @try)."}/$$id"; - $lei->child_error(1, "link($src -> $dst) ($oidhex): $e"); + $lei->child_error(1, "rename_noreplace($src -> $dst) ($oidhex): $e"); for (@try) { return if -e "$mdir/$_/$$id" } $self->{lms}->clear_src("maildir:$mdir", $id); } diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm index 16e7d302dc2f..f1316229bb32 100644 --- a/lib/PublicInbox/LeiStore.pm +++ b/lib/PublicInbox/LeiStore.pm @@ -32,6 +32,7 @@ use POSIX (); use IO::Handle (); # ->autoflush use Sys::Syslog qw(syslog openlog); use Errno qw(EEXIST ENOENT); +use PublicInbox::Syscall qw(rename_noreplace); sub new { my (undef, $dir, $opt) = @_; @@ -185,10 +186,7 @@ sub export1_kw_md ($$$$$) { my $dst = "$mdir/cur/$bn"; for my $d (@try) { my $src = "$mdir/$d/$orig"; - if (link($src, $dst)) { - if (!unlink($src) and $! != ENOENT) { - syslog('warning', "unlink($src): $!"); - } + if (rename_noreplace($src, $dst)) { # TODO: verify oidbin? $self->{lms}->mv_src("maildir:$mdir", $oidbin, \$orig, $bn); @@ -196,7 +194,7 @@ sub export1_kw_md ($$$$$) { } elsif ($! == EEXIST) { # lost race with "lei export-kw"? return; } elsif ($! != ENOENT) { - syslog('warning', "link($src -> $dst): $!"); + syslog('warning', "rename_noreplace($src -> $dst): $!"); } } for (@try) { return if -e "$mdir/$_/$orig" }; diff --git a/lib/PublicInbox/LeiToMail.pm b/lib/PublicInbox/LeiToMail.pm index ca4e92de48b7..d33d27aec006 100644 --- a/lib/PublicInbox/LeiToMail.pm +++ b/lib/PublicInbox/LeiToMail.pm @@ -12,6 +12,7 @@ use PublicInbox::Spawn qw(spawn); use Symbol qw(gensym); use IO::Handle; # ->autoflush use Fcntl qw(SEEK_SET SEEK_END O_CREAT O_EXCL O_WRONLY); +use PublicInbox::Syscall qw(rename_noreplace); my %kw2char = ( # Maildir characters draft => 'D', @@ -262,10 +263,8 @@ sub _buf2maildir ($$$$) { $rand = ''; do { $base = $rand.$common.':2,'.kw2suffix($kw); - } while (!($ok = link($tmp, $dst.$base)) && $!{EEXIST} && - ($rand = _rand.',')); - die "link($tmp, $dst$base): $!" unless $ok; - unlink($tmp) or warn "W: failed to unlink $tmp: $!\n"; + } while (!($ok = rename_noreplace($tmp, $dst.$base)) && + $!{EEXIST} && ($rand = _rand.',')); \$base; } else { my $err = "Error writing $smsg->{blob} to $dst: $!\n"; diff --git a/lib/PublicInbox/Syscall.pm b/lib/PublicInbox/Syscall.pm index 7ab4291119ea..c00385b94db8 100644 --- a/lib/PublicInbox/Syscall.pm +++ b/lib/PublicInbox/Syscall.pm @@ -13,8 +13,9 @@ # License or the Artistic License, as specified in the Perl README file. package PublicInbox::Syscall; use strict; +use v5.10.1; use parent qw(Exporter); -use POSIX qw(ENOSYS O_NONBLOCK); +use POSIX qw(ENOENT EEXIST ENOSYS O_NONBLOCK); use Config; # $VERSION = '0.25'; # Sys::Syscall version @@ -22,7 +23,7 @@ our @EXPORT_OK = qw(epoll_ctl epoll_create epoll_wait EPOLLIN EPOLLOUT EPOLLET EPOLL_CTL_ADD EPOLL_CTL_DEL EPOLL_CTL_MOD EPOLLONESHOT EPOLLEXCLUSIVE - signalfd); + signalfd rename_noreplace); our %EXPORT_TAGS = (epoll => [qw(epoll_ctl epoll_create epoll_wait EPOLLIN EPOLLOUT EPOLL_CTL_ADD EPOLL_CTL_DEL EPOLL_CTL_MOD @@ -64,13 +65,16 @@ our ( $SYS_epoll_ctl, $SYS_epoll_wait, $SYS_signalfd4, + $SYS_renameat2, ); my $SFD_CLOEXEC = 02000000; # Perl does not expose O_CLOEXEC our $no_deprecated = 0; if ($^O eq "linux") { - my $machine = (POSIX::uname())[-1]; + my (undef, undef, $release, undef, $machine) = POSIX::uname(); + my ($maj, $min) = ($release =~ /\A([0-9]+)\.([0-9]+)/); + $SYS_renameat2 = 0 if "$maj.$min" < 3.15; # whether the machine requires 64-bit numbers to be on 8-byte # boundaries. my $u64_mod_8 = 0; @@ -91,22 +95,26 @@ if ($^O eq "linux") { $SYS_epoll_ctl = 255; $SYS_epoll_wait = 256; $SYS_signalfd4 = 327; + $SYS_renameat2 //= 353; } elsif ($machine eq "x86_64") { $SYS_epoll_create = 213; $SYS_epoll_ctl = 233; $SYS_epoll_wait = 232; $SYS_signalfd4 = 289; + $SYS_renameat2 //= 316; } elsif ($machine eq 'x32') { $SYS_epoll_create = 1073742037; $SYS_epoll_ctl = 1073742057; $SYS_epoll_wait = 1073742056; $SYS_signalfd4 = 1073742113; + $SYS_renameat2 //= 0x40000000 + 316; } elsif ($machine eq 'sparc64') { $SYS_epoll_create = 193; $SYS_epoll_ctl = 194; $SYS_epoll_wait = 195; $u64_mod_8 = 1; $SYS_signalfd4 = 317; + $SYS_renameat2 //= 345; $SFD_CLOEXEC = 020000000; } elsif ($machine =~ m/^parisc/) { $SYS_epoll_create = 224; @@ -120,18 +128,21 @@ if ($^O eq "linux") { $SYS_epoll_wait = 238; $u64_mod_8 = 1; $SYS_signalfd4 = 313; + $SYS_renameat2 //= 357; } elsif ($machine eq "ppc") { $SYS_epoll_create = 236; $SYS_epoll_ctl = 237; $SYS_epoll_wait = 238; $u64_mod_8 = 1; $SYS_signalfd4 = 313; + $SYS_renameat2 //= 357; } elsif ($machine =~ m/^s390/) { $SYS_epoll_create = 249; $SYS_epoll_ctl = 250; $SYS_epoll_wait = 251; $u64_mod_8 = 1; $SYS_signalfd4 = 322; + $SYS_renameat2 //= 347; } elsif ($machine eq "ia64") { $SYS_epoll_create = 1243; $SYS_epoll_ctl = 1244; @@ -153,6 +164,7 @@ if ($^O eq "linux") { $u64_mod_8 = 1; $no_deprecated = 1; $SYS_signalfd4 = 74; + $SYS_renameat2 //= 276; } elsif ($machine =~ m/arm(v\d+)?.*l/) { # ARM OABI $SYS_epoll_create = 250; @@ -160,18 +172,21 @@ if ($^O eq "linux") { $SYS_epoll_wait = 252; $u64_mod_8 = 1; $SYS_signalfd4 = 355; + $SYS_renameat2 //= 382; } elsif ($machine =~ m/^mips64/) { $SYS_epoll_create = 5207; $SYS_epoll_ctl = 5208; $SYS_epoll_wait = 5209; $u64_mod_8 = 1; $SYS_signalfd4 = 5283; + $SYS_renameat2 //= 5311; } elsif ($machine =~ m/^mips/) { $SYS_epoll_create = 4248; $SYS_epoll_ctl = 4249; $SYS_epoll_wait = 4250; $u64_mod_8 = 1; $SYS_signalfd4 = 4324; + $SYS_renameat2 //= 4351; } else { # as a last resort, try using the *.ph files which may not # exist or may be wrong @@ -280,6 +295,34 @@ sub signalfd ($$) { } } +sub _rename_noreplace_racy ($$) { + my ($old, $new) = @_; + if (link($old, $new)) { + warn "unlink $old: $!\n" if !unlink($old) && $! != ENOENT; + 1 + } else { + undef; + } +} + +# TODO: support FD args? +sub rename_noreplace ($$) { + my ($old, $new) = @_; + if ($SYS_renameat2) { # RENAME_NOREPLACE = 1, AT_FDCWD = -100 + my $ret = syscall($SYS_renameat2, -100, $old, -100, $new, 1); + if ($ret == 0) { + 1; # like rename() perlop + } elsif ($! == ENOSYS) { + undef $SYS_renameat2; + _rename_noreplace_racy($old, $new); + } else { + undef + } + } else { + _rename_noreplace_racy($old, $new); + } +} + 1; =head1 WARRANTY diff --git a/t/rename_noreplace.t b/t/rename_noreplace.t new file mode 100644 index 000000000000..bd1c4e9236a7 --- /dev/null +++ b/t/rename_noreplace.t @@ -0,0 +1,26 @@ +#!perl -w +# Copyright (C) all contributors <meta@public-inbox.org> +# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt> +use strict; +use v5.10.1; +use PublicInbox::TestCommon; +use_ok 'PublicInbox::Syscall', 'rename_noreplace'; +my ($tmpdir, $for_destroy) = tmpdir; + +open my $fh, '>', "$tmpdir/a" or xbail $!; +my @sa = stat($fh); +is(rename_noreplace("$tmpdir/a", "$tmpdir/b"), 1, 'rename_noreplace'); +my @sb = stat("$tmpdir/b"); +ok(scalar(@sb), 'new file exists'); +ok(!-e "$tmpdir/a", 'original gone'); +is("@sa[0,1]", "@sb[0,1]", 'same st_dev + st_ino'); + +is(rename_noreplace("$tmpdir/a", "$tmpdir/c"), undef, 'undef on ENOENT'); +ok($!{ENOENT}, 'ENOENT set when missing'); + +open $fh, '>', "$tmpdir/a" or xbail $!; +is(rename_noreplace("$tmpdir/a", "$tmpdir/b"), undef, 'undef on EEXIST'); +ok($!{EEXIST}, 'EEXIST set when missing'); +is_deeply([stat("$tmpdir/b")], \@sb, 'target unchanged on EEXIST'); + +done_testing;
Eric Wong <e@80x24.org> wrote: > I got one mysterious test failure here, once, and can't seem > to reproduce it... nearly 2 years later: I've hit it again, but once again cannot reproduce it... > +++ b/t/lei-p2q.t > @@ -7,7 +7,7 @@ require_mods(qw(json DBD::SQLite Search::Xapian)); > > test_lei(sub { > ok(!lei(qw(p2q this-better-cause-format-patch-to-fail)), > - 'p2q fails on bogus arg'); > + 'p2q fails on bogus arg') or diag $lei_err; > like($lei_err, qr/format-patch.*failed/, 'notes format-patch failure'); $? does get printed (32768, so (>> 8) means 128), as it should be. So I wonder if there's a place we drop the socket prematurely or something else is amiss... On a side note, lei's internal IPC could probably be simplified a bit w/o sacrificing parallelism.