* [PATCH 1/2] ds: workaround a memory leak in Perl 5.16.x
2019-09-26 1:50 [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7 Eric Wong
@ 2019-09-26 1:50 ` Eric Wong
2019-09-26 1:50 ` [PATCH 2/2] httpd: disable Deflater middleware by default on Perl <5.18 Eric Wong
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2019-09-26 1:50 UTC (permalink / raw)
To: meta
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] httpd: disable Deflater middleware by default on Perl <5.18
2019-09-26 1:50 [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7 Eric Wong
2019-09-26 1:50 ` [PATCH 1/2] ds: workaround a memory leak in Perl 5.16.x Eric Wong
@ 2019-09-26 1:50 ` Eric Wong
2019-09-26 12:36 ` [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7 Konstantin Ryabitsev
2019-09-27 21:01 ` [PATCH 3/2] qspawn: workaround Perl 5.16.3 leak, re-enable Deflater Eric Wong
3 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2019-09-26 1:50 UTC (permalink / raw)
To: meta
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7
2019-09-26 1:50 [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7 Eric Wong
2019-09-26 1:50 ` [PATCH 1/2] ds: workaround a memory leak in Perl 5.16.x Eric Wong
2019-09-26 1:50 ` [PATCH 2/2] httpd: disable Deflater middleware by default on Perl <5.18 Eric Wong
@ 2019-09-26 12:36 ` Konstantin Ryabitsev
2019-09-26 21:16 ` Eric Wong
2019-09-27 21:01 ` [PATCH 3/2] qspawn: workaround Perl 5.16.3 leak, re-enable Deflater Eric Wong
3 siblings, 1 reply; 6+ messages in thread
From: Konstantin Ryabitsev @ 2019-09-26 12:36 UTC (permalink / raw)
To: Eric Wong; +Cc: meta
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7
2019-09-26 12:36 ` [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7 Konstantin Ryabitsev
@ 2019-09-26 21:16 ` Eric Wong
0 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2019-09-26 21:16 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: meta
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/2] qspawn: workaround Perl 5.16.3 leak, re-enable Deflater
2019-09-26 1:50 [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7 Eric Wong
` (2 preceding siblings ...)
2019-09-26 12:36 ` [PATCH 0/2] leak workarounds for Perl 5.16 on CentOS/RHEL 7 Konstantin Ryabitsev
@ 2019-09-27 21:01 ` Eric Wong
3 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2019-09-27 21:01 UTC (permalink / raw)
To: meta
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
^ permalink raw reply related [flat|nested] 6+ messages in thread