user/dev discussion of public-inbox itself
 help / color / Atom feed
From: Eric Wong <e@80x24.org>
To: meta@public-inbox.org
Subject: [PATCH 47/57] nntp: simplify long response logic and fix nesting
Date: Mon, 24 Jun 2019 02:52:48 +0000
Message-ID: <20190624025258.25592-48-e@80x24.org> (raw)
In-Reply-To: <20190624025258.25592-1-e@80x24.org>

We can get rid of the {long_res} field and reuse the write
buffer ordering logic to prevent nesting of responses from
requeue.

On FreeBSD, this fixes a problem of callbacks firing twice
because kqueue as event_step is now our only callback entry
point.

There's a slight change in the stdout "logging" format, in
that we can no longer distinguish between writes blocked
due to slow clients or deferred long responses.  Not sure
if this affects anybody parsing logs or not, but preserving
the old format could prove expensive and not worth the
effort.
---
 lib/PublicInbox/NNTP.pm | 61 +++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 12ce4e68..6acfcc1b 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -6,7 +6,7 @@ package PublicInbox::NNTP;
 use strict;
 use warnings;
 use base qw(PublicInbox::DS);
-use fields qw(nntpd article rbuf ng long_res);
+use fields qw(nntpd article rbuf ng);
 use PublicInbox::Search;
 use PublicInbox::Msgmap;
 use PublicInbox::MID qw(mid_escape);
@@ -45,17 +45,7 @@ sub next_tick () {
 	$nextt = undef;
 	my $q = $nextq;
 	$nextq = [];
-	foreach my $nntp (@$q) {
-		# for request && response protocols, always finish writing
-		# before finishing reading:
-		if (my $long_cb = $nntp->{long_res}) {
-			$nntp->write($long_cb);
-		} else {
-			# pipelined request, we bypassed socket-readiness
-			# checks to get here:
-			event_step($nntp);
-		}
-	}
+	event_step($_) for @$q;
 }
 
 sub requeue ($) {
@@ -633,8 +623,7 @@ sub get_range ($$) {
 }
 
 sub long_response ($$) {
-	my ($self, $cb) = @_;
-	die "BUG: nested long response" if $self->{long_res};
+	my ($self, $cb) = @_; # cb returns true if more, false if done
 
 	my $fd = fileno($self->{sock});
 	defined $fd or return;
@@ -642,36 +631,38 @@ sub long_response ($$) {
 	# clients should not be sending us stuff and making us do more
 	# work while we are stream a response to them
 	my $t0 = now();
-	$self->{long_res} = sub {
+	my $long_cb; # DANGER: self-referential
+	$long_cb = sub {
+		# wbuf is unset or empty, here; $cb may add to it
 		my $more = eval { $cb->() };
 		if ($@ || !$self->{sock}) { # something bad happened...
-			delete $self->{long_res};
-
+			$long_cb = undef;
+			my $diff = now() - $t0;
 			if ($@) {
 				err($self,
 				    "%s during long response[$fd] - %0.6f",
-				    $@, now() - $t0);
-			}
-			if ($self->{sock}) {
-				update_idle_time($self);
-				requeue($self);
-			} else {
-				out($self, " deferred[$fd] aborted - %0.6f",
-				           now() - $t0);
+				    $@, $diff);
 			}
+			out($self, " deferred[$fd] aborted - %0.6f", $diff);
+			$self->close;
 		} elsif ($more) { # $self->{wbuf}:
+			update_idle_time($self);
+
 			# no recursion, schedule another call ASAP
 			# but only after all pending writes are done
-			update_idle_time($self);
-			requeue($self);
+			my $wbuf = $self->{wbuf} ||= [];
+			push @$wbuf, $long_cb;
+
+			# wbuf may be populated by $cb, no need to rearm if so:
+			requeue($self) if scalar(@$wbuf) == 1;
 		} else { # all done!
-			delete $self->{long_res};
+			$long_cb = undef;
 			res($self, '.');
 			out($self, " deferred[$fd] done - %0.6f", now() - $t0);
-			requeue($self);
+			requeue($self) unless $self->{wbuf};
 		}
 	};
-	$self->{long_res}->(); # kick off!
+	$self->write($long_cb); # kick off!
 	undef;
 }
 
@@ -986,9 +977,8 @@ sub event_step {
 		my $t0 = now();
 		my $fd = fileno($self->{sock});
 		$r = eval { process_line($self, $line) };
-		my $d = $self->{long_res} ?
-			" deferred[$fd]" : '';
-		out($self, "[$fd] %s - %0.6f$d", $line, now() - $t0);
+		my $pending = $self->{wbuf} ? ' pending' : '';
+		out($self, "[$fd] %s - %0.6f$pending", $line, now() - $t0);
 	}
 
 	return $self->close if $r < 0;
@@ -998,7 +988,7 @@ sub event_step {
 
 	# maybe there's more pipelined data, or we'll have
 	# to register it for socket-readiness notifications
-	requeue($self) unless ($self->{long_res} || $self->{wbuf});
+	requeue($self) unless $self->{wbuf};
 }
 
 sub not_idle_long ($$) {
@@ -1012,8 +1002,7 @@ sub not_idle_long ($$) {
 # for graceful shutdown in PublicInbox::Daemon:
 sub busy {
 	my ($self, $now) = @_;
-	($self->{rbuf} ne '' || $self->{long_res} ||
-		$self->{wbuf} || not_idle_long($self, $now));
+	($self->{rbuf} ne '' || $self->{wbuf} || not_idle_long($self, $now));
 }
 
 1;
-- 
EW


  parent reply index

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-24  2:52 [PATCH 00/57] ds: shrink, TLS support, buffer writes to FS Eric Wong
2019-06-24  2:52 ` [PATCH 01/57] ds: get rid of {closed} field Eric Wong
2019-06-24  2:52 ` [PATCH 02/57] ds: get rid of more unused debug instance methods Eric Wong
2019-06-24  2:52 ` [PATCH 03/57] ds: use and export monotonic now() Eric Wong
2019-06-24  2:52 ` [PATCH 04/57] AddTimer: avoid clock_gettime for the '0' case Eric Wong
2019-06-24  2:52 ` [PATCH 05/57] ds: get rid of on_incomplete_write wrapper Eric Wong
2019-06-24  2:52 ` [PATCH 06/57] ds: lazy initialize wbuf_off Eric Wong
2019-06-24  2:52 ` [PATCH 07/57] ds: split out from ->flush_write and ->write Eric Wong
2019-06-24  2:52 ` [PATCH 08/57] ds: lazy-initialize wbuf Eric Wong
2019-06-24  2:52 ` [PATCH 09/57] ds: don't pass `events' arg to EPOLL_CTL_DEL Eric Wong
2019-06-24  2:52 ` [PATCH 10/57] ds: remove support for DS->write(undef) Eric Wong
2019-06-24  2:52 ` [PATCH 11/57] http: favor DS->write(strref) when reasonable Eric Wong
2019-06-24  2:52 ` [PATCH 12/57] ds: share send(..., MSG_MORE) logic Eric Wong
2019-06-24  2:52 ` [PATCH 13/57] ds: switch write buffering to use a tempfile Eric Wong
2019-06-24  2:52 ` [PATCH 14/57] ds: get rid of redundant and unnecessary POLL* constants Eric Wong
2019-06-24  2:52 ` [PATCH 15/57] syscall: get rid of unused EPOLL* constants Eric Wong
2019-06-24  2:52 ` [PATCH 16/57] syscall: get rid of unnecessary uname local vars Eric Wong
2019-06-24  2:52 ` [PATCH 17/57] ds: set event flags directly at initialization Eric Wong
2019-06-24  2:52 ` [PATCH 18/57] ds: import IO::KQueue namespace Eric Wong
2019-06-24  2:52 ` [PATCH 19/57] ds: share watch_chg between watch_read/watch_write Eric Wong
2019-06-24  2:52 ` [PATCH 20/57] ds: remove IO::Poll support (for now) Eric Wong
2019-06-24  2:52 ` [PATCH 21/57] ds: get rid of event_watch field Eric Wong
2019-06-24  2:52 ` [PATCH 22/57] httpd/async: remove EINTR check Eric Wong
2019-06-24  2:52 ` [PATCH 23/57] spawn: remove `Blocking' flag handling Eric Wong
2019-06-24  2:52 ` [PATCH 24/57] qspawn: describe where `$rpipe' come from Eric Wong
2019-06-24  2:52 ` [PATCH 25/57] http|nntp: favor "$! == EFOO" over $!{EFOO} checks Eric Wong
2019-06-24  2:52 ` [PATCH 26/57] ds: favor `delete' over assigning fields to `undef' Eric Wong
2019-06-24  2:52 ` [PATCH 27/57] http: don't pass extra args to PublicInbox::DS::close Eric Wong
2019-06-24  2:52 ` [PATCH 28/57] ds: pass $self to code references Eric Wong
2019-06-24  2:52 ` [PATCH 29/57] evcleanup: replace _run_asap with `event_step' callback Eric Wong
2019-06-24  2:52 ` [PATCH 30/57] ds: remove pointless exit calls Eric Wong
2019-06-24  2:52 ` [PATCH 31/57] http|nntp: be explicit about bytes::length on rbuf Eric Wong
2019-06-24  2:52 ` [PATCH 32/57] ds: hoist out do_read from NNTP and HTTP Eric Wong
2019-06-24  2:52 ` [PATCH 33/57] nntp: simplify re-arming/requeue logic Eric Wong
2019-06-24  2:52 ` [PATCH 34/57] allow use of PerlIO layers for filesystem writes Eric Wong
2019-06-24  2:52 ` [PATCH 35/57] ds: deal better with FS-related errors IO buffers Eric Wong
2019-06-24  2:52 ` [PATCH 36/57] nntp: wait for writability before sending greeting Eric Wong
2019-06-24  2:52 ` [PATCH 37/57] nntp: NNTPS and NNTP+STARTTLS working Eric Wong
2019-06-24  2:52 ` [PATCH 38/57] certs/create-certs.perl: fix cert validity on 32-bit Eric Wong
2019-06-24  2:52 ` [PATCH 39/57] daemon: map inherited sockets to well-known schemes Eric Wong
2019-06-24  2:52 ` [PATCH 40/57] ds|nntp: use CORE::close on socket Eric Wong
2019-06-24  2:52 ` [PATCH 41/57] nntp: call SSL_shutdown in normal cases Eric Wong
2019-06-24  2:52 ` [PATCH 42/57] t/nntpd-tls: slow client connection test Eric Wong
2019-06-24  2:52 ` [PATCH 43/57] daemon: use SSL_MODE_RELEASE_BUFFERS Eric Wong
2019-06-24  2:52 ` [PATCH 44/57] ds: allow ->write callbacks to syswrite directly Eric Wong
2019-06-24  2:52 ` [PATCH 45/57] nntp: reduce allocations for greeting Eric Wong
2019-06-24  2:52 ` [PATCH 46/57] ds: always use EV_ADD with EV_SET Eric Wong
2019-06-24  2:52 ` Eric Wong [this message]
2019-06-24  2:52 ` [PATCH 48/57] ds: flush_write runs ->write callbacks even if closed Eric Wong
2019-06-24  2:52 ` [PATCH 49/57] nntp: lazily allocate and stash rbuf Eric Wong
2019-06-24  2:52 ` [PATCH 50/57] ci: require IO::KQueue on FreeBSD, for now Eric Wong
2019-06-24  2:52 ` [PATCH 51/57] nntp: send greeting immediately for plain sockets Eric Wong
2019-06-24  2:52 ` [PATCH 52/57] daemon: set TCP_DEFER_ACCEPT on everything but NNTP Eric Wong
2019-06-24  2:52 ` [PATCH 53/57] daemon: use FreeBSD accept filters on non-NNTP Eric Wong
2019-06-24  2:52 ` [PATCH 54/57] ds: split out IO::KQueue-specific code Eric Wong
2019-06-24  5:24   ` Eric Wong
2019-06-24  2:52 ` [PATCH 55/57] ds: reimplement IO::Poll support to look like epoll Eric Wong
2019-06-24  2:52 ` [PATCH 56/57] Revert "ci: require IO::KQueue on FreeBSD, for now" Eric Wong
2019-06-24  2:52 ` [PATCH 57/57] ds: reduce overhead of tempfile creation Eric Wong
2019-06-24  5:25 ` [PATCH 58/57] Makefile: skip DSKQXS in global syntax check Eric Wong
2019-06-24 18:28 ` [PATCH 59/57] ds: ->write must not clobber empty wbuf array Eric Wong

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://public-inbox.org/README

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190624025258.25592-48-e@80x24.org \
    --to=e@80x24.org \
    --cc=meta@public-inbox.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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

Example config snippet for mirrors

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/

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