From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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,AWL,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 4911E1F97F for ; Mon, 10 Jun 2019 05:18:47 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 4/9] ds: remove {fd} field Date: Mon, 10 Jun 2019 05:18:41 +0000 Message-Id: <20190610051846.26757-5-e@80x24.org> In-Reply-To: <20190610051846.26757-1-e@80x24.org> References: <20190610051846.26757-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: 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