* [PATCHv2 3/9] lei: always use async `done' requests to store
2023-10-07 21:24 4% ` [PATCH 3/9] lei: always use async `done' requests to store Eric Wong
2023-10-08 1:58 7% ` Eric Wong
@ 2023-10-08 18:54 4% ` Eric Wong
1 sibling, 0 replies; 4+ results
From: Eric Wong @ 2023-10-08 18:54 UTC (permalink / raw)
To: meta
It's safer against deadlocks and we still get proper error
reporting by passing stderr across in addition to the lei
socket.
---
v2: no change to LeiRemote.pm, increase delay in lei-store-fail.t
MANIFEST | 1 +
lib/PublicInbox/LEI.pm | 9 +++----
lib/PublicInbox/LeiInput.pm | 2 +-
lib/PublicInbox/LeiStore.pm | 17 ++++++------
lib/PublicInbox/LeiXSearch.pm | 6 ++---
t/lei-store-fail.t | 51 +++++++++++++++++++++++++++++++++++
6 files changed, 68 insertions(+), 18 deletions(-)
create mode 100644 t/lei-store-fail.t
diff --git a/MANIFEST b/MANIFEST
index 4693cbe0..689c6bf6 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -514,6 +514,7 @@ t/lei-q-thread.t
t/lei-refresh-mail-sync.t
t/lei-reindex.t
t/lei-sigpipe.t
+t/lei-store-fail.t
t/lei-tag.t
t/lei-up.t
t/lei-watch.t
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 1ba2c2a1..e2b3c0d9 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -1537,12 +1537,11 @@ sub lms {
sub sto_done_request {
my ($lei, $wq) = @_;
- return unless $lei->{sto};
+ return unless $lei->{sto} && $lei->{sto}->{-wq_s1};
local $current_lei = $lei;
- my $sock = $wq ? $wq->{lei_sock} : undef;
- $sock //= $lei->{sock};
- my @io;
- push(@io, $sock) if $sock; # async wait iff possible
+ my $s = ($wq ? $wq->{lei_sock} : undef) // $lei->{sock};
+ my $errfh = $lei->{2} // *STDERR{GLOB};
+ my @io = $s ? ($errfh, $s) : ($errfh);
eval { $lei->{sto}->wq_io_do('done', \@io) };
warn($@) if $@;
}
diff --git a/lib/PublicInbox/LeiInput.pm b/lib/PublicInbox/LeiInput.pm
index 91383265..93f8b6b8 100644
--- a/lib/PublicInbox/LeiInput.pm
+++ b/lib/PublicInbox/LeiInput.pm
@@ -467,7 +467,7 @@ sub process_inputs {
}
# always commit first, even on error partial work is acceptable for
# lei <import|tag|convert>
- my $wait = $self->{lei}->{sto}->wq_do('done') if $self->{lei}->{sto};
+ $self->{lei}->sto_done_request;
$self->{lei}->fail($err) if $err;
}
diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm
index 0cb78f79..e19ec88e 100644
--- a/lib/PublicInbox/LeiStore.pm
+++ b/lib/PublicInbox/LeiStore.pm
@@ -582,19 +582,20 @@ sub xchg_stderr {
}
sub done {
- my ($self, $sock_ref) = @_;
- my $err = '';
+ my ($self) = @_;
+ my ($errfh, $lei_sock) = @$self{0, 1}; # via sto_done_request
+ my @err;
if (my $im = delete($self->{im})) {
eval { $im->done };
- if ($@) {
- $err .= "import done: $@\n";
- warn $err;
- }
+ push(@err, "E: import done: $@\n") if $@;
}
delete $self->{lms};
- $self->{priv_eidx}->done; # V2Writable::done
+ eval { $self->{priv_eidx}->done }; # V2Writable::done
+ push(@err, "E: priv_eidx done: $@\n") if $@;
+ print { $errfh // *STDERR{GLOB} } @err;
+ send($lei_sock, 'child_error 256', 0) if @err && $lei_sock;
xchg_stderr($self);
- die $err if $err;
+ die @err if @err;
}
sub ipc_atfork_child {
diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index 1caa9d06..4077191f 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -358,9 +358,7 @@ sub query_remote_mboxrd {
$fh = IO::Uncompress::Gunzip->new($fh, MultiStream => 1);
PublicInbox::MboxReader->mboxrd($fh, \&each_remote_eml, $self,
$lei, $each_smsg);
- if (delete($self->{-sto_imported})) {
- my $wait = $self->{import_sto}->wq_do('done');
- }
+ $lei->sto_done_request if delete($self->{-sto_imported});
$reap_curl->join;
my $nr = delete $lei->{-nr_remote_eml} // 0;
if ($? == 0) {
@@ -402,7 +400,7 @@ sub query_done { # EOF callback for main daemon
delete $lei->{lxs};
($lei->{opt}->{'mail-sync'} && !$lei->{sto}) and
warn "BUG: {sto} missing with --mail-sync";
- $lei->sto_done_request if $lei->{sto};
+ $lei->sto_done_request;
if (my $v2w = delete $lei->{v2w}) {
my $wait = $v2w->wq_do('done'); # may die
$v2w->wq_close;
diff --git a/t/lei-store-fail.t b/t/lei-store-fail.t
new file mode 100644
index 00000000..fb0f2b75
--- /dev/null
+++ b/t/lei-store-fail.t
@@ -0,0 +1,51 @@
+#!perl -w
+# Copyright (C) all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+# ensure we detect errors in lei/store
+use v5.12;
+use PublicInbox::TestCommon;
+use autodie qw(pipe open close seek);
+use Fcntl qw(SEEK_SET);
+use File::Path qw(remove_tree);
+
+my $start_home = $ENV{HOME}; # bug guard
+test_lei(sub {
+ lei_ok qw(import -q t/plack-qp.eml); # start the store
+ my $opt;
+ pipe($opt->{0}, my $in_w);
+ open $opt->{1}, '+>', undef;
+ open $opt->{2}, '+>', undef;
+ $opt->{-CLOFORK} = [ $in_w ];
+ my $cmd = [ qw(lei import -q -F mboxrd) ];
+ my $tp = start_script($cmd, undef, $opt);
+ close $opt->{0};
+ $in_w->autoflush(1);
+ for (1..500) { # need to fill up 64k read buffer
+ print $in_w <<EOM or xbail "print $!";
+From k\@y Fri Oct 2 00:00:00 1993
+From: <k\@example.com>
+Date: Sat, 02 Oct 2010 00:00:00 +0000
+Subject: hi
+Message-ID: <$_\@t>
+
+will this save?
+EOM
+ }
+ tick 0.2; # XXX ugh, this is so hacky
+
+ # make sto_done_request fail:
+ remove_tree("$ENV{HOME}/.local/share/lei/store");
+ # subsequent lei commands are undefined behavior,
+ # but we need to make sure the current lei command fails:
+
+ close $in_w; # should trigger ->done
+ $tp->join;
+ isnt($?, 0, 'lei import error code set on failure');
+ is(-s $opt->{1}, 0, 'nothing in stdout');
+ isnt(-s $opt->{2}, 0, 'stderr not empty');
+ seek($opt->{2}, 0, SEEK_SET);
+ my @err = readline($opt->{2});
+ ok(grep(!/^#/, @err), 'noted error in stderr') or diag "@err";
+});
+
+done_testing;
^ permalink raw reply related [relevance 4%]
* Re: [PATCH 3/9] lei: always use async `done' requests to store
2023-10-07 21:24 4% ` [PATCH 3/9] lei: always use async `done' requests to store Eric Wong
@ 2023-10-08 1:58 7% ` Eric Wong
2023-10-08 18:54 4% ` [PATCHv2 " Eric Wong
1 sibling, 0 replies; 4+ results
From: Eric Wong @ 2023-10-08 1:58 UTC (permalink / raw)
To: meta
Eric Wong <e@80x24.org> wrote:
> diff --git a/lib/PublicInbox/LeiRemote.pm b/lib/PublicInbox/LeiRemote.pm
> index 54750062..15013baa 100644
> --- a/lib/PublicInbox/LeiRemote.pm
> +++ b/lib/PublicInbox/LeiRemote.pm
> @@ -52,7 +52,7 @@ sub mset {
> $self->{smsg} = [];
> $fh = IO::Uncompress::Gunzip->new($fh, MultiStream => 1);
> PublicInbox::MboxReader->mboxrd($fh, \&_each_mboxrd_eml, $self);
> - my $wait = $self->{lei}->{sto}->wq_do('done');
> + $self->{lei}->sto_done_request;
That's usually not the normal lei/store, but the {tmp_sto} one
from LeiRediff. So it must be synchronous in that case because
we make multiple queries, there.
So I'll revert that hunk.
And ugh, the new lei-store-fail.t test is so nasty; I don't
think a 100ms delay is enough for some systems...
diff --git a/t/lei-store-fail.t b/t/lei-store-fail.t
index e9ad779f..fb0f2b75 100644
--- a/t/lei-store-fail.t
+++ b/t/lei-store-fail.t
@@ -31,7 +31,7 @@ Message-ID: <$_\@t>
will this save?
EOM
}
- tick 0.1; # XXX ugh, this is so hacky
+ tick 0.2; # XXX ugh, this is so hacky
# make sto_done_request fail:
remove_tree("$ENV{HOME}/.local/share/lei/store");
^ permalink raw reply related [relevance 7%]
* [PATCH 0/9] more process-related cleanups
@ 2023-10-07 21:24 5% Eric Wong
2023-10-07 21:24 4% ` [PATCH 3/9] lei: always use async `done' requests to store Eric Wong
0 siblings, 1 reply; 4+ results
From: Eric Wong @ 2023-10-07 21:24 UTC (permalink / raw)
To: meta
2/9 fixes an annoying syslog error I spotted running tests;
3/9 is long overdue, and there's a few more overdue things
coming up...
Eric Wong (9):
xt/httpd-async-stream: avoid waitpid call
lei: do not issue sto->done if socket is inactive
lei: always use async `done' requests to store
ipc: require fork+SOCK_SEQPACKET for wq_* functions
ipc: use autodie for most syscalls
import: use autodie, rely on PerlIO for retries
rename ProcessPipe to ProcessIO
process_io: pass args to awaitpid as list
cindex: start using autodie
MANIFEST | 3 +-
lib/PublicInbox/CodeSearchIdx.pm | 70 ++++++++--------
lib/PublicInbox/Gcf2Client.pm | 4 +-
lib/PublicInbox/Git.pm | 4 +-
lib/PublicInbox/HTTPD/Async.pm | 2 +-
lib/PublicInbox/IPC.pm | 82 ++++++++-----------
lib/PublicInbox/Import.pm | 45 ++++------
lib/PublicInbox/LEI.pm | 11 ++-
lib/PublicInbox/LeiInput.pm | 2 +-
lib/PublicInbox/LeiRediff.pm | 2 +-
lib/PublicInbox/LeiRemote.pm | 2 +-
lib/PublicInbox/LeiStore.pm | 17 ++--
lib/PublicInbox/LeiToMail.pm | 6 +-
lib/PublicInbox/LeiXSearch.pm | 6 +-
.../{ProcessPipe.pm => ProcessIO.pm} | 12 ++-
lib/PublicInbox/Qspawn.pm | 8 +-
lib/PublicInbox/Spamcheck/Spamc.pm | 2 +-
lib/PublicInbox/Spawn.pm | 12 +--
t/ipc.t | 19 ++---
t/lei-store-fail.t | 51 ++++++++++++
t/spawn.t | 12 +--
xt/httpd-async-stream.t | 6 +-
22 files changed, 196 insertions(+), 182 deletions(-)
rename lib/PublicInbox/{ProcessPipe.pm => ProcessIO.pm} (83%)
create mode 100644 t/lei-store-fail.t
^ permalink raw reply [relevance 5%]
* [PATCH 3/9] lei: always use async `done' requests to store
2023-10-07 21:24 5% [PATCH 0/9] more process-related cleanups Eric Wong
@ 2023-10-07 21:24 4% ` Eric Wong
2023-10-08 1:58 7% ` Eric Wong
2023-10-08 18:54 4% ` [PATCHv2 " Eric Wong
0 siblings, 2 replies; 4+ results
From: Eric Wong @ 2023-10-07 21:24 UTC (permalink / raw)
To: meta
It's safer against deadlocks and we still get proper error
reporting by passing stderr across in addition to the lei
socket.
---
MANIFEST | 1 +
lib/PublicInbox/LEI.pm | 9 +++----
lib/PublicInbox/LeiInput.pm | 2 +-
lib/PublicInbox/LeiRemote.pm | 2 +-
lib/PublicInbox/LeiStore.pm | 17 ++++++------
lib/PublicInbox/LeiXSearch.pm | 6 ++---
t/lei-store-fail.t | 51 +++++++++++++++++++++++++++++++++++
7 files changed, 69 insertions(+), 19 deletions(-)
create mode 100644 t/lei-store-fail.t
diff --git a/MANIFEST b/MANIFEST
index 4693cbe0..689c6bf6 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -514,6 +514,7 @@ t/lei-q-thread.t
t/lei-refresh-mail-sync.t
t/lei-reindex.t
t/lei-sigpipe.t
+t/lei-store-fail.t
t/lei-tag.t
t/lei-up.t
t/lei-watch.t
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index f00b2465..4f840e89 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -1537,12 +1537,11 @@ sub lms {
sub sto_done_request {
my ($lei, $wq) = @_;
- return unless $lei->{sto};
+ return unless $lei->{sto} && $lei->{sto}->{-wq_s1};
local $current_lei = $lei;
- my $sock = $wq ? $wq->{lei_sock} : undef;
- $sock //= $lei->{sock};
- my @io;
- push(@io, $sock) if $sock; # async wait iff possible
+ my $s = ($wq ? $wq->{lei_sock} : undef) // $lei->{sock};
+ my $errfh = $lei->{2} // *STDERR{GLOB};
+ my @io = $s ? ($errfh, $s) : ($errfh);
eval { $lei->{sto}->wq_io_do('done', \@io) };
warn($@) if $@;
}
diff --git a/lib/PublicInbox/LeiInput.pm b/lib/PublicInbox/LeiInput.pm
index 91383265..93f8b6b8 100644
--- a/lib/PublicInbox/LeiInput.pm
+++ b/lib/PublicInbox/LeiInput.pm
@@ -467,7 +467,7 @@ sub process_inputs {
}
# always commit first, even on error partial work is acceptable for
# lei <import|tag|convert>
- my $wait = $self->{lei}->{sto}->wq_do('done') if $self->{lei}->{sto};
+ $self->{lei}->sto_done_request;
$self->{lei}->fail($err) if $err;
}
diff --git a/lib/PublicInbox/LeiRemote.pm b/lib/PublicInbox/LeiRemote.pm
index 54750062..15013baa 100644
--- a/lib/PublicInbox/LeiRemote.pm
+++ b/lib/PublicInbox/LeiRemote.pm
@@ -52,7 +52,7 @@ sub mset {
$self->{smsg} = [];
$fh = IO::Uncompress::Gunzip->new($fh, MultiStream => 1);
PublicInbox::MboxReader->mboxrd($fh, \&_each_mboxrd_eml, $self);
- my $wait = $self->{lei}->{sto}->wq_do('done');
+ $self->{lei}->sto_done_request;
$ar->join;
$lei->child_error($?) if $?;
$self; # we are the mset (and $ibx, and $self)
diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm
index 0cb78f79..e19ec88e 100644
--- a/lib/PublicInbox/LeiStore.pm
+++ b/lib/PublicInbox/LeiStore.pm
@@ -582,19 +582,20 @@ sub xchg_stderr {
}
sub done {
- my ($self, $sock_ref) = @_;
- my $err = '';
+ my ($self) = @_;
+ my ($errfh, $lei_sock) = @$self{0, 1}; # via sto_done_request
+ my @err;
if (my $im = delete($self->{im})) {
eval { $im->done };
- if ($@) {
- $err .= "import done: $@\n";
- warn $err;
- }
+ push(@err, "E: import done: $@\n") if $@;
}
delete $self->{lms};
- $self->{priv_eidx}->done; # V2Writable::done
+ eval { $self->{priv_eidx}->done }; # V2Writable::done
+ push(@err, "E: priv_eidx done: $@\n") if $@;
+ print { $errfh // *STDERR{GLOB} } @err;
+ send($lei_sock, 'child_error 256', 0) if @err && $lei_sock;
xchg_stderr($self);
- die $err if $err;
+ die @err if @err;
}
sub ipc_atfork_child {
diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index 1caa9d06..4077191f 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -358,9 +358,7 @@ sub query_remote_mboxrd {
$fh = IO::Uncompress::Gunzip->new($fh, MultiStream => 1);
PublicInbox::MboxReader->mboxrd($fh, \&each_remote_eml, $self,
$lei, $each_smsg);
- if (delete($self->{-sto_imported})) {
- my $wait = $self->{import_sto}->wq_do('done');
- }
+ $lei->sto_done_request if delete($self->{-sto_imported});
$reap_curl->join;
my $nr = delete $lei->{-nr_remote_eml} // 0;
if ($? == 0) {
@@ -402,7 +400,7 @@ sub query_done { # EOF callback for main daemon
delete $lei->{lxs};
($lei->{opt}->{'mail-sync'} && !$lei->{sto}) and
warn "BUG: {sto} missing with --mail-sync";
- $lei->sto_done_request if $lei->{sto};
+ $lei->sto_done_request;
if (my $v2w = delete $lei->{v2w}) {
my $wait = $v2w->wq_do('done'); # may die
$v2w->wq_close;
diff --git a/t/lei-store-fail.t b/t/lei-store-fail.t
new file mode 100644
index 00000000..e9ad779f
--- /dev/null
+++ b/t/lei-store-fail.t
@@ -0,0 +1,51 @@
+#!perl -w
+# Copyright (C) all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+# ensure we detect errors in lei/store
+use v5.12;
+use PublicInbox::TestCommon;
+use autodie qw(pipe open close seek);
+use Fcntl qw(SEEK_SET);
+use File::Path qw(remove_tree);
+
+my $start_home = $ENV{HOME}; # bug guard
+test_lei(sub {
+ lei_ok qw(import -q t/plack-qp.eml); # start the store
+ my $opt;
+ pipe($opt->{0}, my $in_w);
+ open $opt->{1}, '+>', undef;
+ open $opt->{2}, '+>', undef;
+ $opt->{-CLOFORK} = [ $in_w ];
+ my $cmd = [ qw(lei import -q -F mboxrd) ];
+ my $tp = start_script($cmd, undef, $opt);
+ close $opt->{0};
+ $in_w->autoflush(1);
+ for (1..500) { # need to fill up 64k read buffer
+ print $in_w <<EOM or xbail "print $!";
+From k\@y Fri Oct 2 00:00:00 1993
+From: <k\@example.com>
+Date: Sat, 02 Oct 2010 00:00:00 +0000
+Subject: hi
+Message-ID: <$_\@t>
+
+will this save?
+EOM
+ }
+ tick 0.1; # XXX ugh, this is so hacky
+
+ # make sto_done_request fail:
+ remove_tree("$ENV{HOME}/.local/share/lei/store");
+ # subsequent lei commands are undefined behavior,
+ # but we need to make sure the current lei command fails:
+
+ close $in_w; # should trigger ->done
+ $tp->join;
+ isnt($?, 0, 'lei import error code set on failure');
+ is(-s $opt->{1}, 0, 'nothing in stdout');
+ isnt(-s $opt->{2}, 0, 'stderr not empty');
+ seek($opt->{2}, 0, SEEK_SET);
+ my @err = readline($opt->{2});
+ ok(grep(!/^#/, @err), 'noted error in stderr') or diag "@err";
+});
+
+done_testing;
^ permalink raw reply related [relevance 4%]
Results 1-4 of 4 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2023-10-07 21:24 5% [PATCH 0/9] more process-related cleanups Eric Wong
2023-10-07 21:24 4% ` [PATCH 3/9] lei: always use async `done' requests to store Eric Wong
2023-10-08 1:58 7% ` Eric Wong
2023-10-08 18:54 4% ` [PATCHv2 " Eric Wong
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).