From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.2 required=3.0 tests=ALL_TRUSTED,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, T_SCC_BODY_TEXT_LINE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 261141F61D for ; Mon, 22 Aug 2022 02:33:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=80x24.org; s=selector1; t=1661135627; bh=EddkYry5fRF8gCr9JNJj7gmSP3p074O1JuL0D85KWJI=; h=From:To:Subject:Date:In-Reply-To:References:From; b=VyYeiTB//Ux2rc1cdFAopld86TflL7PRovH3dwKrheBTxyckRV/dml7xvz6LpbDjR RA4aBSIJwPKhT5Ax48Gwu0IQoNdlGXsm09VDw79aUVA67s/hxh1epE7kKqS08URI38 HbiEsVaW2A8O4gRgDyN5wcaV5nvqkorMX4DWuVlE= From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 5/7] qspawn: improve error reporting and handling Date: Mon, 22 Aug 2022 02:33:44 +0000 Message-Id: <20220822023346.938859-6-e@80x24.org> In-Reply-To: <20220822023346.938859-1-e@80x24.org> References: <20220822023346.938859-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: First off, avoid potential circular references (via {qx_arg}) by dropping the {-qsp} field from $ctx and SolverGit objects. Instead, we only share a reference to an optional error buffer string {qsp_err}. We'll also attempt to call qspawn.wcb if qx_cb fails, and warn in more places w/o checking for $env since we now rely on warn() instead of $env->{'psgi.errors'}. This makes error handling simpler and safer in future callers. --- lib/PublicInbox/Qspawn.pm | 20 +++++++++++--------- lib/PublicInbox/SolverGit.pm | 28 ++++++++++++---------------- lib/PublicInbox/ViewVCS.pm | 14 ++++++-------- t/qspawn.t | 14 +++++++++++--- 4 files changed, 40 insertions(+), 36 deletions(-) diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm index 79db9e76..cea34fc3 100644 --- a/lib/PublicInbox/Qspawn.pm +++ b/lib/PublicInbox/Qspawn.pm @@ -1,4 +1,4 @@ -# Copyright (C) 2016-2021 all contributors +# Copyright (C) all contributors # License: AGPL-3.0+ # Like most Perl modules in public-inbox, this is internal and @@ -26,6 +26,7 @@ package PublicInbox::Qspawn; use strict; +use v5.10.1; use PublicInbox::Spawn qw(popen_rd); use PublicInbox::GzipFilter; @@ -38,6 +39,7 @@ my $def_limiter; # $cmd is the command to spawn # $cmd_env is the environ for the child process (not PSGI env) # $opt can include redirects and perhaps other process spawning options +# {qsp_err} is an optional error buffer callers may access themselves sub new ($$$;) { my ($class, $cmd, $cmd_env, $opt) = @_; bless { args => [ $cmd, $cmd_env, $opt ] }, $class; @@ -93,18 +95,18 @@ sub finalize ($$) { } if ($err) { - if (defined $self->{err}) { - $self->{err} .= "; $err"; - } else { - $self->{err} = $err; - } - if ($env && $self->{cmd}) { - warn join(' ', @{$self->{cmd}}) . ": $err"; + utf8::decode($err); + if (my $dst = $self->{qsp_err}) { + $$dst .= $$dst ? " $err" : "; $err"; } + warn "@{$self->{cmd}}: $err" if $self->{cmd}; } if ($qx_cb) { eval { $qx_cb->($qx_buf, $qx_arg) }; - } elsif (my $wcb = delete $env->{'qspawn.wcb'}) { + return unless $@; + warn "E: $@"; # hope qspawn.wcb can handle it + } + if (my $wcb = delete $env->{'qspawn.wcb'}) { # have we started writing, yet? require PublicInbox::WwwStatic; $wcb->(PublicInbox::WwwStatic::r(500)); diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm index d3567aa2..dbb9b739 100644 --- a/lib/PublicInbox/SolverGit.pm +++ b/lib/PublicInbox/SolverGit.pm @@ -242,10 +242,8 @@ sub find_smsgs ($$$) { sub update_index_result ($$) { my ($bref, $self) = @_; - my ($qsp, $msg) = delete @$self{qw(-qsp -msg)}; - if (my $err = $qsp->{err}) { - ERR($self, "git update-index error: $err"); - } + my ($qsp_err, $msg) = delete @$self{qw(-qsp_err -msg)}; + ERR($self, "git update-index error:$qsp_err") if $qsp_err; dbg($self, $msg); next_step($self); # onto do_git_apply } @@ -278,7 +276,7 @@ sub prepare_index ($) { my $cmd = [ qw(git update-index -z --index-info) ]; my $qsp = PublicInbox::Qspawn->new($cmd, $self->{git_env}, $rdr); $path_a = git_quote($path_a); - $self->{-qsp} = $qsp; + $qsp->{qsp_err} = \($self->{-qsp_err} = ''); $self->{-msg} = "index prepared:\n$mode_a $oid_full\t$path_a"; $qsp->psgi_qx($self->{psgi_env}, undef, \&update_index_result, $self); } @@ -405,10 +403,8 @@ sub mark_found ($$$) { sub parse_ls_files ($$) { my ($self, $bref) = @_; - my ($qsp, $di) = delete @$self{qw(-qsp -cur_di)}; - if (my $err = $qsp->{err}) { - die "git ls-files error: $err"; - } + my ($qsp_err, $di) = delete @$self{qw(-qsp_err -cur_di)}; + die "git ls-files error:$qsp_err" if $qsp_err; my ($line, @extra) = split(/\0/, $$bref); scalar(@extra) and die "BUG: extra files in index: <", @@ -456,11 +452,11 @@ sub skip_identical ($$$) { sub apply_result ($$) { my ($bref, $self) = @_; - my ($qsp, $di) = delete @$self{qw(-qsp -cur_di)}; + my ($qsp_err, $di) = delete @$self{qw(-qsp_err -cur_di)}; dbg($self, $$bref); my $patches = $self->{patches}; - if (my $err = $qsp->{err}) { - my $msg = "git apply error: $err"; + if ($qsp_err) { + my $msg = "git apply error:$qsp_err"; my $nxt = $patches->[0]; if ($nxt && oids_same_ish($nxt->{oid_b}, $di->{oid_b})) { dbg($self, $msg); @@ -474,9 +470,9 @@ sub apply_result ($$) { } my @cmd = qw(git ls-files -s -z); - $qsp = PublicInbox::Qspawn->new(\@cmd, $self->{git_env}); + my $qsp = PublicInbox::Qspawn->new(\@cmd, $self->{git_env}); $self->{-cur_di} = $di; - $self->{-qsp} = $qsp; + $qsp->{qsp_err} = \($self->{-qsp_err} = ''); $qsp->psgi_qx($self->{psgi_env}, undef, \&ls_files_result, $self); } @@ -508,7 +504,7 @@ sub do_git_apply ($) { my $opt = { 2 => 1, -C => $dn, quiet => 1 }; my $qsp = PublicInbox::Qspawn->new(\@cmd, $self->{git_env}, $opt); $self->{-cur_di} = $di; - $self->{-qsp} = $qsp; + $qsp->{qsp_err} = \($self->{-qsp_err} = ''); $qsp->psgi_qx($self->{psgi_env}, undef, \&apply_result, $self); } @@ -660,7 +656,7 @@ sub new { gits => $ibx->{-repo_objs}, user_cb => $user_cb, uarg => $uarg, - # -cur_di, -qsp, -msg => temporary fields for Qspawn callbacks + # -cur_di, -qsp_err, -msg => temp fields for Qspawn callbacks # TODO: config option for searching related inboxes inboxes => [ $ibx ], diff --git a/lib/PublicInbox/ViewVCS.pm b/lib/PublicInbox/ViewVCS.pm index 3cbc363b..d3fd152d 100644 --- a/lib/PublicInbox/ViewVCS.pm +++ b/lib/PublicInbox/ViewVCS.pm @@ -1,4 +1,4 @@ -# Copyright (C) 2019-2021 all contributors +# Copyright (C) all contributors # License: AGPL-3.0+ # show any VCS object, similar to "git show" @@ -76,10 +76,9 @@ sub stream_large_blob ($$$$) { sub show_other_result ($$) { my ($bref, $ctx) = @_; - my ($qsp, $logref) = delete @$ctx{qw(-qsp -logref)}; - if (my $err = $qsp->{err}) { - utf8::decode($$err); - $$logref .= "git show error: $err"; + my ($qsp_err, $logref) = delete @$ctx{qw(-qsp_err -logref)}; + if ($qsp_err) { + $$logref .= "git show error:$qsp_err"; return html_page($ctx, 500, $logref); } my $l = PublicInbox::Linkify->new; @@ -99,10 +98,9 @@ sub show_other ($$$$) { my $cmd = ['git', "--git-dir=$git->{git_dir}", qw(show --encoding=UTF-8 --no-color --no-abbrev), $oid ]; my $qsp = PublicInbox::Qspawn->new($cmd); - my $env = $ctx->{env}; - $ctx->{-qsp} = $qsp; + $qsp->{qsp_err} = \($ctx->{-qsp_err} = ''); $ctx->{-logref} = $logref; - $qsp->psgi_qx($env, undef, \&show_other_result, $ctx); + $qsp->psgi_qx($ctx->{env}, undef, \&show_other_result, $ctx); } # user_cb for SolverGit, called as: user_cb->($result_or_error, $uarg) diff --git a/t/qspawn.t b/t/qspawn.t index 4b9dc8a5..224e20db 100644 --- a/t/qspawn.t +++ b/t/qspawn.t @@ -1,6 +1,6 @@ -# Copyright (C) 2016-2021 all contributors +# Copyright (C) all contributors # License: AGPL-3.0+ -use strict; +use v5.12; use Test::More; use_ok 'PublicInbox::Qspawn'; @@ -20,12 +20,13 @@ use_ok 'PublicInbox::Qspawn'; sub finish_err ($) { my ($qsp) = @_; $qsp->finish; - $qsp->{err}; + $qsp->{qsp_err} && ${$qsp->{qsp_err}}; } my $limiter = PublicInbox::Qspawn::Limiter->new(1); { my $x = PublicInbox::Qspawn->new([qw(true)]); + $x->{qsp_err} = \(my $err = ''); my $run = 0; $x->start($limiter, sub { my ($self) = @_; @@ -37,7 +38,9 @@ my $limiter = PublicInbox::Qspawn::Limiter->new(1); } { + my @err; local $SIG{__WARN__} = sub { push @err, @_ }; my $x = PublicInbox::Qspawn->new([qw(false)]); + $x->{qsp_err} = \(my $err = ''); my $run = 0; $x->start($limiter, sub { my ($self) = @_; @@ -47,10 +50,13 @@ my $limiter = PublicInbox::Qspawn::Limiter->new(1); $run = 1; }); is($run, 1, 'callback ran alright'); + ok(scalar @err, 'got warning'); } foreach my $cmd ([qw(sleep 1)], [qw(sh -c), 'sleep 1; false']) { + my @err; local $SIG{__WARN__} = sub { push @err, @_ }; my $s = PublicInbox::Qspawn->new($cmd); + $s->{qsp_err} = \(my $err = ''); my @run; $s->start($limiter, sub { my ($self) = @_; @@ -70,8 +76,10 @@ foreach my $cmd ([qw(sleep 1)], [qw(sh -c), 'sleep 1; false']) { if ($cmd->[-1] =~ /false\z/) { ok(finish_err($s), 'got error on false after sleep'); + ok(scalar @err, 'got warning'); } else { ok(!finish_err($s), 'no error on sleep'); + is_deeply([], \@err, 'no warnings'); } ok(!finish_err($_->[0]), "true $_->[1] succeeded") foreach @t; is_deeply([qw(sleep 0 1 2)], \@run, 'ran in order');