From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <e@80x24.org>
X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net
X-Spam-Level: 
X-Spam-ASN:  
X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,BAYES_00
	shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2
Received: from localhost (dcvr.yhbt.net [127.0.0.1])
	by dcvr.yhbt.net (Postfix) with ESMTP id 16B661F461
	for <meta@public-inbox.org>; Wed, 10 Jul 2019 08:26:14 +0000 (UTC)
From: Eric Wong <e@80x24.org>
To: meta@public-inbox.org
Subject: [PATCH] http|nntp: avoid recursion inside ->write
Date: Wed, 10 Jul 2019 08:26:13 +0000
Message-Id: <20190710082613.31816-1-e@80x24.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
List-Id: <meta.public-inbox.org>

In HTTP.pm, we can use the same technique NNTP.pm uses with
long_response with the $long_cb callback and avoid storing
$pull in the per-client structure at all.  We can also reuse
the same logic to push the callback into wbuf from NNTP.

This does NOT introduce a new circular reference, but documents
it more clearly.
---
 lib/PublicInbox/HTTP.pm | 64 ++++++++++++++++++++---------------------
 lib/PublicInbox/NNTP.pm |  3 +-
 2 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index 60b287c4..5afe167e 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -11,7 +11,7 @@ package PublicInbox::HTTP;
 use strict;
 use warnings;
 use base qw(PublicInbox::DS);
-use fields qw(httpd env input_left remote_addr remote_port forward pull);
+use fields qw(httpd env input_left remote_addr remote_port forward);
 use bytes (); # only for bytes::length
 use Fcntl qw(:seek);
 use Plack::HTTPParser qw(parse_http_request); # XS or pure Perl
@@ -260,48 +260,49 @@ sub response_done_cb ($$) {
 	}
 }
 
-sub getline_cb ($$$) {
+sub getline_response ($$$) {
 	my ($self, $write, $close) = @_;
-	local $/ = \8192;
-	my $forward = $self->{forward};
-	# limit our own running time for fairness with other
-	# clients and to avoid buffering too much:
-	if ($forward) {
-		my $buf = eval { $forward->getline };
+	my $pull; # DANGER: self-referential
+	$pull = sub {
+		my $forward = $self->{forward};
+		# limit our own running time for fairness with other
+		# clients and to avoid buffering too much:
+		my $buf = eval {
+			local $/ = \8192;
+			$forward->getline;
+		} if $forward;
+
 		if (defined $buf) {
 			$write->($buf); # may close in PublicInbox::DS::write
+
 			if ($self->{sock}) {
-				my $next = $self->{pull};
-				if ($self->{wbuf}) {
-					$self->write($next);
-				} else {
-					PublicInbox::DS::requeue($next);
-				}
-				return;
+				my $wbuf = $self->{wbuf} ||= [];
+				push @$wbuf, $pull;
+
+				# wbuf may be populated by $write->($buf),
+				# no need to rearm if so:
+				$self->requeue if scalar(@$wbuf) == 1;
+				return; # likely
 			}
 		} elsif ($@) {
 			err($self, "response ->getline error: $@");
-			$forward = undef;
 			$self->close;
 		}
-	}
 
-	delete @$self{qw(forward pull)};
-	# avoid recursion
-	if ($forward) {
-		eval { $forward->close };
-		if ($@) {
-			err($self, "response ->close error: $@");
-			$self->close; # idempotent
+		$pull = undef; # all done!
+		# avoid recursion
+		if (delete $self->{forward}) {
+			eval { $forward->close };
+			if ($@) {
+				err($self, "response ->close error: $@");
+				$self->close; # idempotent
+			}
 		}
-	}
-	$close->();
-}
+		$forward = undef;
+		$close->(); # call response_done_cb
+	};
 
-sub getline_response ($$$) {
-	my ($self, $write, $close) = @_;
-	my $pull = $self->{pull} = sub { getline_cb($self, $write, $close) };
-	$pull->();
+	$pull->(); # kick-off!
 }
 
 sub response_write {
@@ -453,7 +454,6 @@ sub close {
 	if (my $env = delete $self->{env}) {
 		delete $env->{'psgix.io'}; # prevent circular references
 	}
-	delete $self->{pull};
 	if (my $forward = delete $self->{forward}) {
 		eval { $forward->close };
 		err($self, "forward ->close error: $@") if $@;
diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 6796a3c4..f4208f87 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -658,7 +658,8 @@ sub long_response ($$) {
 			$long_cb = undef;
 			res($self, '.');
 			out($self, " deferred[$fd] done - %0.6f", now() - $t0);
-			$self->requeue unless $self->{wbuf};
+			my $wbuf = $self->{wbuf};
+			$self->requeue unless $wbuf && @$wbuf;
 		}
 	};
 	$self->write($long_cb); # kick off!
-- 
EW