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,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 2DF7C1F46D for ; Sat, 21 Dec 2019 08:00:08 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 2/8] nntp: remove cyclic refs from long_response Date: Sat, 21 Dec 2019 08:00:01 +0000 Message-Id: <20191221080007.27810-3-e@80x24.org> In-Reply-To: <20191221080007.27810-1-e@80x24.org> References: <20191221080007.27810-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: Leftover cyclic references are a source of memory leaks. While our code is AFAIK unaffected by such leaks at the moment, eliminating a potential source of bugs will make maintenance easier. We make the long_response API cycle-free by stashing the callback into the NNTP object. However, callers will need to be updated to get rid of the circular reference to $self. We do that be replacing anonymous subs with name subroutine references, such as xref_range_i replacing the formerly anonymous sub inside hdr_xref. --- lib/PublicInbox/NNTP.pm | 117 +++++++++++++++++++++------------------- 1 file changed, 61 insertions(+), 56 deletions(-) diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm index cd5d7bba..33c557f5 100644 --- a/lib/PublicInbox/NNTP.pm +++ b/lib/PublicInbox/NNTP.pm @@ -6,7 +6,7 @@ package PublicInbox::NNTP; use strict; use warnings; use base qw(PublicInbox::DS); -use fields qw(nntpd article ng); +use fields qw(nntpd article ng long_cb); use PublicInbox::Search; use PublicInbox::Msgmap; use PublicInbox::MID qw(mid_escape); @@ -596,53 +596,57 @@ sub get_range ($$) { [ \$beg, $end ]; } -sub long_response ($$) { - my ($self, $cb) = @_; # cb returns true if more, false if done +sub long_step { + my ($self) = @_; + # wbuf is unset or empty, here; {long} may add to it + my ($cb, $t0, @args) = @{$self->{long_cb}}; + my $more = eval { $cb->($self, @args) }; + if ($@ || !$self->{sock}) { # something bad happened... + delete $self->{long_cb}; + my $elapsed = now() - $t0; + my $fd = fileno($self->{sock}); + if ($@) { + err($self, + "%s during long response[$fd] - %0.6f", + $@, $elapsed); + } + out($self, " deferred[$fd] aborted - %0.6f", $elapsed); + $self->close; + } elsif ($more) { # $self->{wbuf}: + $self->update_idle_time; + + # COMPRESS users all share the same DEFLATE context. + # Flush it here to ensure clients don't see + # each other's data + $self->zflush; + + # no recursion, schedule another call ASAP + # but only after all pending writes are done + my $wbuf = $self->{wbuf} ||= []; + push @$wbuf, \&long_step; + + # wbuf may be populated by $cb, no need to rearm if so: + $self->requeue if scalar(@$wbuf) == 1; + } else { # all done! + delete $self->{long_cb}; + res($self, '.'); + my $elapsed = now() - $t0; + my $fd = fileno($self->{sock}); + out($self, " deferred[$fd] done - %0.6f", $elapsed); + my $wbuf = $self->{wbuf}; + $self->requeue unless $wbuf && @$wbuf; + } +} - my $fd = fileno($self->{sock}); - defined $fd or return; +sub long_response ($$;@) { + my ($self, $cb, @args) = @_; # cb returns true if more, false if done + + $self->{sock} or return; # make sure we disable reading during a long response, # clients should not be sending us stuff and making us do more # work while we are stream a response to them - my $t0 = now(); - my $long_cb; # DANGER: self-referential - $long_cb = sub { - # wbuf is unset or empty, here; $cb may add to it - my $more = eval { $cb->() }; - if ($@ || !$self->{sock}) { # something bad happened... - $long_cb = undef; - my $diff = now() - $t0; - if ($@) { - err($self, - "%s during long response[$fd] - %0.6f", - $@, $diff); - } - out($self, " deferred[$fd] aborted - %0.6f", $diff); - $self->close; - } elsif ($more) { # $self->{wbuf}: - $self->update_idle_time; - - # COMPRESS users all share the same DEFLATE context. - # Flush it here to ensure clients don't see - # each other's data - $self->zflush; - - # no recursion, schedule another call ASAP - # but only after all pending writes are done - my $wbuf = $self->{wbuf} ||= []; - push @$wbuf, $long_cb; - - # wbuf may be populated by $cb, no need to rearm if so: - $self->requeue if scalar(@$wbuf) == 1; - } else { # all done! - $long_cb = undef; - res($self, '.'); - out($self, " deferred[$fd] done - %0.6f", now() - $t0); - my $wbuf = $self->{wbuf}; - $self->requeue unless $wbuf && @$wbuf; - } - }; - $self->write($long_cb); # kick off! + $self->{long_cb} = [ $cb, now(), @args ]; + long_step($self); # kick off! undef; } @@ -686,6 +690,18 @@ sub mid_lookup ($$) { (undef, undef); } +sub xref_range_i { + my ($self, $beg, $end) = @_; + my $ng = $self->{ng}; + my $r = $ng->mm->msg_range($beg, $end); + @$r or return; + more($self, join("\r\n", map { + my $num = $_->[0]; + "$num ".xref($self, $ng, $num, $_->[1]); + } @$r)); + 1; +} + sub hdr_xref ($$$) { # optimize XHDR Xref [range] for rtin my ($self, $xhdr, $range) = @_; @@ -699,19 +715,8 @@ sub hdr_xref ($$$) { # optimize XHDR Xref [range] for rtin $range = $self->{article} unless defined $range; my $r = get_range($self, $range); return $r unless ref $r; - my $ng = $self->{ng}; - my $mm = $ng->mm; - my ($beg, $end) = @$r; more($self, $xhdr ? r221 : r225); - long_response($self, sub { - my $r = $mm->msg_range($beg, $end); - @$r or return; - more($self, join("\r\n", map { - my $num = $_->[0]; - "$num ".xref($self, $ng, $num, $_->[1]); - } @$r)); - 1; - }); + long_response($self, \&xref_range_i, @$r); } }