[PATCH 0/4] httpd/async fixes for coderepos

[PATCH 0/4] httpd/async fixes for coderepos

[Eric Wong]

1/4 is a critical fix.  The rest are minor quality-of-life
things I noticed along the way.

Eric Wong (4):
  httpd: avoid crash on cgit -> coderepo 404 fallback
  httpd/async: remove useless undef
  qspawn: shorten life of {hdr_buf} in generic code path
  httpd/async + qspawn: rename {fh} fields

 lib/PublicInbox/GitHTTPBackend.pm |  6 ++++--
 lib/PublicInbox/HTTPD/Async.pm    | 23 +++++++++++------------
 lib/PublicInbox/Qspawn.pm         |  8 ++++----
 3 files changed, 19 insertions(+), 18 deletions(-)

[PATCH 1/4] httpd: avoid crash on cgit -> coderepo 404 fallback

[Eric Wong]

A trickled cgit response can cause HTTPD::Async->event_step to
fire an extra time after header parsing.  We need to account for
the lack of async_pass call populating ->{fh} and ->{http} in
that case and avoid calling $self->{fh}->write when there's
no {fh}.
---
 lib/PublicInbox/GitHTTPBackend.pm | 6 ++++--
 lib/PublicInbox/HTTPD/Async.pm    | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/GitHTTPBackend.pm b/lib/PublicInbox/GitHTTPBackend.pm
index 1eb51f27..974a1831 100644
--- a/lib/PublicInbox/GitHTTPBackend.pm
+++ b/lib/PublicInbox/GitHTTPBackend.pm
@@ -128,7 +128,7 @@ sub input_prepare {
 	{ 0 => $in };
 }
 
-sub parse_cgi_headers {
+sub parse_cgi_headers { # {parse_hdr} for Qspawn
 	my ($r, $bref, $ctx) = @_;
 	return r(500) unless defined $r && $r >= 0;
 	$$bref =~ s/\A(.*?)\r?\n\r?\n//s or return $r == 0 ? r(500) : undef;
@@ -145,7 +145,9 @@ sub parse_cgi_headers {
 	}
 
 	# fallback to WwwCoderepo if cgit 404s.  Duplicating $ctx prevents
-	# ->finalize from the current Qspawn from using qspawn.wcb
+	# ->finalize from the current Qspawn from using qspawn.wcb.
+	# This makes qspawn skip ->async_pass and causes
+	# PublicInbox::HTTPD::Async::event_step to close shortly after
 	if ($code == 404 && $ctx->{www} && !$ctx->{_coderepo_tried}++) {
 		my %ctx = %$ctx;
 		$ctx{env} = +{ %{$ctx->{env}} };
diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm
index cb76cfab..9e592f47 100644
--- a/lib/PublicInbox/HTTPD/Async.pm
+++ b/lib/PublicInbox/HTTPD/Async.pm
@@ -48,7 +48,8 @@ sub event_step {
 		# this may call async_pass when headers are done
 		$cb->(my $refcnt_guard = delete $self->{arg});
 	} elsif (my $sock = $self->{sock}) {
-		my $http = $self->{http};
+		# $http may be undef if discarding body output from cgit on 404
+		my $http = $self->{http} or return $self->close;
 		# $self->{sock} is a read pipe for git-http-backend or cgit
 		# and 65536 is the default Linux pipe size
 		my $r = sysread($sock, my $buf, 65536);

[PATCH 2/4] httpd/async: remove useless undef

[Eric Wong]

Assigning `undef' to a scalar doesn't free it's memory,
we need to call `undef($var)' in the caller.  It's also
been pointless since we simplified ->async_pass in commit
b7fbffd1f8c12556 (httpd/async: get rid of ephemeral main_cb, 2019-12-25)
---
 lib/PublicInbox/HTTPD/Async.pm | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm
index 9e592f47..75b3bd50 100644
--- a/lib/PublicInbox/HTTPD/Async.pm
+++ b/lib/PublicInbox/HTTPD/Async.pm
@@ -84,10 +84,6 @@ sub async_pass {
 	# *_wcb methods respond to ->write (and ->close), not ->print
 	$fh->write($$bref);
 
-	# we're done with this, free this memory up ASAP since the
-	# calls after this may use much memory:
-	$$bref = undef;
-
 	$self->{http} = $http;
 	$self->{fh} = $fh;
 }

[PATCH 3/4] qspawn: shorten life of {hdr_buf} in generic code path

[Eric Wong]

No point in keeping the old buffer around if we don't need to.
---
 lib/PublicInbox/Qspawn.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm
index ef9db43e..da7cd74f 100644
--- a/lib/PublicInbox/Qspawn.pm
+++ b/lib/PublicInbox/Qspawn.pm
@@ -247,9 +247,9 @@ sub psgi_return_init_cb {
 					delete($self->{hdr_buf}));
 	} else { # for synchronous PSGI servers
 		require PublicInbox::GetlineBody;
+		my $buf = delete $self->{hdr_buf};
 		$r->[2] = PublicInbox::GetlineBody->new($self->{rpipe},
-					\&event_step, $self,
-					${$self->{hdr_buf}}, $filter);
+					\&event_step, $self, $$buf, $filter);
 		$wcb->($r);
 	}
 }

[PATCH 4/4] httpd/async + qspawn: rename {fh} fields

[Eric Wong]

Use more unique names within the project to minimize confusion
since these packages interact quite a bit and using identical
names leads to needless confusion.
---
 lib/PublicInbox/HTTPD/Async.pm | 16 +++++++++-------
 lib/PublicInbox/Qspawn.pm      |  4 ++--
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm
index 75b3bd50..e03daafa 100644
--- a/lib/PublicInbox/HTTPD/Async.pm
+++ b/lib/PublicInbox/HTTPD/Async.pm
@@ -37,7 +37,7 @@ sub new {
 		arg => $arg, # arg for $cb
 		end_obj => $end_obj, # like END{}, can ->event_step
 	}, $class;
-	my $pp = tied *$io;
+	my $pp = tied *$io; # ProcessPipe
 	$pp->{fh}->blocking(0) // die "$io->blocking(0): $!";
 	$self->SUPER::new($io, EPOLLIN);
 }
@@ -54,7 +54,7 @@ sub event_step {
 		# and 65536 is the default Linux pipe size
 		my $r = sysread($sock, my $buf, 65536);
 		if ($r) {
-			$self->{fh}->write($buf); # may call $http->close
+			$self->{ofh}->write($buf); # may call $http->close
 			# let other clients get some work done, too
 			return if $http->{sock}; # !closed
 
@@ -63,7 +63,7 @@ sub event_step {
 			return; # EPOLLIN means we'll be notified
 		}
 
-		# Done! Error handling will happen in $self->{fh}->close
+		# Done! Error handling will happen in $self->{ofh}->close
 		# called by end_obj->event_step handler
 		delete $http->{forward};
 		$self->close; # queues end_obj->event_step to be called
@@ -71,9 +71,11 @@ sub event_step {
 }
 
 # once this is called, all data we read is passed to the
-# to the PublicInbox::HTTP instance ($http) via $fh->write
+# to the PublicInbox::HTTP instance ($http) via $ofh->write
+# $ofh is typically PublicInbox::HTTP::{Chunked,Identity}, but
+# may be PublicInbox::GzipFilter or $PublicInbox::Qspawn::qx_fh
 sub async_pass {
-	my ($self, $http, $fh, $bref) = @_;
+	my ($self, $http, $ofh, $bref) = @_;
 	# In case the client HTTP connection ($http) dies, it
 	# will automatically close this ($self) object.
 	$http->{forward} = $self;
@@ -82,10 +84,10 @@ sub async_pass {
 	# This is typically PublicInbox:HTTP::{chunked,identity}_wcb,
 	# but may be PublicInbox::GzipFilter::write.  PSGI requires
 	# *_wcb methods respond to ->write (and ->close), not ->print
-	$fh->write($$bref);
+	$ofh->write($$bref);
 
 	$self->{http} = $http;
-	$self->{fh} = $fh;
+	$self->{ofh} = $ofh;
 }
 
 # may be called as $forward->close in PublicInbox::HTTP or EOF (event_step)
diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm
index da7cd74f..6e245389 100644
--- a/lib/PublicInbox/Qspawn.pm
+++ b/lib/PublicInbox/Qspawn.pm
@@ -187,7 +187,7 @@ sub event_step {
 	my ($self, $err) = @_; # $err: $!
 	warn "psgi_{return,qx} $err" if defined($err);
 	finish($self);
-	my ($fh, $qx_fh) = delete(@$self{qw(fh qx_fh)});
+	my ($fh, $qx_fh) = delete(@$self{qw(qfh qx_fh)});
 	$fh->close if $fh; # async-only (psgi_return)
 }
 
@@ -242,7 +242,7 @@ sub psgi_return_init_cb {
 		# done reading headers, handoff to read body
 		my $fh = $wcb->($r); # scalar @$r == 2
 		$fh = $filter->attach($fh) if $filter;
-		$self->{fh} = $fh;
+		$self->{qfh} = $fh;
 		$async->async_pass($env->{'psgix.io'}, $fh,
 					delete($self->{hdr_buf}));
 	} else { # for synchronous PSGI servers