[PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7

[PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7

[Eric Wong]

After many hours of reviewing our code in PublicInbox::Qspawn,
PublicInbox::GitHTTPBackend, and PublicInbox::HTTP and finding
nothing but cleanups and documentation improvements; it seems
the leaks affecting lore is down to bugs in Perl 5.16.3.

After removing the warning for Deflater being missing in
d883d4a93b23be134038e28f421eafca70c3d838
("httpd: get rid of Deflater warning"), I missed that my
own CentOS 7 VM was missing that module so was unable to
reproduce the FD leaks :x

The first patch is a straightforward workaround that I was
able to test without Plack::Middleware::Deflater being installed.

The second patch stops loading Deflater in httpd on Perls
earlier than 5.18.  (But I haven't built or tested 5.18 myself).
Enabling gzip in varnish will be needed for 5.16 users.

Independently of this fix, I've long been considering replacing
Deflater with a buffer-to-gzip layer which would reduce memory
pressure from intermediate uncompressed strings.  This might
serve as impetus to move that idea along (and of course I'd
test it heavily under CentOS 7 :>).

Anybody using custom .psgi files on Perl 5.16.x will need to
make adjustments to disable the Deflater in that.

Eric Wong (2):
  ds: workaround a memory leak in Perl 5.16.x
  httpd: disable Deflater middleware by default on Perl <5.18

 lib/PublicInbox/DS.pm     | 9 ++++++---
 script/public-inbox-httpd | 8 +++++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

-- 
EW

[PATCH 1/2] ds: workaround a memory leak in Perl 5.16.x

[Eric Wong]

The perl-5.16.3-294.el7_6 RPM package on RHEL/CentOS 7 is
affected by a memory leak in Perl when calling `ref' on
blessed references.  This resulted in a very slow leak that
manifests more quickly with a nonstop "git fetch" loop.

Use Scalar::Util::blessed to work around the issue.
Tested overnight on a CentOS 7 VM.

cf. https://rt.perl.org/Public/Bug/Display.html?id=114340
---
 lib/PublicInbox/DS.pm | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 30a9641..7f7cb85 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -24,6 +24,7 @@ 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;
@@ -178,10 +179,12 @@ sub next_tick () {
     my $q = $nextq;
     $nextq = [];
     for (@$q) {
-        if (ref($_) eq 'CODE') {
-            $_->();
-        } else {
+        # we avoid "ref" on blessed refs to workaround a Perl 5.16.3 leak:
+        # https://rt.perl.org/Public/Bug/Display.html?id=114340
+        if (blessed($_)) {
             $_->event_step;
+        } else {
+            $_->();
         }
     }
 }
-- 
EW

[PATCH 2/2] httpd: disable Deflater middleware by default on Perl <5.18

[Eric Wong]

Testing with perl-5.16.3-294.el7_6 RPM package on RHEL/CentOS 7,
the Deflater middleware triggers a leak when used in conjunction
with our push-based responses from PublicInbox::Qspawn.

I could not find another solution to workaround the memory leak
in this case, and I could not find a specific leak fix in
the perl5180delta manpage[1] which looked like it would
solve our problem.

Attempting to workaround the issue proved futile.  Using
internal Deflater-specific keys to prevent deflating in
GitHTTPBackend and Qspawn did not solve the problem:

        $env->{"plack.skip-deflater"} = 1;
        $env->{"psgix.no-compress"} = 1;

Nor did forcing an invalid encoding via "git fetch":

	git -c http.extraheader=Accept-Encoding:gzap fetch

So this appears to be a problem with Plack::Util::response_cb
somewhere.

This does NOT appear to be a problem with ref() leaking as in
DS::next_tick[2], since I couldn't find where
Plack::Middleware::Deflater or Plack::Util::response_cb would be
calling ref() on a blessed reference to trigger a leak.

Also, oddly enough, the ref() use for backwards compatibility at
the top of PublicInbox::GitHTTPBackend::serve does NOT seem to
trigger a leak on 5.16.3 due to [2]:

	# XXX compatibility... ugh, can we stop supporting this?
	$git = PublicInbox::Git->new($git) unless ref($git);

[1] https://perldoc.perl.org/perl5180delta.html
[2] https://rt.perl.org/Public/Bug/Display.html?id=114340
---
 script/public-inbox-httpd | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/script/public-inbox-httpd b/script/public-inbox-httpd
index b2464f4..9b869f9 100755
--- a/script/public-inbox-httpd
+++ b/script/public-inbox-httpd
@@ -24,7 +24,13 @@ my $refresh = sub {
 		my $www = PublicInbox::WWW->new;
 		$www->preload;
 		$app = builder {
-			eval {
+			# Perl 5.16.3 leaks in our "push" response code path
+			# (e.g. Qspawn) due to something in
+			# Plack::Util::response_cb, regardless of whether the
+			# client is sending Accept-Encoding:gzip requests.
+			# perl5180delta documents many leak fixes, so assume
+			# 5.18+ is safe for now and bump the check as-need:
+			$] >= 5.018000 and eval {
 				enable 'Deflater',
 					content_type => [ qw(
 						text/html
-- 
EW

Re: [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7

[Konstantin Ryabitsev]

On Thu, Sep 26, 2019 at 01:50:36AM +0000, Eric Wong wrote:
>After many hours of reviewing our code in PublicInbox::Qspawn,
>PublicInbox::GitHTTPBackend, and PublicInbox::HTTP and finding
>nothing but cleanups and documentation improvements; it seems
>the leaks affecting lore is down to bugs in Perl 5.16.3.
>
>After removing the warning for Deflater being missing in
>d883d4a93b23be134038e28f421eafca70c3d838
>("httpd: get rid of Deflater warning"), I missed that my
>own CentOS 7 VM was missing that module so was unable to
>reproduce the FD leaks :x
>
>The first patch is a straightforward workaround that I was
>able to test without Plack::Middleware::Deflater being installed.
>
>The second patch stops loading Deflater in httpd on Perls
>earlier than 5.18.  (But I haven't built or tested 5.18 myself).
>Enabling gzip in varnish will be needed for 5.16 users.

Yay, I can confirm that the latest master fixes all the FD leaks that 
have been plaguing lore.kernel.org for the past few weeks. The number of 
pipes is stable and there are no (deleted) tempfiles showing up. Thanks 
so much for this!

Can you elaborate on gzip+varnish changes? I'm assuming it's something 
as simple as:

@@ -63,6 +63,10 @@
        } else {
                /* short TTL for up-to-dateness, our PSGI is not that slow */
                set beresp.ttl = 10s;
+               /* Compress text responses on CentOS7 */
+               if (beresp.http.content-type ~ "text") {
+                       set beresp.do_gzip = true;
+               }
        }
        return (deliver);
 }

-K

Re: [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7

[Eric Wong]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> On Thu, Sep 26, 2019 at 01:50:36AM +0000, Eric Wong wrote:
> > After many hours of reviewing our code in PublicInbox::Qspawn,
> > PublicInbox::GitHTTPBackend, and PublicInbox::HTTP and finding
> > nothing but cleanups and documentation improvements; it seems
> > the leaks affecting lore is down to bugs in Perl 5.16.3.
> > 
> > After removing the warning for Deflater being missing in
> > d883d4a93b23be134038e28f421eafca70c3d838
> > ("httpd: get rid of Deflater warning"), I missed that my
> > own CentOS 7 VM was missing that module so was unable to
> > reproduce the FD leaks :x
> > 
> > The first patch is a straightforward workaround that I was
> > able to test without Plack::Middleware::Deflater being installed.
> > 
> > The second patch stops loading Deflater in httpd on Perls
> > earlier than 5.18.  (But I haven't built or tested 5.18 myself).
> > Enabling gzip in varnish will be needed for 5.16 users.
> 
> Yay, I can confirm that the latest master fixes all the FD leaks that have
> been plaguing lore.kernel.org for the past few weeks. The number of pipes is
> stable and there are no (deleted) tempfiles showing up. Thanks so much for
> this!

Good to know!  Also, how's memory use?

It ought to be stable if Perl is doing the right thing; but I
haven't tested all the endpoints thoroughly on 5.16.3
(especially the mbox.gz ones)

> Can you elaborate on gzip+varnish changes? I'm assuming it's something as
> simple as:
> 
> @@ -63,6 +63,10 @@
>        } else {
>                /* short TTL for up-to-dateness, our PSGI is not that slow */
>                set beresp.ttl = 10s;
> +               /* Compress text responses on CentOS7 */
> +               if (beresp.http.content-type ~ "text") {
> +                       set beresp.do_gzip = true;
> +               }

*shrug*  Whatever the varnish docs says :)

I still prefer to do gzip in the Perl process to minimize IPC
overhead (and to eventually make things easier-to-deploy w/o
needing Varnish).

I think I can add gzip support to WwwStream and WwwAtomStream
pretty easily, and we're already using a custom gzipper for
the mboxrd endpoints.

[PATCH 3/2] qspawn: workaround Perl 5.16.3 leak, re-enable Deflater

[Eric Wong]

The httpd-supplied write callback is the leak culprit under Perl
5.16.3.  undef-ing it immediately after use keeps a repeated
"git fetch" loop from monotonically increasing memory and FD use
on the Perl shipped with RHEL/CentOS 7.x.

Other endpoints tested showed no increase in memory use under
constant load with "ab -HAccept-Encoding:gzip -k", including the
async psgi_qx code path used by $INBOX_URL/$OBJECT_ID/s/ via
SolverGit module.
---
 Note: I initially tried this change, but thought it only slowed
 down the leaking because I had not yet discovered the
 workaround in commit cd71a869c7e9c811
 ("ds: workaround a memory leak in Perl 5.16.x").

 Now that both leaks are worked around, memory usage is completely
 flat when repeating a single request of any type with gzip-enabled.

 lib/PublicInbox/Qspawn.pm | 4 ++++
 script/public-inbox-httpd | 8 +-------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm
index 5a30064..cb3dc51 100644
--- a/lib/PublicInbox/Qspawn.pm
+++ b/lib/PublicInbox/Qspawn.pm
@@ -281,6 +281,10 @@ sub psgi_return {
 								$buf, $filter);
 			$wcb->($r);
 		}
+
+		# Workaround a leak under Perl 5.16.3 when combined with
+		# Plack::Middleware::Deflater:
+		$wcb = undef;
 	};
 	$limiter ||= $def_limiter ||= PublicInbox::Qspawn::Limiter->new(32);
 	my $start_cb = sub { # may run later, much later...
diff --git a/script/public-inbox-httpd b/script/public-inbox-httpd
index 9b869f9..b2464f4 100755
--- a/script/public-inbox-httpd
+++ b/script/public-inbox-httpd
@@ -24,13 +24,7 @@ my $refresh = sub {
 		my $www = PublicInbox::WWW->new;
 		$www->preload;
 		$app = builder {
-			# Perl 5.16.3 leaks in our "push" response code path
-			# (e.g. Qspawn) due to something in
-			# Plack::Util::response_cb, regardless of whether the
-			# client is sending Accept-Encoding:gzip requests.
-			# perl5180delta documents many leak fixes, so assume
-			# 5.18+ is safe for now and bump the check as-need:
-			$] >= 5.018000 and eval {
+			eval {
 				enable 'Deflater',
 					content_type => [ qw(
 						text/html
-- 
EW