user/dev discussion of public-inbox itself
 help / color / Atom feed
* [PATCH] ds: stop distinguishing event read and write callbacks
@ 2019-06-16 17:45 Eric Wong
  0 siblings, 0 replies; only message in thread
From: Eric Wong @ 2019-06-16 17:45 UTC (permalink / raw)
  To: meta

Having separate read/write callbacks in every class is too
confusing to my easily-confused mind.  Instead, give every class
an "event_step" callback which is easier to wrap my head around.

This will make future code to support IO::Socket::SSL-wrapped
sockets easier-to-digest, since SSL_write() can require waiting
on POLLIN events, and SSL_read() can require waiting on POLLOUT
events.
---
 lib/PublicInbox/DS.pm          | 47 +++-------------------------------
 lib/PublicInbox/EvCleanup.pm   |  4 +--
 lib/PublicInbox/HTTP.pm        | 22 +++++++++++-----
 lib/PublicInbox/HTTPD/Async.pm |  2 +-
 lib/PublicInbox/Listener.pm    |  2 +-
 lib/PublicInbox/NNTP.pm        | 21 ++++++++-------
 lib/PublicInbox/ParentPipe.pm  |  2 +-
 7 files changed, 35 insertions(+), 65 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 2f028a3..2b04886 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -239,21 +239,6 @@ sub RunTimers {
     return $timeout;
 }
 
-sub event_step ($) {
-    my ($self) = @_;
-    return if $self->{closed};
-
-    my $wbuf = $self->{wbuf};
-    if (@$wbuf) {
-        $self->event_write;
-        return if $self->{closed} || scalar(@$wbuf);
-    }
-
-    # only read more requests if we've drained the write buffer,
-    # otherwise we can be buffering infinitely w/o backpressure
-    $self->event_read;
-}
-
 ### The epoll-based event loop. Gets installed as EventLoop if IO::Epoll loads
 ### okay.
 sub EpollEventLoop {
@@ -267,13 +252,11 @@ sub EpollEventLoop {
         # get up to 1000 events
         my $evcount = epoll_wait($Epoll, 1000, $timeout, \@events);
         for ($i=0; $i<$evcount; $i++) {
-            my $ev = $events[$i];
-
             # it's possible epoll_wait returned many events, including some at the end
             # that ones in the front triggered unregister-interest actions.  if we
             # can't find the %sock entry, it's because we're no longer interested
             # in that event.
-            event_step($DescriptorMap{$ev->[0]});
+            $DescriptorMap{$events[$i]->[0]}->event_step;
         }
         return unless PostEventLoop();
     }
@@ -316,9 +299,7 @@ sub PollEventLoop {
         # Fetch handles with read events
         while (@poll) {
             my ($fd, $state) = splice(@poll, 0, 2);
-            next unless $state;
-
-            event_step($DescriptorMap{$fd});
+            $DescriptorMap{$fd}->event_step if $state;
         }
 
         return unless PostEventLoop();
@@ -345,8 +326,7 @@ sub KQueueEventLoop {
         }
 
         foreach my $kev (@ret) {
-            my ($fd, $filter, $flags, $fflags) = @$kev;
-            event_step($DescriptorMap{$fd});
+            $DescriptorMap{$kev->[0]}->event_step;
         }
         return unless PostEventLoop();
     }
@@ -647,27 +627,6 @@ sub on_incomplete_write {
     $self->watch_write(1);
 }
 
-=head2 (VIRTUAL) C<< $obj->event_read() >>
-
-Readable event handler. Concrete deriviatives of PublicInbox::DS should
-provide an implementation of this. The default implementation is a noop
-if called.
-
-=cut
-sub event_read {} # noop
-
-=head2 C<< $obj->event_write() >>
-
-Writable event handler. Concrete deriviatives of PublicInbox::DS may wish to
-provide an implementation of this. The default implementation calls
-C<write()> with an C<undef>.
-
-=cut
-sub event_write {
-    my $self = shift;
-    $self->write(undef);
-}
-
 =head2 C<< $obj->watch_read( $boolean ) >>
 
 Turn 'readable' event notification on or off.
diff --git a/lib/PublicInbox/EvCleanup.pm b/lib/PublicInbox/EvCleanup.pm
index f76fb68..c64e238 100644
--- a/lib/PublicInbox/EvCleanup.pm
+++ b/lib/PublicInbox/EvCleanup.pm
@@ -25,7 +25,7 @@ sub once_init () {
 	fcntl($w, 1031, 4096) if $^O eq 'linux'; # 1031: F_SETPIPE_SZ
 	$self->SUPER::new($w);
 
-	# always writable, since PublicInbox::EvCleanup::event_write
+	# always writable, since PublicInbox::EvCleanup::event_step
 	# never drains wbuf.  We can avoid wasting a hash slot by
 	# stuffing the read-end of the pipe into the never-to-be-touched
 	# wbuf
@@ -57,7 +57,7 @@ sub _run_later () {
 }
 
 # Called by PublicInbox::DS
-sub event_write {
+sub event_step {
 	my ($self) = @_;
 	$self->watch_write(0);
 	_run_asap();
diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index 4fbc34e..45bf23e 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -64,10 +64,18 @@ sub new ($$$) {
 	$self;
 }
 
-sub event_read { # called by PublicInbox::DS
+sub event_step { # called by PublicInbox::DS
 	my ($self) = @_;
 
-	return event_read_input($self) if defined $self->{env};
+	my $wbuf = $self->{wbuf};
+	if (@$wbuf) {
+		$self->write(undef);
+		return if $self->{closed} || scalar(@$wbuf);
+	}
+	# only read more requests if we've drained the write buffer,
+	# otherwise we can be buffering infinitely w/o backpressure
+
+	return read_input($self) if defined $self->{env};
 
 	my $off = length($self->{rbuf});
 	my $r = sysread($self->{sock}, $self->{rbuf}, 8192, $off);
@@ -101,13 +109,14 @@ sub rbuf_process {
 	my $len = input_prepare($self, \%env);
 	defined $len or return write_err($self, undef); # EMFILE/ENFILE
 
-	$len ? event_read_input($self) : app_dispatch($self);
+	$len ? read_input($self) : app_dispatch($self);
 }
 
-sub event_read_input ($) {
+sub read_input ($) {
 	my ($self) = @_;
 	my $env = $self->{env};
-	return event_read_input_chunked($self) if env_chunked($env);
+	return if $env->{REMOTE_ADDR}; # in app dispatch
+	return read_input_chunked($self) if env_chunked($env);
 
 	# env->{CONTENT_LENGTH} (identity)
 	my $sock = $self->{sock};
@@ -229,7 +238,6 @@ sub identity_wcb ($) {
 
 sub next_request ($) {
 	my ($self) = @_;
-	$self->watch_write(0);
 	if ($self->{rbuf} eq '') { # wait for next request
 		$self->watch_read(1);
 	} else { # avoid recursion for pipelined requests
@@ -392,7 +400,7 @@ sub write_in_full {
 	$rv
 }
 
-sub event_read_input_chunked { # unlikely...
+sub read_input_chunked { # unlikely...
 	my ($self) = @_;
 	my $input = $self->{env}->{'psgi.input'};
 	my $sock = $self->{sock};
diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm
index 4d0c8d5..604627a 100644
--- a/lib/PublicInbox/HTTPD/Async.pm
+++ b/lib/PublicInbox/HTTPD/Async.pm
@@ -75,7 +75,7 @@ sub async_pass {
 	$self->{cb} = main_cb($http, $fh, $bref);
 }
 
-sub event_read { $_[0]->{cb}->(@_) }
+sub event_step { $_[0]->{cb}->(@_) }
 
 sub close {
 	my $self = shift;
diff --git a/lib/PublicInbox/Listener.pm b/lib/PublicInbox/Listener.pm
index a75a6fd..6ee3abb 100644
--- a/lib/PublicInbox/Listener.pm
+++ b/lib/PublicInbox/Listener.pm
@@ -23,7 +23,7 @@ sub new ($$$) {
 	$self
 }
 
-sub event_read {
+sub event_step {
 	my ($self) = @_;
 	my $sock = $self->{sock};
 
diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index fa412f8..796ac74 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -53,7 +53,7 @@ sub next_tick () {
 		} else {
 			# pipelined request, we bypassed socket-readiness
 			# checks to get here:
-			event_read($nntp);
+			event_step($nntp);
 
 			# maybe there's more pipelined data, or we'll have
 			# to register it for socket-readiness notifications
@@ -964,17 +964,20 @@ sub do_more ($$) {
 	do_write($self, $data);
 }
 
-sub event_write {
+sub event_step {
 	my ($self) = @_;
-	update_idle_time($self);
-	# only continue watching for readability when we are done writing:
-	if ($self->write(undef) == 1 && !$self->{long_res}) {
-		$self->watch_read(1);
+	return if $self->{closed};
+
+	my $wbuf = $self->{wbuf};
+	if (@$wbuf) {
+		update_idle_time($self);
+		$self->write(undef);
+		return if $self->{closed} || scalar(@$wbuf);
 	}
-}
+	return if $self->{long_res};
+	# only read more requests if we've drained the write buffer,
+	# otherwise we can be buffering infinitely w/o backpressure
 
-sub event_read {
-	my ($self) = @_;
 	use constant LINE_MAX => 512; # RFC 977 section 2.3
 	my $rbuf = \($self->{rbuf});
 	my $r;
diff --git a/lib/PublicInbox/ParentPipe.pm b/lib/PublicInbox/ParentPipe.pm
index 25f13a8..a9f05fc 100644
--- a/lib/PublicInbox/ParentPipe.pm
+++ b/lib/PublicInbox/ParentPipe.pm
@@ -16,6 +16,6 @@ sub new ($$$) {
 	$self;
 }
 
-sub event_read { $_[0]->{cb}->($_[0]) }
+sub event_step { $_[0]->{cb}->($_[0]) }
 
 1;
-- 
EW


^ permalink raw reply	[flat|nested] only message in thread

only message in thread, back to index

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-16 17:45 [PATCH] ds: stop distinguishing event read and write callbacks Eric Wong

user/dev discussion of public-inbox itself

Archives are clonable:
	git clone --mirror http://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/

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