ds: simplify write buffer accounting

Keeping track of write_buf_size was redundant and pointless when
we can simply check the number of elements in the buffer array.
Multiple sources of truth leads to confusion; confusion leads to
bugs.

Finally, rename the prefixes to 'wbuf' to ensure we loudly
(instead of silently) break any external dependencies being
ported over from Danga::Socket, as further changes are pending.

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 03612ce870e17ac66082545c2c0939095def3d29..172a9f52ad53b99e5ba394c2b5aef0e70c363450 100644 (file)
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -27,9 +27,8 @@ use PublicInbox::Syscall qw(:epoll);
 
 use fields ('sock',              # underlying socket
             'fd',                # numeric file descriptor
-            'write_buf',         # arrayref of scalars, scalarrefs, or coderefs to write
-            'write_buf_offset',  # offset into first array of write_buf to start writing at
-            'write_buf_size',    # total length of data in all write_buf items
+            'wbuf',              # arrayref of scalars, scalarrefs, or coderefs to write
+            'wbuf_off',  # offset into first element of wbuf to start writing at
             'closed',            # bool: socket is closed
             'event_watch',       # bitmask of events the client is interested in (POLLIN,OUT,etc.)
             );
@@ -449,9 +448,8 @@ sub new {
         unless $sock && $fd;
 
     $self->{fd}          = $fd;
-    $self->{write_buf}      = [];
-    $self->{write_buf_offset} = 0;
-    $self->{write_buf_size} = 0;
+    $self->{wbuf} = [];
+    $self->{wbuf_off} = 0;
     $self->{closed} = 0;
 
     my $ev = $self->{event_watch} = POLLERR|POLLHUP|POLLNVAL;
@@ -552,7 +550,7 @@ sub _cleanup {
     # we need to flush our write buffer, as there may
     # be self-referential closures (sub { $client->close })
     # preventing the object from being destroyed
-    $self->{write_buf} = [];
+    @{$self->{wbuf}} = ();
 
     # if we're using epoll, we have to remove this from our epoll fd so we stop getting
     # notifications about it
@@ -612,12 +610,12 @@ sub write {
 
     # just queue data if there's already a wait
     my $need_queue;
+    my $wbuf = $self->{wbuf};
 
     if (defined $data) {
         $bref = ref $data ? $data : \$data;
-        if ($self->{write_buf_size}) {
-            push @{$self->{write_buf}}, $bref;
-            $self->{write_buf_size} += ref $bref eq "SCALAR" ? length($$bref) : 1;
+        if (scalar @$wbuf) {
+            push @$wbuf, $bref;
             return 0;
         }
 
@@ -629,7 +627,7 @@ sub write {
 
   WRITE:
     while (1) {
-        return 1 unless $bref ||= $self->{write_buf}[0];
+        return 1 unless $bref ||= $wbuf->[0];
 
         my $len;
         eval {
@@ -638,8 +636,7 @@ sub write {
         if ($@) {
             if (UNIVERSAL::isa($bref, "CODE")) {
                 unless ($need_queue) {
-                    $self->{write_buf_size}--; # code refs are worth 1
-                    shift @{$self->{write_buf}};
+                    shift @$wbuf;
                 }
                 $bref->();
 
@@ -654,9 +651,9 @@ sub write {
             die "Write error: $@ <$bref>";
         }
 
-        my $to_write = $len - $self->{write_buf_offset};
+        my $to_write = $len - $self->{wbuf_off};
         my $written = syswrite($self->{sock}, $$bref, $to_write,
-                               $self->{write_buf_offset});
+                               $self->{wbuf_off});
 
         if (! defined $written) {
             if ($! == EPIPE) {
@@ -665,8 +662,7 @@ sub write {
                 # since connection has stuff to write, it should now be
                 # interested in pending writes:
                 if ($need_queue) {
-                    push @{$self->{write_buf}}, $bref;
-                    $self->{write_buf_size} += $len;
+                    push @$wbuf, $bref;
                 }
                 $self->watch_write(1);
                 return 0;
@@ -681,19 +677,17 @@ sub write {
             DebugLevel >= 2 && $self->debugmsg("Wrote PARTIAL %d bytes to %d",
                                                $written, $self->{fd});
             if ($need_queue) {
-                push @{$self->{write_buf}}, $bref;
-                $self->{write_buf_size} += $len;
+                push @$wbuf, $bref;
             }
             # since connection has stuff to write, it should now be
             # interested in pending writes:
-            $self->{write_buf_offset} += $written;
-            $self->{write_buf_size} -= $written;
+            $self->{wbuf_off} += $written;
             $self->on_incomplete_write;
             return 0;
         } elsif ($written == $to_write) {
             DebugLevel >= 2 && $self->debugmsg("Wrote ALL %d bytes to %d (nq=%d)",
                                                $written, $self->{fd}, $need_queue);
-            $self->{write_buf_offset} = 0;
+            $self->{wbuf_off} = 0;
             $self->watch_write(0);
 
             # this was our only write, so we can return immediately
@@ -702,8 +696,7 @@ sub write {
             # can't be anything else to write.
             return 1 if $need_queue;
 
-            $self->{write_buf_size} -= $written;
-            shift @{$self->{write_buf}};
+            shift @$wbuf;
             undef $bref;
             next WRITE;
         }

diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index 977614b489d80143c75ebe366513194e5d4977cb..fd103251588730dd63186515a2177a2396cec65e 100644 (file)
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -260,7 +260,7 @@ sub getline_cb ($$$) {
                        $write->($buf); # may close in PublicInbox::DS::write
                        unless ($self->{closed}) {
                                my $next = $self->{pull};
-                               if ($self->{write_buf_size}) {
+                               if (scalar @{$self->{wbuf}}) {
                                        $self->write($next);
                                } else {
                                        PublicInbox::EvCleanup::asap($next);
@@ -315,7 +315,7 @@ use constant MSG_MORE => ($^O eq 'linux') ? 0x8000 : 0;
 sub more ($$) {
        my $self = $_[0];
        return if $self->{closed};
-       if (MSG_MORE && !$self->{write_buf_size}) {
+       if (MSG_MORE && !scalar(@{$self->{wbuf}})) {
                my $n = send($self->{sock}, $_[1], MSG_MORE);
                if (defined $n) {
                        my $nlen = length($_[1]) - $n;
@@ -487,7 +487,7 @@ sub close {
 # for graceful shutdown in PublicInbox::Daemon:
 sub busy () {
        my ($self) = @_;
-       ($self->{rbuf} ne '' || $self->{env} || $self->{write_buf_size});
+       ($self->{rbuf} ne '' || $self->{env} || scalar(@{$self->{wbuf}}));
 }
 
 1;

diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm
index dbe8a84abbbd3291116e816a919073d7422c01cb..60701085d07d3e71764fc99d0cd93664fabafb17 100644 (file)
--- a/lib/PublicInbox/HTTPD/Async.pm
+++ b/lib/PublicInbox/HTTPD/Async.pm
@@ -46,7 +46,7 @@ sub main_cb ($$$) {
                if ($r) {
                        $fh->write($$bref);
                        unless ($http->{closed}) { # PublicInbox::DS sets this
-                               if ($http->{write_buf_size}) {
+                               if (scalar @{$http->{wbuf}}) {
                                        $self->watch_read(0);
                                        $http->write(restart_read_cb($self));
                                }

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index be80560ffa4a965a02e597796790ad7082d08f4d..b62c2187dc01bb19c7e21db703f2c388748097db 100644 (file)
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -619,7 +619,7 @@ sub long_response ($$) {
                                update_idle_time($self);
                                check_read($self);
                        }
-               } elsif ($more) { # $self->{write_buf_size}:
+               } elsif ($more) { # scalar @{$self->{wbuf}}:
                        # no recursion, schedule another call ASAP
                        # but only after all pending writes are done
                        update_idle_time($self);
@@ -925,7 +925,7 @@ use constant MSG_MORE => ($^O eq 'linux') ? 0x8000 : 0;
 
 sub do_more ($$) {
        my ($self, $data) = @_;
-       if (MSG_MORE && !$self->{write_buf_size}) {
+       if (MSG_MORE && !scalar(@{$self->{wbuf}})) {
                my $n = send($self->{sock}, $data, MSG_MORE);
                if (defined $n) {
                        my $dlen = length($data);
@@ -1004,8 +1004,8 @@ sub not_idle_long ($$) {
 # for graceful shutdown in PublicInbox::Daemon:
 sub busy {
        my ($self, $now) = @_;
-       ($self->{rbuf} ne '' || $self->{long_res} || $self->{write_buf_size} ||
-        not_idle_long($self, $now));
+       ($self->{rbuf} ne '' || $self->{long_res} ||
+               scalar(@{$self->{wbuf}}) || not_idle_long($self, $now));
 }
 
 1;