[PATCH 4/9] ds: remove {fd} field

[Eric Wong <e@80x24.org>]

Storing the file descriptor was redundant as we can quickly call
fileno($self->{sock}) and not have to store an extra hash table
entry.  Multiple sources of truth leads to confusion, confusion
leads to bugs.
---
 lib/PublicInbox/DS.pm   | 42 +++++++++++++++--------------------------
 lib/PublicInbox/NNTP.pm | 10 ++++++----
 2 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index e2aa4b55..7d7baac7 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -26,7 +26,6 @@ use warnings;
 use PublicInbox::Syscall qw(:epoll);
 
 use fields ('sock',              # underlying socket
-            'fd',                # numeric file descriptor
             '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
@@ -440,13 +439,12 @@ sub new {
     my ($self, $sock, $exclusive) = @_;
     $self = fields::new($self) unless ref $self;
 
-    $self->{sock}        = $sock;
+    $self->{sock} = $sock;
     my $fd = fileno($sock);
 
     Carp::cluck("undef sock and/or fd in PublicInbox::DS->new.  sock=" . ($sock || "") . ", fd=" . ($fd || ""))
         unless $sock && $fd;
 
-    $self->{fd}          = $fd;
     $self->{wbuf} = [];
     $self->{wbuf_off} = 0;
     $self->{closed} = 0;
@@ -523,9 +521,8 @@ sub close {
 
     # defer closing the actual socket until the event loop is done
     # processing this round of events.  (otherwise we might reuse fds)
-    if ($self->{sock}) {
-        push @ToClose, $self->{sock};
-        $self->{sock} = undef;
+    if (my $sock = delete $self->{sock}) {
+        push @ToClose, $sock;
     }
 
     return 0;
@@ -546,11 +543,10 @@ sub _cleanup {
 
     # if we're using epoll, we have to remove this from our epoll fd so we stop getting
     # notifications about it
-    if ($HaveEpoll && $self->{fd}) {
-        if (epoll_ctl($Epoll, EPOLL_CTL_DEL, $self->{fd}, $self->{event_watch}) != 0) {
-            # dump_error prints a backtrace so we can try to figure out why this happened
-            $self->dump_error("epoll_ctl(): failure deleting fd=$self->{fd} during _cleanup(); $! (" . ($!+0) . ")");
-        }
+    if ($HaveEpoll && $self->{sock}) {
+        my $fd = fileno($self->{sock});
+        epoll_ctl($Epoll, EPOLL_CTL_DEL, $fd, $self->{event_watch}) and
+            confess("EPOLL_CTL_DEL: $!");
     }
 
     # we explicitly don't delete from DescriptorMap here until we
@@ -561,9 +557,6 @@ sub _cleanup {
     # looked at $pob->{closed} and ignore it.  but if it's an
     # un-accounted for fd, then it (understandably) freak out a bit
     # and emit warnings, thinking their state got off.
-
-    # and finally get rid of our fd so we can't use it anywhere else
-    $self->{fd} = undef;
 }
 
 =head2 C<< $obj->sock() >>
@@ -660,8 +653,6 @@ sub write {
 
             return $self->close;
         } elsif ($written != $to_write) {
-            DebugLevel >= 2 && $self->debugmsg("Wrote PARTIAL %d bytes to %d",
-                                               $written, $self->{fd});
             if ($need_queue) {
                 push @$wbuf, $bref;
             }
@@ -671,8 +662,6 @@ sub write {
             $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->{wbuf_off} = 0;
             $self->watch_write(0);
 
@@ -718,7 +707,6 @@ sub read {
 
     if (! $res && $! != EAGAIN) {
         # catches 0=conn closed or undef=error
-        DebugLevel >= 2 && $self->debugmsg("Fd \#%d read hit the end of the road.", $self->{fd});
         return undef;
     }
 
@@ -779,16 +767,16 @@ sub watch_read {
     $event &= ~POLLIN if ! $val;
     $event |=  POLLIN if   $val;
 
+    my $fd = fileno($self->{sock});
     # If it changed, set it
     if ($event != $self->{event_watch}) {
         if ($HaveKQueue) {
-            $KQueue->EV_SET($self->{fd}, IO::KQueue::EVFILT_READ(),
+            $KQueue->EV_SET($fd, IO::KQueue::EVFILT_READ(),
                             $val ? IO::KQueue::EV_ENABLE() : IO::KQueue::EV_DISABLE());
         }
         elsif ($HaveEpoll) {
-            epoll_ctl($Epoll, EPOLL_CTL_MOD, $self->{fd}, $event)
-                and $self->dump_error("couldn't modify epoll settings for $self->{fd} " .
-                                      "from $self->{event_watch} -> $event: $! (" . ($!+0) . ")");
+            epoll_ctl($Epoll, EPOLL_CTL_MOD, $fd, $event) and
+                confess("EPOLL_CTL_MOD: $!");
         }
         $self->{event_watch} = $event;
     }
@@ -808,17 +796,17 @@ sub watch_write {
 
     $event &= ~POLLOUT if ! $val;
     $event |=  POLLOUT if   $val;
+    my $fd = fileno($self->{sock});
 
     # If it changed, set it
     if ($event != $self->{event_watch}) {
         if ($HaveKQueue) {
-            $KQueue->EV_SET($self->{fd}, IO::KQueue::EVFILT_WRITE(),
+            $KQueue->EV_SET($fd, IO::KQueue::EVFILT_WRITE(),
                             $val ? IO::KQueue::EV_ENABLE() : IO::KQueue::EV_DISABLE());
         }
         elsif ($HaveEpoll) {
-            epoll_ctl($Epoll, EPOLL_CTL_MOD, $self->{fd}, $event)
-                and $self->dump_error("couldn't modify epoll settings for $self->{fd} " .
-                                      "from $self->{event_watch} -> $event: $! (" . ($!+0) . ")");
+            epoll_ctl($Epoll, EPOLL_CTL_MOD, $fd, $event) and
+                    confess "EPOLL_CTL_MOD: $!";
         }
         $self->{event_watch} = $event;
     }
diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index b62c2187..7729399a 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -66,7 +66,8 @@ sub next_tick () {
 
 sub update_idle_time ($) {
 	my ($self) = @_;
-	my $fd = $self->{fd};
+        my $sock = $self->{sock} or return;
+	my $fd = fileno($sock);
 	defined $fd and $EXPMAP->{$fd} = [ now(), $self ];
 }
 
@@ -595,7 +596,7 @@ sub long_response ($$) {
 	my ($self, $cb) = @_;
 	die "BUG: nested long response" if $self->{long_res};
 
-	my $fd = $self->{fd};
+	my $fd = fileno($self->{sock});
 	defined $fd or return;
 	# make sure we disable reading during a long response,
 	# clients should not be sending us stuff and making us do more
@@ -963,7 +964,7 @@ sub event_read {
 		my $line = $1;
 		return $self->close if $line =~ /[[:cntrl:]]/s;
 		my $t0 = now();
-		my $fd = $self->{fd};
+		my $fd = fileno($self->{sock});
 		$r = eval { process_line($self, $line) };
 		my $d = $self->{long_res} ?
 			" deferred[$fd]" : '';
@@ -995,7 +996,8 @@ sub check_read {
 
 sub not_idle_long ($$) {
 	my ($self, $now) = @_;
-	defined(my $fd = $self->{fd}) or return;
+        my $sock = $self->{sock} or return;
+	defined(my $fd = fileno($sock)) or return;
 	my $ary = $EXPMAP->{$fd} or return;
 	my $exp_at = $ary->[0] + $EXPTIME;
 	$exp_at > $now;
-- 
EW