[PATCH 0/3] http: circular reference avoidance cleanups

[PATCH 0/3] http: circular reference avoidance cleanups

[Eric Wong]

This doesn't fix any bugs, but cleans up our circular reference
avoidance techniques to avoid using weak references in httpd
entirely (and any potential performance problems from too many
backrefs).

The inbox WWW code for releasing old objects is due for a
rethink, too...

Eric Wong (3):
      http: fix spelling error
      httpd/async: remove weaken usage
      http: remove weaken usage, reduce anonsub capture scope

 lib/PublicInbox/HTTP.pm        | 27 +++++++++++++--------------
 lib/PublicInbox/HTTPD/Async.pm | 31 +++++++++++++++++--------------
 2 files changed, 30 insertions(+), 28 deletions(-)

[PATCH 1/3] http: fix spelling error

[Eric Wong]

Oops.  And we'll be fixing circular references from now...
---
 lib/PublicInbox/HTTP.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index c4b74b4..03ce4fe 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -473,7 +473,7 @@ sub close {
 	my $self = shift;
 	my $forward = $self->{forward};
 	my $env = $self->{env};
-	delete $env->{'psgix.io'} if $env; # prevent circular referernces
+	delete $env->{'psgix.io'} if $env; # prevent circular references
 	$self->{pull} = $self->{forward} = $self->{env} = undef;
 	if ($forward) {
 		eval { $forward->close };
-- 
EW

[PATCH 2/3] httpd/async: remove weaken usage

[Eric Wong]

We do not need to use weaken() here, so avoid it to simplify our
interactions with Perl; as weaken requires additional storage
and (it seems) time complexity.
---
 lib/PublicInbox/HTTPD/Async.pm | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm
index 79951ca..54b6245 100644
--- a/lib/PublicInbox/HTTPD/Async.pm
+++ b/lib/PublicInbox/HTTPD/Async.pm
@@ -10,7 +10,6 @@ use strict;
 use warnings;
 use base qw(Danga::Socket);
 use fields qw(cb cleanup);
-use Scalar::Util qw(weaken);
 require PublicInbox::EvCleanup;
 
 sub new {
@@ -29,22 +28,17 @@ sub restart_read_cb ($) {
 	sub { $self->watch_read(1) }
 }
 
-sub async_pass {
-	my ($self, $http, $fh, $bref) = @_;
-	# In case the client HTTP connection ($http) dies, it
-	# will automatically close this ($self) object.
-	$http->{forward} = $self;
-	$fh->write($$bref);
-	my $restart_read = restart_read_cb($self);
-	weaken($self);
-	$self->{cb} = sub {
+sub main_cb ($$$) {
+	my ($http, $fh, $bref) = @_;
+	sub {
+		my ($self) = @_;
 		my $r = sysread($self->{sock}, $$bref, 8192);
 		if ($r) {
 			$fh->write($$bref);
 			return if $http->{closed};
 			if ($http->{write_buf_size}) {
 				$self->watch_read(0);
-				$http->write($restart_read); # D::S::write
+				$http->write(restart_read_cb($self));
 			}
 			# stay in watch_read, but let other clients
 			# get some work done, too.
@@ -60,9 +54,18 @@ sub async_pass {
 	}
 }
 
-sub event_read { $_[0]->{cb}->() }
-sub event_hup { $_[0]->{cb}->() }
-sub event_err { $_[0]->{cb}->() }
+sub async_pass {
+	my ($self, $http, $fh, $bref) = @_;
+	# In case the client HTTP connection ($http) dies, it
+	# will automatically close this ($self) object.
+	$http->{forward} = $self;
+	$fh->write($$bref); # PublicInbox:HTTP::{chunked,identity}_wcb
+	$self->{cb} = main_cb($http, $fh, $bref);
+}
+
+sub event_read { $_[0]->{cb}->(@_) }
+sub event_hup { $_[0]->{cb}->(@_) }
+sub event_err { $_[0]->{cb}->(@_) }
 sub sysread { shift->{sock}->sysread(@_) }
 
 sub close {
-- 
EW

[PATCH 3/3] http: remove weaken usage, reduce anonsub capture scope

[Eric Wong]

Avoiding weaken here is no more dangerous than the existing
circular refs (e.g. psgix.io) we create and manage throughout
the lifetime of the connection.  So, trust ourselves to maintain
the data structure properly and avoid triggering extra memory
usage.

While we're at it, avoid having anonymous subroutines capture
more variables than necessary to simplify reference auditing.
---
 lib/PublicInbox/HTTP.pm | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index 03ce4fe..3530f8b 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -16,7 +16,6 @@ use Fcntl qw(:seek);
 use Plack::HTTPParser qw(parse_http_request); # XS or pure Perl
 use HTTP::Status qw(status_message);
 use HTTP::Date qw(time2str);
-use Scalar::Util qw(weaken);
 use IO::Handle;
 use constant {
 	CHUNK_START => -1,   # [a-f0-9]+\r\n
@@ -237,12 +236,14 @@ sub next_request ($) {
 	}
 }
 
-sub response_done ($$) {
+sub response_done_cb ($$) {
 	my ($self, $alive) = @_;
-	my $env = $self->{env};
-	$self->{env} = undef;
-	$self->write("0\r\n\r\n") if $alive == 2;
-	$self->write(sub { $alive ? next_request($self) : $self->close });
+	sub {
+		my $env = $self->{env};
+		$self->{env} = undef;
+		$self->write("0\r\n\r\n") if $alive == 2;
+		$self->write(sub{$alive ? next_request($self) : $self->close});
+	}
 }
 
 sub getline_cb ($$$) {
@@ -283,10 +284,8 @@ sub getline_cb ($$$) {
 	$close->();
 }
 
-sub getline_response {
-	my ($self, $body, $write, $close) = @_;
-	$self->{forward} = $body;
-	weaken($self);
+sub getline_response ($$$) {
+	my ($self, $write, $close) = @_;
 	my $pull = $self->{pull} = sub { getline_cb($self, $write, $close) };
 	$pull->();
 }
@@ -294,15 +293,15 @@ sub getline_response {
 sub response_write {
 	my ($self, $env, $res) = @_;
 	my $alive = response_header_write($self, $env, $res);
-
+	my $close = response_done_cb($self, $alive);
 	my $write = $alive == 2 ? chunked_wcb($self) : identity_wcb($self);
-	my $close = sub { response_done($self, $alive) };
 	if (defined(my $body = $res->[2])) {
 		if (ref $body eq 'ARRAY') {
 			$write->($_) foreach @$body;
 			$close->();
 		} else {
-			getline_response($self, $body, $write, $close);
+			$self->{forward} = $body;
+			getline_response($self, $write, $close);
 		}
 	} else {
 		# this is returned to the calling application:
-- 
EW