From: Eric Wong Date: Wed, 26 Mar 2025 03:35:02 +0000 (+0000) Subject: search: improve xap_helper mset error reporting X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=01673fc91dc67bf364c02ca7e9dd059716798e1d;p=thirdparty%2Fpublic-inbox.git search: improve xap_helper mset error reporting For user errors generating bad queries, dumping exception messages from Xapian::QueryParser can be helpful. So unconditionally pass the stderr FD as a regular file to xap_helper processes for both lei and WWW search users. --- diff --git a/lib/PublicInbox/Search.pm b/lib/PublicInbox/Search.pm index 4fa028858..389a95012 100644 --- a/lib/PublicInbox/Search.pm +++ b/lib/PublicInbox/Search.pm @@ -10,6 +10,7 @@ use parent qw(Exporter); our @EXPORT_OK = qw(retry_reopen int_val get_pct xap_terms); use List::Util qw(max); use POSIX qw(strftime); +use autodie qw(open pipe); use Carp (); our $XHC = 0; # defined but false @@ -486,8 +487,13 @@ sub async_mset { xdb($self); # populate {nshards} my @margs = ($self->xh_args, xh_opt($self, $opt), '--'); my $ret = eval { - my $rd = $XHC->mkreq(undef, 'mset', @margs, $qry_str); - PublicInbox::XhcMset->maybe_new($rd, $self, $cb, @args); + pipe my $out_rd, my $out_wr; + open my $err_rw, '+>', undef; + $XHC->mkreq([ $out_wr, $err_rw ], + 'mset', @margs, $qry_str); + undef $out_wr; + PublicInbox::XhcMset->maybe_new($out_rd, $err_rw, + $self, $cb, @args); }; $cb->(@args, undef, $@) if $@; $ret; diff --git a/lib/PublicInbox/SearchView.pm b/lib/PublicInbox/SearchView.pm index f5b10663a..90a319ced 100644 --- a/lib/PublicInbox/SearchView.pm +++ b/lib/PublicInbox/SearchView.pm @@ -164,7 +164,7 @@ sub mset_summary { sub err_txt { my ($ctx, $err) = @_; my $u = $ctx->{ibx}->base_url($ctx->{env}) . '_/text/help/'; - $err =~ s/^\s*Exception:\s*//; # bad word to show users :P + $err =~ s/\s*\bException:\s*/ /; # bad word to show users :P sanitize_local_paths $err; $err = ascii_html($err); "\nBad query: $err\n" . diff --git a/lib/PublicInbox/XapClient.pm b/lib/PublicInbox/XapClient.pm index b54e9be56..8b87b0431 100644 --- a/lib/PublicInbox/XapClient.pm +++ b/lib/PublicInbox/XapClient.pm @@ -16,14 +16,11 @@ our $tries = -1; # set to zero by read-only daemon sub mkreq { my ($self, $ios, @arg) = @_; - my ($r, $n); - pipe($r, $ios->[0]) if !defined($ios->[0]); my @fds = map fileno($_), @$ios; my $buf = join("\0", @arg, ''); - $n = $PublicInbox::IPC::send_cmd->($self->{io}, \@fds, $buf, 0, $tries) - // die "send_cmd: $!"; + my $n = $PublicInbox::IPC::send_cmd->($self->{io}, + \@fds, $buf, 0, $tries) // die "send_cmd: $!"; $n == length($buf) or die "send_cmd: $n != ".length($buf); - $r; } sub start_helper (@) { diff --git a/lib/PublicInbox/XapHelper.pm b/lib/PublicInbox/XapHelper.pm index 54e2c174b..42f80de43 100644 --- a/lib/PublicInbox/XapHelper.pm +++ b/lib/PublicInbox/XapHelper.pm @@ -197,7 +197,7 @@ sub srch_init_extra ($$) { } } -sub dispatch { +sub dispatch (@) { my ($req, $cmd, @argv) = @_; my $fn = $req->can("cmd_$cmd") or return; $GLP->getoptionsfromarray(\@argv, $req, @PublicInbox::Search::XH_SPEC) @@ -269,11 +269,15 @@ sub recv_loop { my $req = bless {}, __PACKAGE__; my $i = 0; open($req->{$i++}, '+<&=', $_) for @fds; + $req->{1}->autoflush(1) if $req->{1}; local $stderr = $req->{1} // \*STDERR; die "not NUL-terminated" if chop($rbuf) ne "\0"; - my @argv = split(/\0/, $rbuf); + my ($cmd, @argv) = split(/\0/, $rbuf); $req->{nr_out} = 0; - $req->dispatch(@argv) if @argv; + if (defined $cmd) { + eval { dispatch $req, $cmd, @argv }; + warn "$cmd: $@" if $@; + } } } diff --git a/lib/PublicInbox/XhcMset.pm b/lib/PublicInbox/XhcMset.pm index ac25eece4..d6dc877f9 100644 --- a/lib/PublicInbox/XhcMset.pm +++ b/lib/PublicInbox/XhcMset.pm @@ -5,25 +5,41 @@ package PublicInbox::XhcMset; use v5.12; use parent qw(PublicInbox::DS); +use autodie qw(seek); +use PublicInbox::IO qw(read_all); use PublicInbox::XhcMsetIterator; use PublicInbox::Syscall qw(EPOLLIN EPOLLONESHOT); +use Fcntl qw(SEEK_SET); +use Carp qw(croak); +use PublicInbox::Git qw(git_quote); + +sub die_err ($@) { + my ($self, @msg) = @_; + my $err_rw = delete $self->{err_rw}; + seek $err_rw, 0, SEEK_SET; + my $s = read_all $err_rw; + chomp $s; + croak $s, @msg; +} sub event_step { my ($self) = @_; my ($cb, @args) = @{delete $self->{cb_args} // return}; my $rd = $self->{sock}; eval { - my $hdr = <$rd> // die "E: reading mset header: $!"; + my $hdr = <$rd> // + die_err $self, $! ? ("reading mset header: $!") : (); for (split /\s+/, $hdr) { # read mset.size + estimated_matches my ($k, $v) = split /=/, $_, 2; $k =~ s/\A[^\.]*\.//; # s/(mset)?\./ $self->{$k} = $v; } - my $size = $self->{size} // die "E: bad xhc header: `$hdr'"; + my $size = $self->{size} // + die_err $self, 'bad xhc header: ', git_quote($hdr); my @it = map { PublicInbox::XhcMsetIterator::make($_) } <$rd>; $self->{items} = \@it; - scalar(@it) == $size or die - 'E: got ',scalar(@it),", expected mset.size=$size"; + scalar(@it) == $size or die_err $self, + 'got ', scalar(@it), ', expected mset.size=', $size; }; my $err = $@; $self->close; @@ -32,12 +48,16 @@ sub event_step { } sub maybe_new { - my (undef, $rd, $srch, @cb_args) = @_; - my $self = bless { cb_args => \@cb_args, srch => $srch }, __PACKAGE__; + my (undef, $out_rd, $err_rw, $srch, @cb_args) = @_; + my $self = bless { + cb_args => \@cb_args, + srch => $srch, + err_rw => $err_rw, + }, __PACKAGE__; if ($PublicInbox::DS::in_loop) { # async - $self->SUPER::new($rd, EPOLLIN|EPOLLONESHOT); + $self->SUPER::new($out_rd, EPOLLIN|EPOLLONESHOT); } else { # synchronous - $self->{sock} = $rd; + $self->{sock} = $out_rd; event_step($self); undef; } diff --git a/lib/PublicInbox/xh_mset.h b/lib/PublicInbox/xh_mset.h index 6fdecc397..86996ca5c 100644 --- a/lib/PublicInbox/xh_mset.h +++ b/lib/PublicInbox/xh_mset.h @@ -14,7 +14,6 @@ static bool cmd_mset(struct req *req) { if (optind >= req->argc) ABORT("usage: mset [OPTIONS] WANT QRY_STR"); - if (req->fp[1]) ABORT("mset only accepts 1 FD"); const char *qry_str = req->argv[optind]; CLEANUP_FBUF struct fbuf wbuf = {}; Xapian::MSet mset = req->code_search ? commit_mset(req, qry_str) : diff --git a/t/cindex.t b/t/cindex.t index 0ae0b2b4a..2743aaa5c 100644 --- a/t/cindex.t +++ b/t/cindex.t @@ -5,7 +5,7 @@ use v5.12; use PublicInbox::TestCommon; use Cwd qw(getcwd); use List::Util qw(sum); -use autodie qw(close mkdir open rename); +use autodie qw(close mkdir open pipe rename); require_mods(qw(json Xapian +SCM_RIGHTS DBD::SQLite)); use_ok 'PublicInbox::CodeSearchIdx'; use PublicInbox::Import; @@ -149,8 +149,10 @@ my $test_xhc = sub { my ($xhc) = @_; my $csrch = PublicInbox::CodeSearch->new("$tmp/ext"); my $impl = $xhc->{impl}; - my ($r, @l); - $r = $xhc->mkreq([], qw(mset -c -g), $zp_git, @xh_args, 'NUL'); + my ($r, $w, @l); + pipe $r, $w; + $xhc->mkreq([$w], qw(mset -c -g), $zp_git, @xh_args, 'NUL'); + close $w; chomp(@l = <$r>); like shift(@l), qr/\bmset\.size=2\b/, "got expected header $impl"; my %docid2data; @@ -164,7 +166,9 @@ my $test_xhc = sub { } @l; is_deeply(\@got, $exp, "expected doc_data $impl"); - $r = $xhc->mkreq([], qw(mset -c -g), "$tmp/wt0/.git", @xh_args, 'NUL'); + pipe $r, $w; + $xhc->mkreq([$w], qw(mset -c -g), "$tmp/wt0/.git", @xh_args, 'NUL'); + close $w; chomp(@l = <$r>); like shift(@l), qr/\bmset.size=0\b/, "got miss in wrong dir $impl"; is_deeply(\@l, [], "no extra lines $impl"); diff --git a/t/xap_helper.t b/t/xap_helper.t index 0429e3719..d79326786 100644 --- a/t/xap_helper.t +++ b/t/xap_helper.t @@ -222,20 +222,25 @@ for my $n (@NO_CXX) { # git patch-id --stable mkreq([ undef, $err_w ], qw(dump_ibx -A XDFID -A Q), + + pipe my $r, my $w; + $xhc->mkreq([ $w, $err_w ], qw(dump_ibx -A XDFID -A Q), (map { ('-d', $_) } @ibx_idx), 9, "mid:$mid"); close $err_w; + close $w; my $res = do { local $/; <$r> }; is($res, "$dfid 9\n$mid 9\n", "got expected result ($xhc->{impl})"); my $err = do { local $/; <$err_r> }; is($err, "mset.size=1 nr_out=2\n", "got expected status ($xhc->{impl})"); pipe($err_r, $err_w); - $r = $xhc->mkreq([ undef, $err_w ], qw(dump_roots -c -A XDFID), + pipe $r, $w; + $xhc->mkreq([ $w, $err_w ], qw(dump_roots -c -A XDFID), (map { ('-d', $_) } @int), $root2id_file, 'dt:19700101'.'000000..'); close $err_w; + close $w; my @res = <$r>; is(scalar(@res), 5, 'got expected rows'); is(scalar(@res), scalar(grep(/\A[0-9a-f]{40,} [0-9]+\n\z/, @res)), @@ -243,8 +248,10 @@ for my $n (@NO_CXX) { $err = do { local $/; <$err_r> }; is $err, "mset.size=6 nr_out=5\n", "got expected status ($xhc->{impl})"; - $r = $xhc->mkreq([], qw(mset), @ibx_shard_args, + pipe $r, $w; + $xhc->mkreq([$w], qw(mset), @ibx_shard_args, 'dfn:lib/PublicInbox/Search.pm'); + close $w; chomp((my $hdr, @res) = readline($r)); like $hdr, qr/\bmset\.size=1\b/, "got expected header via mset ($xhc->{impl}"; @@ -259,8 +266,10 @@ for my $n (@NO_CXX) { ok $res[1] > 0 && $res[1] <= 100, 'pct > 0 && <= 100'; is scalar(@res), 3, 'only 3 columns in result'; - $r = $xhc->mkreq([], qw(mset), @ibx_shard_args, + pipe $r, $w; + $xhc->mkreq([$w], qw(mset), @ibx_shard_args, 'dt:19700101'.'000000..'); + close $w; chomp(($hdr, @res) = readline($r)); like $hdr, qr/\bmset\.size=6\b/, "got expected header via multi-result mset ($xhc->{impl}"; @@ -276,10 +285,12 @@ for my $n (@NO_CXX) { my $nr; for my $i (7, 8, 39, 40) { pipe($err_r, $err_w); - $r = $xhc->mkreq([ undef, $err_w ], qw(dump_roots -c -A), + pipe($r, $w); + $xhc->mkreq([ $w, $err_w ], qw(dump_roots -c -A), "XDFPOST$i", (map { ('-d', $_) } @int), $root2id_file, 'dt:19700101'.'000000..'); close $err_w; + close $w; @res = <$r>; my @err = <$err_r>; if (defined $nr) { @@ -297,9 +308,11 @@ for my $n (@NO_CXX) { or diag explain(\@res, \@err); } pipe($err_r, $err_w); - $r = $xhc->mkreq([ undef, $err_w ], qw(dump_ibx -A XDFPOST7), + pipe $r, $w; + $xhc->mkreq([ $w, $err_w ], qw(dump_ibx -A XDFPOST7), @ibx_shard_args, qw(13 rt:0..)); close $err_w; + close $w; @res = <$r>; my @err = <$err_r>; my ($nr_out) = ("@err" =~ /nr_out=(\d+)/); @@ -318,8 +331,12 @@ for my $n (@NO_CXX) { my $capture = sub { ($mset, $err) = @_ }; my $retrieve = sub { my ($qstr) = @_; - $r = $xhc->mkreq(undef, 'mset', @thr_shard_args, $qstr); - PublicInbox::XhcMset->maybe_new($r, undef, $capture); + pipe $r, $w; + $xhc->mkreq([ $w ], 'mset', @thr_shard_args, $qstr); + close $w; + open my $err_rw, '+>', undef; + PublicInbox::XhcMset->maybe_new($r, $err_rw, + undef, $capture); map { $over->get_art($_->get_docid) } $mset->items; }; @art = $retrieve->('thread:thread-root@example wildfires'); @@ -365,8 +382,10 @@ for my $n (@NO_CXX) { diag 'testing timeouts...'; for my $j (qw(0 1)) { my $t0 = now; - $r = $xhc->mkreq(undef, qw(test_sleep -K 1 -d), + pipe $r, $w; + $xhc->mkreq([ $w ], qw(test_sleep -K 1 -d), $ibx_idx[0]); + close $w; is readline($r), undef, 'got EOF'; my $diff = now - $t0; ok $diff < 3, "timeout didn't take too long -j$j"; @@ -387,8 +406,10 @@ SKIP: { my $exp; for (0..(PublicInbox::Search::ulimit_n() * $nr)) { for my $xhc (@xhc) { - my $r = $xhc->mkreq([], qw(mset -Q), "tst$n=XTST$n", + pipe my $r, my $w; + $xhc->mkreq([$w], qw(mset -Q), "tst$n=XTST$n", @ibx_shard_args, qw(rt:0..)); + close $w; chomp(my @res = readline($r)); $exp //= $res[0]; $exp eq $res[0] or