user/dev discussion of public-inbox itself
 help / Atom feed
* [PATCH 0/3] http: circular reference avoidance cleanups
@ 2017-01-04 11:20 Eric Wong
  2017-01-04 11:20 ` [PATCH 1/3] http: fix spelling error Eric Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Eric Wong @ 2017-01-04 11:20 UTC (permalink / raw)
  To: meta

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(-)


^ permalink raw reply	[flat|threaded] 4+ messages in thread

* [PATCH 1/3] http: fix spelling error
  2017-01-04 11:20 [PATCH 0/3] http: circular reference avoidance cleanups Eric Wong
@ 2017-01-04 11:20 ` Eric Wong
  2017-01-04 11:20 ` [PATCH 2/3] httpd/async: remove weaken usage Eric Wong
  2017-01-04 11:20 ` [PATCH 3/3] http: remove weaken usage, reduce anonsub capture scope Eric Wong
  2 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2017-01-04 11:20 UTC (permalink / raw)
  To: meta

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


^ permalink raw reply	[flat|threaded] 4+ messages in thread

* [PATCH 2/3] httpd/async: remove weaken usage
  2017-01-04 11:20 [PATCH 0/3] http: circular reference avoidance cleanups Eric Wong
  2017-01-04 11:20 ` [PATCH 1/3] http: fix spelling error Eric Wong
@ 2017-01-04 11:20 ` Eric Wong
  2017-01-04 11:20 ` [PATCH 3/3] http: remove weaken usage, reduce anonsub capture scope Eric Wong
  2 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2017-01-04 11:20 UTC (permalink / raw)
  To: meta

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


^ permalink raw reply	[flat|threaded] 4+ messages in thread

* [PATCH 3/3] http: remove weaken usage, reduce anonsub capture scope
  2017-01-04 11:20 [PATCH 0/3] http: circular reference avoidance cleanups Eric Wong
  2017-01-04 11:20 ` [PATCH 1/3] http: fix spelling error Eric Wong
  2017-01-04 11:20 ` [PATCH 2/3] httpd/async: remove weaken usage Eric Wong
@ 2017-01-04 11:20 ` Eric Wong
  2 siblings, 0 replies; 4+ messages in thread
From: Eric Wong @ 2017-01-04 11:20 UTC (permalink / raw)
  To: meta

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


^ permalink raw reply	[flat|threaded] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-04 11:20 [PATCH 0/3] http: circular reference avoidance cleanups Eric Wong
2017-01-04 11:20 ` [PATCH 1/3] http: fix spelling error Eric Wong
2017-01-04 11:20 ` [PATCH 2/3] httpd/async: remove weaken usage Eric Wong
2017-01-04 11:20 ` [PATCH 3/3] http: remove weaken usage, reduce anonsub capture scope Eric Wong

user/dev discussion of public-inbox itself

Archives are clonable:
	git clone --mirror https://public-inbox.org/meta
	git clone --mirror http://czquwvybam4bgbro.onion/meta
	git clone --mirror http://hjrcffqmbrq6wope.onion/meta
	git clone --mirror http://ou63pmih66umazou.onion/meta

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.mail.public-inbox.meta
	nntp://ou63pmih66umazou.onion/inbox.comp.mail.public-inbox.meta
	nntp://czquwvybam4bgbro.onion/inbox.comp.mail.public-inbox.meta
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.mail.public-inbox.meta
	nntp://news.gmane.org/gmane.mail.public-inbox.general

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox