[PATCH 5/7] qspawn: improve error reporting and handling

[Eric Wong <e@80x24.org>]

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 <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 
 # 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 <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 
 # 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 <meta@public-inbox.org>
+# 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.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');