[PATCH 0/3] start removing Plack::Request from WWW

[PATCH 0/3] start removing Plack::Request from WWW

[Eric Wong]

It's unnecessary overhead and parsing request bodies for
forms should not be done for our application.

In other words, workarounds like commit 311c2adc8c63
("git-http-backend: avoid Plack::Request parsing body")
should not be necessary.

Eric Wong (3):
      git-http-backend: remove dependency on Plack::Request
      www: remove gratuitous use of Plack::Request methods
      www: remove a few more Plack::Request dependencies

 lib/PublicInbox/Feed.pm           | 18 +++++++++---------
 lib/PublicInbox/GitHTTPBackend.pm | 26 +++++++++++++-------------
 lib/PublicInbox/SearchView.pm     | 14 +++++++-------
 lib/PublicInbox/WWW.pm            | 28 ++++++++++++++++++----------
 t/git-http-backend.psgi           |  6 ++----
 t/git-http-backend.t              |  2 +-
 6 files changed, 50 insertions(+), 44 deletions(-)

[PATCH 1/3] git-http-backend: remove dependency on Plack::Request

[Eric Wong]

Plack::Request is unnecessary overhead for this given the
strictness of git-http-backend.  Furthermore, having to make
commit 311c2adc8c63 ("avoid Plack::Request parsing body")
to avoid tempfiles should not have been necessary.
---
 lib/PublicInbox/GitHTTPBackend.pm | 26 +++++++++++++-------------
 lib/PublicInbox/WWW.pm            |  8 ++++----
 t/git-http-backend.psgi           |  6 ++----
 t/git-http-backend.t              |  2 +-
 4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/lib/PublicInbox/GitHTTPBackend.pm b/lib/PublicInbox/GitHTTPBackend.pm
index 58e75cd..9660d21 100644
--- a/lib/PublicInbox/GitHTTPBackend.pm
+++ b/lib/PublicInbox/GitHTTPBackend.pm
@@ -40,15 +40,17 @@ sub r ($;$) {
 }
 
 sub serve {
-	my ($cgi, $git, $path) = @_;
+	my ($env, $git, $path) = @_;
 
-	my $service = $cgi->query_parameters->get('service') || '';
-	if ($service =~ /\Agit-\w+-pack\z/ || $path =~ /\Agit-\w+-pack\z/) {
-		my $ok = serve_smart($cgi, $git, $path);
+	# Documentation/technical/http-protocol.txt in git.git
+	# requires one and exactly one query parameter:
+	if ($env->{QUERY_STRING} =~ /\Aservice=git-\w+-pack\z/ ||
+				$path =~ /\Agit-\w+-pack\z/) {
+		my $ok = serve_smart($env, $git, $path);
 		return $ok if $ok;
 	}
 
-	serve_dumb($cgi, $git, $path);
+	serve_dumb($env, $git, $path);
 }
 
 sub err ($@) {
@@ -63,7 +65,7 @@ sub drop_client ($) {
 }
 
 sub serve_dumb {
-	my ($cgi, $git, $path) = @_;
+	my ($env, $git, $path) = @_;
 
 	my @h;
 	my $type;
@@ -82,7 +84,6 @@ sub serve_dumb {
 	return r(404) unless -f $f && -r _; # just in case it's a FIFO :P
 	my @st = stat(_);
 	my $size = $st[7];
-	my $env = $cgi->{env};
 
 	# TODO: If-Modified-Since and Last-Modified?
 	open my $in, '<', $f or return r(404);
@@ -90,7 +91,7 @@ sub serve_dumb {
 	my $code = 200;
 	push @h, 'Content-Type', $type;
 	if (($env->{HTTP_RANGE} || '') =~ /\bbytes=(\d*)-(\d*)\z/) {
-		($code, $len) = prepare_range($cgi, $in, \@h, $1, $2, $size);
+		($code, $len) = prepare_range($env, $in, \@h, $1, $2, $size);
 		if ($code == 416) {
 			push @h, 'Content-Range', "bytes */$size";
 			return [ 416, \@h, [] ];
@@ -118,7 +119,7 @@ sub serve_dumb {
 }
 
 sub prepare_range {
-	my ($cgi, $in, $h, $beg, $end, $size) = @_;
+	my ($env, $in, $h, $beg, $end, $size) = @_;
 	my $code = 200;
 	my $len = $size;
 	if ($beg eq '') {
@@ -152,7 +153,7 @@ sub prepare_range {
 			push @$h, "bytes $beg-$end/$size";
 
 			# FIXME: Plack::Middleware::Deflater bug?
-			$cgi->{env}->{'psgix.no-compress'} = 1;
+			$env->{'psgix.no-compress'} = 1;
 		}
 	}
 	($code, $len);
@@ -160,8 +161,7 @@ sub prepare_range {
 
 # returns undef if 403 so it falls back to dumb HTTP
 sub serve_smart {
-	my ($cgi, $git, $path) = @_;
-	my $env = $cgi->{env};
+	my ($env, $git, $path) = @_;
 	my $in = $env->{'psgi.input'};
 	my $fd = eval { fileno($in) };
 	unless (defined $fd && $fd >= 0) {
@@ -201,7 +201,7 @@ sub serve_smart {
 		return if !defined($r) && ($!{EINTR} || $!{EAGAIN});
 		return r(500, 'http-backend error') unless $r;
 		$r = parse_cgi_headers(\$buf) or return; # incomplete headers
-		$r->[0] == 403 ? serve_dumb($cgi, $git, $path) : $r;
+		$r->[0] == 403 ? serve_dumb($env, $git, $path) : $r;
 	};
 	my $res;
 	my $async = $env->{'pi-httpd.async'};
diff --git a/lib/PublicInbox/WWW.pm b/lib/PublicInbox/WWW.pm
index cf370af..88d4f6f 100644
--- a/lib/PublicInbox/WWW.pm
+++ b/lib/PublicInbox/WWW.pm
@@ -48,7 +48,7 @@ sub call {
 		 $path_info =~ m!$INBOX_RE/(git-upload-pack)\z!) {
 		my $path = $2;
 		return (invalid_inbox($self, $ctx, $1) ||
-			serve_git($cgi, $ctx->{git}, $path));
+			serve_git($env, $ctx->{git}, $path));
 	}
 	elsif ($method !~ /\AGET|HEAD\z/) {
 		return r(405, 'Method Not Allowed');
@@ -68,7 +68,7 @@ sub call {
 				($PublicInbox::GitHTTPBackend::ANY)\z!ox) {
 		my $path = $2;
 		invalid_inbox($self, $ctx, $1) ||
-			serve_git($cgi, $ctx->{git}, $path);
+			serve_git($env, $ctx->{git}, $path);
 	} elsif ($path_info =~ m!$INBOX_RE/([\w-]+).mbox\.gz\z!o) {
 		serve_mbox_range($self, $ctx, $1, $2);
 	} elsif ($path_info =~ m!$INBOX_RE/$MID_RE/$END_RE\z!o) {
@@ -424,8 +424,8 @@ sub msg_page {
 }
 
 sub serve_git {
-	my ($cgi, $git, $path) = @_;
-	PublicInbox::GitHTTPBackend::serve($cgi, $git, $path);
+	my ($env, $git, $path) = @_;
+	PublicInbox::GitHTTPBackend::serve($env, $git, $path);
 }
 
 sub serve_mbox_range {
diff --git a/t/git-http-backend.psgi b/t/git-http-backend.psgi
index c960714..66f4150 100644
--- a/t/git-http-backend.psgi
+++ b/t/git-http-backend.psgi
@@ -6,7 +6,6 @@ use warnings;
 use PublicInbox::GitHTTPBackend;
 use PublicInbox::Git;
 use Plack::Builder;
-use Plack::Request;
 use BSD::Resource qw(getrusage);
 my $git_dir = $ENV{GIANT_GIT_DIR} or die 'GIANT_GIT_DIR not defined in env';
 my $git = PublicInbox::Git->new($git_dir);
@@ -14,9 +13,8 @@ builder {
 	enable 'Head';
 	sub {
 		my ($env) = @_;
-		my $pr = Plack::Request->new($env);
-		if ($pr->path_info =~ m!\A/(.+)\z!s) {
-			PublicInbox::GitHTTPBackend::serve($pr, $git, $1);
+		if ($env->{PATH_INFO} =~ m!\A/(.+)\z!s) {
+			PublicInbox::GitHTTPBackend::serve($env, $git, $1);
 		} else {
 			my $ru = getrusage();
 			my $b = $ru->maxrss . "\n";
diff --git a/t/git-http-backend.t b/t/git-http-backend.t
index 889d507..e879392 100644
--- a/t/git-http-backend.t
+++ b/t/git-http-backend.t
@@ -13,7 +13,7 @@ use Cwd qw(getcwd);
 my $git_dir = $ENV{GIANT_GIT_DIR};
 plan 'skip_all' => 'GIANT_GIT_DIR not defined' unless $git_dir;
 foreach my $mod (qw(Danga::Socket
-			Plack::Util Plack::Request Plack::Builder
+			Plack::Util Plack::Builder
 			HTTP::Date HTTP::Status Net::HTTP)) {
 	eval "require $mod";
 	plan skip_all => "$mod missing for git-http-backend.t" if $@;

[PATCH 2/3] www: remove gratuitous use of Plack::Request methods

[Eric Wong]

Accessing $env directly is faster and we will eventually
remove all Plack::Request dependencies.
---
 lib/PublicInbox/WWW.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/WWW.pm b/lib/PublicInbox/WWW.pm
index 88d4f6f..a820207 100644
--- a/lib/PublicInbox/WWW.pm
+++ b/lib/PublicInbox/WWW.pm
@@ -41,9 +41,9 @@ sub call {
 	my ($self, $env) = @_;
 	my $cgi = Plack::Request->new($env);
 	my $ctx = {cgi => $cgi, pi_config => $self->{pi_config}, www => $self};
-	my $path_info = $cgi->path_info;
+	my $path_info = $env->{PATH_INFO};
+	my $method = $env->{REQUEST_METHOD};
 
-	my $method = $cgi->method;
 	if ($method eq 'POST' &&
 		 $path_info =~ m!$INBOX_RE/(git-upload-pack)\z!) {
 		my $path = $2;

[PATCH 3/3] www: remove a few more Plack::Request dependencies

[Eric Wong]

Still a work in progress, but SearchView no longer depends
on Plack::Request at all and Feed is getting there.

We now parse all query parameters up front, but we may do
that lazily again in the future.
---
 lib/PublicInbox/Feed.pm       | 18 +++++++++---------
 lib/PublicInbox/SearchView.pm | 14 +++++++-------
 lib/PublicInbox/WWW.pm        | 16 ++++++++++++----
 3 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/lib/PublicInbox/Feed.pm b/lib/PublicInbox/Feed.pm
index 81895db..07774cb 100644
--- a/lib/PublicInbox/Feed.pm
+++ b/lib/PublicInbox/Feed.pm
@@ -140,8 +140,8 @@ sub emit_html_index {
 
 	# if the 'r' query parameter is given, it is a legacy permalink
 	# which we must continue supporting:
-	my $cgi = $ctx->{cgi};
-	if ($cgi && !$cgi->param('r') && $srch) {
+	my $qp = $ctx->{qp};
+	if ($qp && !$qp->{r} && $srch) {
 		$state->{srch} = $srch;
 		$last = PublicInbox::View::emit_index_topics($state);
 		$param = 'o';
@@ -149,7 +149,7 @@ sub emit_html_index {
 		$last = emit_index_nosrch($ctx, $state);
 		$param = 'r';
 	}
-	$footer = nav_footer($cgi, $last, $feed_opts, $state, $param);
+	$footer = nav_footer($ctx, $last, $feed_opts, $state, $param);
 	if ($footer) {
 		my $list_footer = $ctx->{footer};
 		$footer .= "\n\n" . $list_footer if $list_footer;
@@ -174,9 +174,9 @@ sub emit_index_nosrch {
 }
 
 sub nav_footer {
-	my ($cgi, $last, $feed_opts, $state, $param) = @_;
-	$cgi or return '';
-	my $old_r = $cgi->param($param);
+	my ($ctx, $last, $feed_opts, $state, $param) = @_;
+	my $qp = $ctx->{qp} or return '';
+	my $old_r = $qp->{$param};
 	my $head = '    ';
 	my $next = '    ';
 	my $first = $state->{first};
@@ -186,7 +186,7 @@ sub nav_footer {
 		$next = qq!<a\nhref="?$param=$last"\nrel=next>next</a>!;
 	}
 	if ($old_r) {
-		$head = $cgi->path_info;
+		$head = $ctx->{env}->{PATH_INFO};
 		$head = qq!<a\nhref="$head">head</a>!;
 	}
 	my $atom = "<a\nhref=\"$feed_opts->{atomurl}\">Atom feed</a>";
@@ -200,11 +200,11 @@ sub each_recent_blob {
 	my $addmsg = qr!^:000000 100644 \S+ \S+ A\t(${hex}{2}/${hex}{38})$!;
 	my $delmsg = qr!^:100644 000000 \S+ \S+ D\t(${hex}{2}/${hex}{38})$!;
 	my $refhex = qr/(?:HEAD|${hex}{4,40})(?:~\d+)?/;
-	my $cgi = $ctx->{cgi};
+	my $qp = $ctx->{qp};
 
 	# revision ranges may be specified
 	my $range = 'HEAD';
-	my $r = $cgi->param('r') if $cgi;
+	my $r = $qp->{r} if $qp;
 	if ($r && ($r =~ /\A(?:$refhex\.\.)?$refhex\z/o)) {
 		$range = $r;
 	}
diff --git a/lib/PublicInbox/SearchView.pm b/lib/PublicInbox/SearchView.pm
index 0ae0505..2ec7ddf 100644
--- a/lib/PublicInbox/SearchView.pm
+++ b/lib/PublicInbox/SearchView.pm
@@ -16,7 +16,7 @@ our $LIM = 50;
 
 sub sres_top_html {
 	my ($ctx) = @_;
-	my $q = PublicInbox::SearchQuery->new($ctx->{cgi});
+	my $q = PublicInbox::SearchQuery->new($ctx->{qp});
 	my $code = 200;
 
 	# double the limit for expanded views:
@@ -260,13 +260,13 @@ use warnings;
 use PublicInbox::Hval;
 
 sub new {
-	my ($class, $cgi) = @_;
-	my $r = $cgi->param('r');
-	my ($off) = (($cgi->param('o') || '0') =~ /(\d+)/);
+	my ($class, $qp) = @_;
+
+	my $r = $qp->{r};
 	bless {
-		q => $cgi->param('q'),
-		x => $cgi->param('x') || '',
-		o => $off,
+		q => $qp->{'q'},
+		x => $qp->{x} || '',
+		o => (($qp->{o} || '0') =~ /(\d+)/),
 		r => (defined $r && $r ne '0'),
 	}, $class;
 }
diff --git a/lib/PublicInbox/WWW.pm b/lib/PublicInbox/WWW.pm
index a820207..e211cd6 100644
--- a/lib/PublicInbox/WWW.pm
+++ b/lib/PublicInbox/WWW.pm
@@ -40,7 +40,15 @@ sub run {
 sub call {
 	my ($self, $env) = @_;
 	my $cgi = Plack::Request->new($env);
-	my $ctx = {cgi => $cgi, pi_config => $self->{pi_config}, www => $self};
+	my $ctx = { cgi => $cgi, env => $env, www => $self,
+		pi_config => $self->{pi_config} };
+
+	# we don't care about multi-value
+	my %qp = map {
+		(split('=', $_, 2))
+	} split(/[&;]/, uri_unescape($env->{QUERY_STRING}));
+	$ctx->{qp} = \%qp;
+
 	my $path_info = $env->{PATH_INFO};
 	my $method = $env->{REQUEST_METHOD};
 
@@ -180,7 +188,7 @@ sub get_index {
 	require PublicInbox::Feed;
 	my $srch = searcher($ctx);
 	footer($ctx);
-	if (defined $ctx->{cgi}->param('q')) {
+	if ($ctx->{env}->{QUERY_STRING} =~ /(?:\A|[&;])q=/) {
 		require PublicInbox::SearchView;
 		PublicInbox::SearchView::sres_top_html($ctx);
 	} else {
@@ -262,7 +270,7 @@ sub footer {
 	my $cgi = $ctx->{cgi};
 	my $http = $cgi->base->as_string . $obj->{name};
 	$seen{$http} or unshift @urls, $http;
-	my $ssoma_url = PublicInbox::Hval::prurl($cgi->{env}, SSOMA_URL);
+	my $ssoma_url = PublicInbox::Hval::prurl($ctx->{env}, SSOMA_URL);
 	if (scalar(@urls) == 1) {
 		$urls = "URL for <a\nhref=\"" . $ssoma_url .
 			qq(">ssoma</a> or <b>git clone --mirror $urls[0]</b>);
@@ -394,7 +402,7 @@ sub r301 {
 		$obj = $ctx->{-inbox};
 	}
 	my $url = $obj->base_url($cgi);
-	my $qs = $cgi->env->{QUERY_STRING};
+	my $qs = $ctx->{env}->{QUERY_STRING};
 	$url .= (uri_escape_utf8($mid) . '/') if (defined $mid);
 	$url .= $suffix if (defined $suffix);
 	$url .= "?$qs" if $qs ne '';