From 82c33800809d9e2f72699a03dce7affb7d23aecd Mon Sep 17 00:00:00 2001
From: Eric Wong <e@80x24.org>
Date: Wed, 25 Oct 2023 00:29:41 +0000
Subject: [PATCH] qspawn: simplify internal argument passing

Now that psgi_return is gone, we can further simplify our
internals to support only psgi_qx and psgi_yield.  Internal
argument passing is reduced and we keep the command env and
redirects in the Qspawn object for as long as it's alive.

I wanted to get rid of finalize() entirely, but it seems
trickier to do when having to support generic PSGI.
---
 lib/PublicInbox/Qspawn.pm | 50 +++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm
index 0bb020819..a03e1b019 100644
--- a/lib/PublicInbox/Qspawn.pm
+++ b/lib/PublicInbox/Qspawn.pm
@@ -47,28 +47,27 @@ my $def_limiter;
 # {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;
+	bless { args => [ $cmd, $cmd_env, $opt ? { %$opt } : {} ] }, $class;
 }
 
 sub _do_spawn {
 	my ($self, $start_cb, $limiter) = @_;
-	my ($cmd, $cmd_env, $opt) = @{delete $self->{args}};
+	my ($cmd, $cmd_env, $opt) = @{$self->{args}};
 	my %o = %{$opt || {}};
 	$self->{limiter} = $limiter;
 	for my $k (@PublicInbox::Spawn::RLIMITS) {
-		$o{$k} = $limiter->{$k} // next;
+		$opt->{$k} = $limiter->{$k} // next;
 	}
-	$self->{cmd} = $cmd;
 	$self->{-quiet} = 1 if $o{quiet};
 	$limiter->{running}++;
 	if ($start_cb) {
 		eval { # popen_rd may die on EMFILE, ENFILE
-			$self->{rpipe} = popen_rd($cmd, $cmd_env, \%o,
-						\&waitpid_err, $self, \%o);
+			$self->{rpipe} = popen_rd($cmd, $cmd_env, $opt,
+							\&waitpid_err, $self);
 			$start_cb->($self); # EPOLL_CTL_ADD may ENOSPC/ENOMEM
 		};
 	} else {
-		eval { run_await($cmd, $cmd_env, \%o, \&wait_await, $self) };
+		eval { run_await($cmd, $cmd_env, $opt, \&wait_await, $self) };
 		warn "E: $@" if $@;
 	}
 	finish($self, $@) if $@;
@@ -79,8 +78,8 @@ sub psgi_status_err { # Qspawn itself is useful w/o PSGI
 	PublicInbox::WwwStatic::r($_[0] // 500);
 }
 
-sub finalize ($;$) {
-	my ($self, $opt) = @_;
+sub finalize ($) {
+	my ($self) = @_;
 
 	# process is done, spawn whatever's in the queue
 	my $limiter = delete $self->{limiter} or return;
@@ -96,13 +95,13 @@ sub finalize ($;$) {
 		if (my $dst = $self->{qsp_err}) {
 			$$dst .= $$dst ? " $err" : "; $err";
 		}
-		warn "@{$self->{cmd}}: $err\n" if !$self->{-quiet};
+		warn "E: @{$self->{args}->[0]}: $err\n" if !$self->{-quiet};
 	}
 
 	my ($env, $qx_cb_arg) = delete @$self{qw(psgi_env qx_cb_arg)};
 	if ($qx_cb_arg) {
 		my $cb = shift @$qx_cb_arg;
-		eval { $cb->($opt->{1}, @$qx_cb_arg) };
+		eval { $cb->($self->{args}->[2]->{1}, @$qx_cb_arg) };
 		return unless $@;
 		warn "E: $@"; # hope qspawn.wcb can handle it
 	}
@@ -113,23 +112,21 @@ sub finalize ($;$) {
 	}
 }
 
-sub DESTROY { finalize($_[0]) } # ->finalize is idempotent
-
 sub waitpid_err { # callback for awaitpid
-	my (undef, $self, $opt) = @_; # $_[0]: pid
+	my (undef, $self) = @_; # $_[0]: pid
 	$self->{_err} = ''; # for defined check in ->finish
-	if ($?) { # FIXME: redundant
+	if ($?) { # XXX this may be redundant
 		my $status = $? >> 8;
 		my $sig = $? & 127;
 		$self->{_err} .= "exit status=$status";
 		$self->{_err} .= " signal=$sig" if $sig;
 	}
-	finalize($self, $opt) if !$self->{rpipe};
+	finalize($self) if !$self->{rpipe};
 }
 
 sub wait_await { # run_await cb
 	my ($pid, $cmd, $cmd_env, $opt, $self) = @_;
-	waitpid_err($pid, $self, $opt);
+	waitpid_err($pid, $self);
 }
 
 sub yield_chunk { # $_[-1] is sysread buffer (or undef)
@@ -214,26 +211,24 @@ sub yield_pass {
 
 sub parse_hdr_done ($$) {
 	my ($self) = @_;
-	my $ret;
+	my ($ret, $err);
 	if (defined $_[-1]) {
 		my ($bref, $ph_cb, @ph_arg) = @{$self->{yield_parse_hdr}};
 		$$bref .= $_[-1];
 		$ret = eval { $ph_cb->(length($_[-1]), $bref, @ph_arg) };
-		if ($@) {
-			carp "parse_hdr (@{$self->{cmd}}): $@\n";
+		if (($err = $@)) {
 			$ret = psgi_status_err();
 		} elsif (!$ret && $_[-1] eq '') {
-			carp <<EOM;
-EOF parsing headers from @{$self->{cmd}} ($self->{psgi_env}->{REQUEST_URI})
-EOM
+			$err = 'EOF';
 			$ret = psgi_status_err();
 		}
 	} else {
-		carp <<EOM;
-E: parsing headers: $! from @{$self->{cmd}} ($self->{psgi_env}->{REQUEST_URI})
-EOM
+		$err = "$!";
 		$ret = psgi_status_err();
 	}
+	carp <<EOM if $err;
+E: $err @{$self->{args}->[0]} ($self->{psgi_env}->{REQUEST_URI})
+EOM
 	$ret; # undef if headers incomplete
 }
 
@@ -301,4 +296,7 @@ sub psgi_yield {
 	}
 }
 
+no warnings 'once';
+*DESTROY = \&finalize; # ->finalize is idempotent
+
 1;
-- 
2.47.2