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-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,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 828A41F915 for ; Sat, 27 Jun 2020 10:04:02 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 12/34] ds: remove fields.pm usage Date: Sat, 27 Jun 2020 10:03:38 +0000 Message-Id: <20200627100400.9871-13-e@yhbt.net> In-Reply-To: <20200627100400.9871-1-e@yhbt.net> References: <20200627100400.9871-1-e@yhbt.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: Since the removal of pseudo-hash support in Perl 5.10, the "fields" module no longer provides the space or speed benefits it did in 5.8. It also does not allow for compile-time checks, only run-time checks. To me, the extra developer overhead in maintaining "use fields" args has become a hassle. None of our non-DS-related code uses fields.pm, nor do any of our current dependencies. In fact, Danga::Socket (which DS was originally forked from) and its subclasses are the only fields.pm users I've ever encountered in the wild. Removing fields may make our code more approachable to other Perl hackers. So stop using fields.pm and locked hashes, but continue to document what fields do for non-trivial classes. --- lib/PublicInbox/DS.pm | 17 ++++++----------- lib/PublicInbox/DirIdle.pm | 5 ++--- lib/PublicInbox/GitAsyncCat.pm | 4 +--- lib/PublicInbox/HTTP.pm | 23 +++++++++++++++-------- lib/PublicInbox/HTTPD/Async.pm | 22 +++++++++++++--------- lib/PublicInbox/IMAP.pm | 19 +++++++++++-------- lib/PublicInbox/InboxIdle.pm | 9 ++++++--- lib/PublicInbox/Listener.pm | 8 ++------ lib/PublicInbox/NNTP.pm | 12 +++++++----- lib/PublicInbox/NNTPdeflate.pm | 5 +---- lib/PublicInbox/ParentPipe.pm | 8 ++------ lib/PublicInbox/Sigfd.pm | 9 +++++---- xt/mem-imapd-tls.t | 18 +++++++----------- 13 files changed, 78 insertions(+), 81 deletions(-) diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm index aa65b2d3642..da68802dda9 100644 --- a/lib/PublicInbox/DS.pm +++ b/lib/PublicInbox/DS.pm @@ -13,6 +13,12 @@ # Bugs encountered were reported to bug-Danga-Socket@rt.cpan.org, # fixed in Danga::Socket 1.62 and visible at: # https://rt.cpan.org/Public/Dist/Display.html?Name=Danga-Socket +# +# fields: +# sock: underlying socket +# rbuf: scalarref, usually undef +# wbuf: arrayref of coderefs or tmpio (autovivified)) +# (tmpio = [ GLOB, offset, [ length ] ]) package PublicInbox::DS; use strict; use bytes; @@ -22,19 +28,10 @@ use Fcntl qw(SEEK_SET :DEFAULT O_APPEND); use Time::HiRes qw(clock_gettime CLOCK_MONOTONIC); use parent qw(Exporter); our @EXPORT_OK = qw(now msg_more); -use warnings; use 5.010_001; use Scalar::Util qw(blessed); - use PublicInbox::Syscall qw(:epoll); use PublicInbox::Tmpfile; - -use fields ('sock', # underlying socket - 'rbuf', # scalarref, usually undef - 'wbuf', # arrayref of coderefs or tmpio (autovivified)) - # (tmpio = [ GLOB, offset, [ length ] ]) - ); - use Errno qw(EAGAIN EINVAL); use Carp qw(confess carp); @@ -328,8 +325,6 @@ This is normally (always?) called from your subclass via: =cut sub new { my ($self, $sock, $ev) = @_; - $self = fields::new($self) unless ref $self; - $self->{sock} = $sock; my $fd = fileno($sock); diff --git a/lib/PublicInbox/DirIdle.pm b/lib/PublicInbox/DirIdle.pm index ffceda66530..fbbc9531a20 100644 --- a/lib/PublicInbox/DirIdle.pm +++ b/lib/PublicInbox/DirIdle.pm @@ -4,8 +4,7 @@ # Used by public-inbox-watch for Maildir (and possibly MH in the future) package PublicInbox::DirIdle; use strict; -use base 'PublicInbox::DS'; -use fields qw(inot); +use parent 'PublicInbox::DS'; use PublicInbox::Syscall qw(EPOLLIN EPOLLET); use PublicInbox::In2Tie; @@ -24,7 +23,7 @@ if ($^O eq 'linux' && eval { require Linux::Inotify2; 1 }) { sub new { my ($class, $dirs, $cb) = @_; - my $self = fields::new($class); + my $self = bless {}, $class; my $inot; if ($ino_cls) { $inot = $ino_cls->new or die "E: $ino_cls->new: $!"; diff --git a/lib/PublicInbox/GitAsyncCat.pm b/lib/PublicInbox/GitAsyncCat.pm index 098101aed00..0b777204a7c 100644 --- a/lib/PublicInbox/GitAsyncCat.pm +++ b/lib/PublicInbox/GitAsyncCat.pm @@ -11,16 +11,14 @@ package PublicInbox::GitAsyncCat; use strict; use parent qw(PublicInbox::DS Exporter); -use fields qw(git); use PublicInbox::Syscall qw(EPOLLIN EPOLLET); our @EXPORT = qw(git_async_cat); sub _add { my ($class, $git) = @_; - my $self = fields::new($class); $git->batch_prepare; + my $self = bless { git => $git }, $class; $self->SUPER::new($git->{in}, EPOLLIN|EPOLLET); - $self->{git} = $git; \undef; # this is a true ref() } diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm index 6ccf2059240..8281746538e 100644 --- a/lib/PublicInbox/HTTP.pm +++ b/lib/PublicInbox/HTTP.pm @@ -6,12 +6,21 @@ # to learn different ways to admin both NNTP and HTTP components. # There's nothing which depends on public-inbox, here. # Each instance of this class represents a HTTP client socket - +# +# fields: +# httpd: PublicInbox::HTTPD ref +# env: PSGI env hashref +# input_left: bytes left to read in request body (e.g. POST/PUT) +# remote_addr: remote IP address as a string (e.g. "127.0.0.1") +# remote_port: peer port +# forward: response body object, response to ->getline + ->close +# alive: HTTP keepalive state: +# 0: drop connection when done +# 1: keep connection when done +# 2: keep connection, chunk responses package PublicInbox::HTTP; use strict; -use warnings; -use base qw(PublicInbox::DS); -use fields qw(httpd env input_left remote_addr remote_port forward alive); +use parent qw(PublicInbox::DS); use bytes (); # only for bytes::length use Fcntl qw(:seek); use Plack::HTTPParser qw(parse_http_request); # XS or pure Perl @@ -56,7 +65,7 @@ sub http_date () { sub new ($$$) { my ($class, $sock, $addr, $httpd) = @_; - my $self = fields::new($class); + my $self = bless { httpd => $httpd }, $class; my $ev = EPOLLIN; my $wbuf; if ($sock->can('accept_SSL') && !$sock->accept_SSL) { @@ -64,12 +73,10 @@ sub new ($$$) { $ev = PublicInbox::TLS::epollbit(); $wbuf = [ \&PublicInbox::DS::accept_tls_step ]; } - $self->SUPER::new($sock, $ev | EPOLLONESHOT); - $self->{httpd} = $httpd; $self->{wbuf} = $wbuf if $wbuf; ($self->{remote_addr}, $self->{remote_port}) = PublicInbox::Daemon::host_with_port($addr); - $self; + $self->SUPER::new($sock, $ev | EPOLLONESHOT); } sub event_step { # called by PublicInbox::DS diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm index 35075d344b0..87a6a5f9cf0 100644 --- a/lib/PublicInbox/HTTPD/Async.pm +++ b/lib/PublicInbox/HTTPD/Async.pm @@ -6,11 +6,16 @@ # The name of this key is not even stable! # Currently intended for use with read-only pipes with expensive # processes such as git-http-backend(1), cgit(1) +# +# fields: +# http: PublicInbox::HTTP ref +# fh: PublicInbox::HTTP::{Identity,Chunked} ref (can ->write + ->close) +# cb: initial read callback +# arg: arg for {cb} +# end_obj: CODE or object which responds to ->event_step when ->close is called package PublicInbox::HTTPD::Async; use strict; -use warnings; -use base qw(PublicInbox::DS); -use fields qw(http fh cb arg end_obj); +use parent qw(PublicInbox::DS); use Errno qw(EAGAIN); use PublicInbox::Syscall qw(EPOLLIN EPOLLET); @@ -27,14 +32,13 @@ sub new { die '$end_obj unsupported w/o $io' if $end_obj; return; } - - my $self = fields::new($class); + my $self = bless { + cb => $cb, # initial read callback + arg => $arg, # arg for $cb + end_obj => $end_obj, # like END{}, can ->event_step + }, $class; IO::Handle::blocking($io, 0); $self->SUPER::new($io, EPOLLIN | EPOLLET); - $self->{cb} = $cb; # initial read callback - $self->{arg} = $arg; # arg for $cb - $self->{end_obj} = $end_obj; # like END{}, can ->event_step - $self; } sub event_step { diff --git a/lib/PublicInbox/IMAP.pm b/lib/PublicInbox/IMAP.pm index 0a6993c64c4..24f96e6691a 100644 --- a/lib/PublicInbox/IMAP.pm +++ b/lib/PublicInbox/IMAP.pm @@ -21,12 +21,18 @@ # as a 50K uint16_t array (via pack("S*", ...)). "UID offset" # is the offset from {uid_base} which determines the start of # the mailbox slice. - +# +# fields: +# imapd: PublicInbox::IMAPD ref +# ibx: PublicInbox::Inbox ref +# long_cb: long_response private data +# uid_base: base UID for mailbox slice (0-based) +# -login_tag: IMAP TAG for LOGIN +# -idle_tag: IMAP response tag for IDLE +# uo2m: UID-to-MSN mapping package PublicInbox::IMAP; use strict; -use base qw(PublicInbox::DS); -use fields qw(imapd ibx long_cb -login_tag - uid_base -idle_tag uo2m); +use parent qw(PublicInbox::DS); use PublicInbox::Eml; use PublicInbox::EmlContentFoo qw(parse_content_disposition); use PublicInbox::DS qw(now); @@ -34,7 +40,6 @@ use PublicInbox::Syscall qw(EPOLLIN EPOLLONESHOT); use PublicInbox::GitAsyncCat; use Text::ParseWords qw(parse_line); use Errno qw(EAGAIN); -use Hash::Util qw(unlock_hash); # dependency of fields for perl 5.10+, anyways use PublicInbox::Search; use PublicInbox::IMAPsearchqp; *mdocid = \&PublicInbox::Search::mdocid; @@ -107,8 +112,7 @@ sub greet ($) { sub new ($$$) { my ($class, $sock, $imapd) = @_; - my $self = fields::new('PublicInbox::IMAP_preauth'); - unlock_hash(%$self); + my $self = bless { imapd => $imapd }, 'PublicInbox::IMAP_preauth'; my $ev = EPOLLIN; my $wbuf; if ($sock->can('accept_SSL') && !$sock->accept_SSL) { @@ -117,7 +121,6 @@ sub new ($$$) { $wbuf = [ \&PublicInbox::DS::accept_tls_step, \&greet ]; } $self->SUPER::new($sock, $ev | EPOLLONESHOT); - $self->{imapd} = $imapd; if ($wbuf) { $self->{wbuf} = $wbuf; } else { diff --git a/lib/PublicInbox/InboxIdle.pm b/lib/PublicInbox/InboxIdle.pm index ba8200aef05..59cb833fd5a 100644 --- a/lib/PublicInbox/InboxIdle.pm +++ b/lib/PublicInbox/InboxIdle.pm @@ -1,10 +1,13 @@ # Copyright (C) 2020 all contributors # License: AGPL-3.0+ +# fields: +# pi_config: PublicInbox::Config ref +# inot: Linux::Inotify2-like object +# pathmap => { inboxdir => [ ibx, watch1, watch2, watch3... ] } mapping package PublicInbox::InboxIdle; use strict; -use base qw(PublicInbox::DS); -use fields qw(pi_config inot pathmap); +use parent qw(PublicInbox::DS); use Cwd qw(abs_path); use PublicInbox::Syscall qw(EPOLLIN EPOLLET); my $IN_MODIFY = 0x02; # match Linux inotify @@ -50,7 +53,7 @@ sub refresh { sub new { my ($class, $pi_config) = @_; - my $self = fields::new($class); + my $self = bless {}, $class; my $inot; if ($ino_cls) { $inot = $ino_cls->new or die "E: $ino_cls->new: $!"; diff --git a/lib/PublicInbox/Listener.pm b/lib/PublicInbox/Listener.pm index eb7dd8d46cc..2e0fc248fe7 100644 --- a/lib/PublicInbox/Listener.pm +++ b/lib/PublicInbox/Listener.pm @@ -4,10 +4,8 @@ # Used by -nntpd for listen sockets package PublicInbox::Listener; use strict; -use warnings; -use base 'PublicInbox::DS'; +use parent 'PublicInbox::DS'; use Socket qw(SOL_SOCKET SO_KEEPALIVE IPPROTO_TCP TCP_NODELAY); -use fields qw(post_accept); use IO::Handle; use PublicInbox::Syscall qw(EPOLLIN EPOLLEXCLUSIVE EPOLLET); use Errno qw(EAGAIN ECONNABORTED EPERM); @@ -23,10 +21,8 @@ sub new ($$$) { setsockopt($s, SOL_SOCKET, SO_KEEPALIVE, 1); setsockopt($s, IPPROTO_TCP, TCP_NODELAY, 1); # ignore errors on non-TCP listen($s, 1024); - my $self = fields::new($class); + my $self = bless { post_accept => $cb }, $class; $self->SUPER::new($s, EPOLLIN|EPOLLET|EPOLLEXCLUSIVE); - $self->{post_accept} = $cb; - $self } sub event_step { diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm index 76f14bbd97d..9d91544abd3 100644 --- a/lib/PublicInbox/NNTP.pm +++ b/lib/PublicInbox/NNTP.pm @@ -2,11 +2,14 @@ # License: AGPL-3.0+ # # Each instance of this represents a NNTP client socket +# fields: +# nntpd: PublicInbox::NNTPD ref +# article: per-session current article number +# ng: PublicInbox::Inbox ref +# long_cb: long_response private data package PublicInbox::NNTP; use strict; -use warnings; -use base qw(PublicInbox::DS); -use fields qw(nntpd article ng long_cb); +use parent qw(PublicInbox::DS); use PublicInbox::MID qw(mid_escape $MID_EXTRACT); use PublicInbox::Eml; use POSIX qw(strftime); @@ -45,7 +48,7 @@ sub greet ($) { $_[0]->write($_[0]->{nntpd}->{greet}) }; sub new ($$$) { my ($class, $sock, $nntpd) = @_; - my $self = fields::new($class); + my $self = bless { nntpd => $nntpd }, $class; my $ev = EPOLLIN; my $wbuf; if ($sock->can('accept_SSL') && !$sock->accept_SSL) { @@ -54,7 +57,6 @@ sub new ($$$) { $wbuf = [ \&PublicInbox::DS::accept_tls_step, \&greet ]; } $self->SUPER::new($sock, $ev | EPOLLONESHOT); - $self->{nntpd} = $nntpd; if ($wbuf) { $self->{wbuf} = $wbuf; } else { diff --git a/lib/PublicInbox/NNTPdeflate.pm b/lib/PublicInbox/NNTPdeflate.pm index dec88aba3a5..02af935f0df 100644 --- a/lib/PublicInbox/NNTPdeflate.pm +++ b/lib/PublicInbox/NNTPdeflate.pm @@ -16,11 +16,9 @@ # efficient in terms of server memory usage. package PublicInbox::NNTPdeflate; use strict; -use warnings; use 5.010_001; -use base qw(PublicInbox::NNTP); +use parent qw(PublicInbox::NNTP); use Compress::Raw::Zlib; -use Hash::Util qw(unlock_hash); # dependency of fields for perl 5.10+, anyways my %IN_OPT = ( -Bufsize => PublicInbox::NNTP::LINE_MAX, @@ -53,7 +51,6 @@ sub enable { $self->res('403 Unable to activate compression'); return; } - unlock_hash(%$self); $self->res('206 Compression active'); bless $self, $class; $self->{zin} = $in; diff --git a/lib/PublicInbox/ParentPipe.pm b/lib/PublicInbox/ParentPipe.pm index f62f011bbe3..538b5632c62 100644 --- a/lib/PublicInbox/ParentPipe.pm +++ b/lib/PublicInbox/ParentPipe.pm @@ -5,17 +5,13 @@ # notified if the master process dies. package PublicInbox::ParentPipe; use strict; -use warnings; -use base qw(PublicInbox::DS); -use fields qw(cb); +use parent qw(PublicInbox::DS); use PublicInbox::Syscall qw(EPOLLIN EPOLLONESHOT); sub new ($$$) { my ($class, $pipe, $worker_quit) = @_; - my $self = fields::new($class); + my $self = bless { cb => $worker_quit }, $class; $self->SUPER::new($pipe, EPOLLIN|EPOLLONESHOT); - $self->{cb} = $worker_quit; - $self; } # master process died, time to call worker_quit ourselves diff --git a/lib/PublicInbox/Sigfd.pm b/lib/PublicInbox/Sigfd.pm index 17456592a7e..bf91bb3774f 100644 --- a/lib/PublicInbox/Sigfd.pm +++ b/lib/PublicInbox/Sigfd.pm @@ -1,9 +1,11 @@ # Copyright (C) 2019-2020 all contributors # License: AGPL-3.0+ + +# Wraps a signalfd (or similar) for PublicInbox::DS +# fields: (sig: hashref similar to %SIG, but signal numbers as keys) package PublicInbox::Sigfd; use strict; use parent qw(PublicInbox::DS); -use fields qw(sig); # hashref similar to %SIG, but signal numbers as keys use PublicInbox::Syscall qw(signalfd EPOLLIN EPOLLET SFD_NONBLOCK); use POSIX qw(:signal_h); use IO::Handle (); @@ -12,7 +14,6 @@ use IO::Handle (); # are available. sub new { my ($class, $sig, $flags) = @_; - my $self = fields::new($class); my %signo = map {; my $cb = $sig->{$_}; # SIGWINCH is 28 on FreeBSD, NetBSD, OpenBSD @@ -22,6 +23,7 @@ sub new { }; $num => $cb; } keys %$sig; + my $self = bless { sig => \%signo }, $class; my $io; my $fd = signalfd(-1, [keys %signo], $flags); if (defined $fd && $fd >= 0) { @@ -35,9 +37,8 @@ sub new { $self->SUPER::new($io, EPOLLIN | EPOLLET); } else { # master main loop $self->{sock} = $io; + $self; } - $self->{sig} = \%signo; - $self; } # PublicInbox::Daemon in master main loop (blocking) diff --git a/xt/mem-imapd-tls.t b/xt/mem-imapd-tls.t index 97e67d3029a..660fdc77a2b 100644 --- a/xt/mem-imapd-tls.t +++ b/xt/mem-imapd-tls.t @@ -132,8 +132,8 @@ done_testing; package IMAPC; use strict; -use base qw(PublicInbox::DS); -use fields qw(step zin); +use parent qw(PublicInbox::DS); +# fields: step: state machine, zin: Zlib inflate context use PublicInbox::Syscall qw(EPOLLIN EPOLLOUT EPOLLONESHOT); use Errno qw(EAGAIN); # determines where we start event_step @@ -207,26 +207,23 @@ sub event_step { sub new { my ($class, $io) = @_; - my $self = fields::new($class); - - # wait for connect(), and maybe SSL_connect() - $self->SUPER::new($io, EPOLLOUT|EPOLLONESHOT); + my $self = bless { step => FIRST_STEP }, $class; if ($io->can('connect_SSL')) { $self->{wbuf} = [ \&connect_tls_step ]; } - $self->{step} = FIRST_STEP; - $self; + # wait for connect(), and maybe SSL_connect() + $self->SUPER::new($io, EPOLLOUT|EPOLLONESHOT); } 1; package IMAPCdeflate; use strict; -use base qw(IMAPC); # parent doesn't work for fields -use Hash::Util qw(unlock_hash); # dependency of fields for perl 5.10+, anyways +our @ISA; use Compress::Raw::Zlib; use PublicInbox::IMAPdeflate; my %ZIN_OPT; BEGIN { + @ISA = qw(IMAPC); %ZIN_OPT = ( -WindowBits => -15, -AppendOutput => 1 ); *write = \&PublicInbox::IMAPdeflate::write; *do_read = \&PublicInbox::IMAPdeflate::do_read; @@ -236,7 +233,6 @@ sub enable { my ($class, $self) = @_; my ($in, $err) = Compress::Raw::Zlib::Inflate->new(%ZIN_OPT); die "Inflate->new failed: $err" if $err != Z_OK; - unlock_hash(%$self); bless $self, $class; $self->{zin} = $in; }