From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.2 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, T_SCC_BODY_TEXT_LINE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id B23341F51A for ; Tue, 31 Oct 2023 20:42:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=80x24.org; s=selector1; t=1698784977; bh=8ExzmFMbLrRGFUKkxabdsbPsuTuGbetP8SysK2ssN1s=; h=From:To:Subject:Date:In-Reply-To:References:From; b=yfBBoCOw0n1brYVlzEf3IXZrOwlu8ZCYHZXsg1xXcvi2bvFxGfk1VNtUgjJDLBcIu 1NKSu6bLY2hW1/H4o6IbPVtiZdHwV1/vyh2AwaGZy7EYE8v1sJLHW991Kx7J4I3wta wNpEXoM62CbMlEW6ahoFIgJUYRWSTno493pLH+ks= From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 6/6] ds: make ->close behave like CORE::close Date: Tue, 31 Oct 2023 20:42:55 +0000 Message-Id: <20231031204255.1837649-7-e@80x24.org> In-Reply-To: <20231031204255.1837649-1-e@80x24.org> References: <20231031204255.1837649-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: Matching existing Perl IO semantics seems like a good idea to reduce confusion in the future. We'll also fix some outdated comments and update indentation to match the rest of our code base since we're far detached from Danga::Socket at this point. --- lib/PublicInbox/CidxComm.pm | 2 +- lib/PublicInbox/CidxLogP.pm | 2 +- lib/PublicInbox/DS.pm | 84 ++++++++++++++----------------------- 3 files changed, 33 insertions(+), 55 deletions(-) diff --git a/lib/PublicInbox/CidxComm.pm b/lib/PublicInbox/CidxComm.pm index fb7be0aa..c7ab3c10 100644 --- a/lib/PublicInbox/CidxComm.pm +++ b/lib/PublicInbox/CidxComm.pm @@ -21,7 +21,7 @@ sub new { sub event_step { my ($self) = @_; my $rd = $self->{sock} // return warn('BUG?: no {sock}'); - $self->close; # PublicInbox::DS::close, deferred, so $sock is usable + $self->close; # EPOLL_CTL_DEL delete($self->{cidx})->cidx_read_comm($rd); } diff --git a/lib/PublicInbox/CidxLogP.pm b/lib/PublicInbox/CidxLogP.pm index ac4c1b37..5ea675bf 100644 --- a/lib/PublicInbox/CidxLogP.pm +++ b/lib/PublicInbox/CidxLogP.pm @@ -21,7 +21,7 @@ sub new { sub event_step { my ($self) = @_; my $rd = $self->{sock} // return warn('BUG?: no {sock}'); - $self->close; # PublicInbox::DS::close, deferred, so $sock is usable + $self->close; # EPOLL_CTL_DEL delete($self->{cidx})->cidx_read_log_p($self, $rd); } diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm index 9e1f66c2..ca1f0f81 100644 --- a/lib/PublicInbox/DS.pm +++ b/lib/PublicInbox/DS.pm @@ -354,37 +354,21 @@ sub greet { $self; } -##################################################################### -### I N S T A N C E M E T H O D S -##################################################################### - sub requeue ($) { push @$nextq, $_[0] } # autovivifies -=head2 C<< $obj->close >> - -Close the socket. - -=cut +# drop the IO::Handle ref, true if successful, false if not (or already dropped) +# (this is closer to CORE::close than Danga::Socket::close) sub close { - my ($self) = @_; - my $sock = delete $self->{sock} or return; - - # we need to flush our write buffer, as there may - # be self-referential closures (sub { $client->close }) - # preventing the object from being destroyed - delete $self->{wbuf}; - - delete $DescriptorMap{fileno($sock)}; + my ($self) = @_; + my $sock = delete $self->{sock} or return; - # if we're using epoll, we have to remove this from our epoll fd so we - # stop getting notifications about it - $Poller->ep_del($sock) and croak("EPOLL_CTL_DEL($self/$sock): $!"); - # let refcount drop to zero.. + # we need to clear our write buffer, as there may + # be self-referential closures (sub { $client->close }) + # preventing the object from being destroyed + delete $self->{wbuf}; + delete $DescriptorMap{fileno($sock)}; - # FIXME this is the opposite of CORE::close return value - # TODO: callers need to be updated to expect true on success like - # CORE::close (and false otherwise) - 0; + !$Poller->ep_del($sock); # stop getting notifications } # portable, non-thread-safe sendfile emulation (no pread, yet) @@ -430,8 +414,7 @@ next_buf: shift @$wbuf; goto next_buf; } - } elsif ($! == EAGAIN) { - my $ev = epbit($sock, EPOLLOUT) or return $self->close; + } elsif ($! == EAGAIN && (my $ev = epbit($sock, EPOLLOUT))) { epwait($sock, $ev | EPOLLONESHOT); return 0; } else { @@ -461,28 +444,28 @@ sub rbuf_idle ($$) { } } +# returns true if bytes are read, false otherwise sub do_read ($$$;$) { - my ($self, $rbuf, $len, $off) = @_; - my $r = sysread(my $sock = $self->{sock}, $$rbuf, $len, $off // 0); - return ($r == 0 ? $self->close : $r) if defined $r; - # common for clients to break connections without warning, - # would be too noisy to log here: - if ($! == EAGAIN) { - my $ev = epbit($sock, EPOLLIN) or return $self->close; - epwait($sock, $ev | EPOLLONESHOT); - rbuf_idle($self, $rbuf); - 0; - } else { - $self->close; - } + my ($self, $rbuf, $len, $off) = @_; + my ($ev, $r, $s); + $r = sysread($s = $self->{sock}, $$rbuf, $len, $off // 0) and return $r; + + if (!defined($r) && $! == EAGAIN && ($ev = epbit($s, EPOLLIN))) { + epwait($s, $ev | EPOLLONESHOT); + rbuf_idle($self, $rbuf); + } else { + $self->close; + } + undef; } # drop the socket if we hit unrecoverable errors on our system which # require BOFH attention: ENOSPC, EFBIG, EIO, EMFILE, ENFILE... sub drop { - my $self = shift; - carp(@_); - $self->close; + my $self = shift; + carp(@_); + $self->close; + undef; } sub tmpio ($$$) { @@ -603,7 +586,7 @@ sub accept_tls_step ($) { 0; } -# return true if complete, false if incomplete (or failure) +# return value irrelevant sub shutdn_tls_step ($) { my ($self) = @_; my $sock = $self->{sock} or return; @@ -612,19 +595,14 @@ sub shutdn_tls_step ($) { my $ev = PublicInbox::TLS::epollbit() or return $self->close; epwait($sock, $ev | EPOLLONESHOT); unshift(@{$self->{wbuf}}, \&shutdn_tls_step); # autovivifies - 0; } # don't bother with shutdown($sock, 2), we don't fork+exec w/o CLOEXEC # or fork w/o exec, so no inadvertent socket sharing sub shutdn ($) { - my ($self) = @_; - my $sock = $self->{sock} or return; - if ($sock->can('stop_SSL')) { - shutdn_tls_step($self); - } else { - $self->close; - } + my ($self) = @_; + my $sock = $self->{sock} or return; + $sock->can('stop_SSL') ? shutdn_tls_step($self) : $self->close; } sub dflush {} # overridden by DSdeflate