You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Joe Orton <jo...@redhat.com> on 2004/06/15 16:23:42 UTC
Re: huge memory leak in 2.0.x
http://issues.apache.org/bugzilla/show_bug.cgi?id=23567
On Mon, Jun 14, 2004 at 01:45:26PM -0600, Brad Nicholes wrote:
> Actually I think this was addressed quite a while ago with the
> introduction of the MaxMemFree directive. This problem sounds a lot
> like the bucket issue where memory allocated for the bucket brigade that
> pushes the data through from the CGI app to the wire, simply held on to
> the memory assuming that it would be needed later. The MaxMemFree
> directive allowed the memory pool manager to release excess memory
> rather than allowing it to hang around.
The problem here looks exactly as Jeff diagnosed in the bug report: for
each apr_brigade_split() call by the content-length filter, a new
cleanup is allocated out of the request pool. So for this case, you
leak 32 bytes for every 4K block produced by the CGI script as it is
being consumed faster than it's being produced, and read() keeps
returning EAGAIN from the pipe, and the filter keeps wanting to flush.
It's actually very easy to fix that leak by putting used cleanup
structures onto a freelist and re-using them, e.g.:
http://cvs.apache.org/~jorton/apr_cleanfree.diff
But that doesn't solve the problem: apr_brigade_split() pallocs another
32 bytes for the new brigade structure itself, so there's still a leak.
My naive attempts to allocate the brigade structure using either the
bucket allocator or even malloc/free broke everything in strange ways.
So, calling bucket gurus again :) - are there fundamental reasons why
the former is not going to work? Any better ideas?
joe
Re: huge memory leak in 2.0.x
Posted by Bill Stoddard <bi...@wstoddard.com>.
Bill Stoddard wrote:
>
>> @@ -1340,7 +1343,7 @@
>>
>> /* Run cleanups */
>> run_cleanups(&pool->cleanups);
>> - pool->cleanups = NULL;
>> + pool->free_cleanups = pool->cleanups = NULL;
>>
>> /* If new child pools showed up, this is a reason to raise a flag */
>> if (pool->child)
>> @@ -1886,7 +1889,13 @@
>> #endif /* APR_POOL_DEBUG */
>>
>> if (p != NULL) {
>> - c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
>> + if (p->free_cleanups) {
>> + /* reuse a cleanup structure */
>> + c = p->free_cleanups;
>> + p->free_cleanups = c->next;
>> + } else {
>> + c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
>> + }
>> c->data = data;
>> c->plain_cleanup_fn = plain_cleanup_fn;
>> c->child_cleanup_fn = child_cleanup_fn;
>
>
> c->next = NULL; ????
>
Nevermind...
Bill
Re: huge memory leak in 2.0.x
Posted by Bill Stoddard <bi...@wstoddard.com>.
Bill Stoddard wrote:
>
>> @@ -1340,7 +1343,7 @@
>>
>> /* Run cleanups */
>> run_cleanups(&pool->cleanups);
>> - pool->cleanups = NULL;
>> + pool->free_cleanups = pool->cleanups = NULL;
>>
>> /* If new child pools showed up, this is a reason to raise a flag */
>> if (pool->child)
>> @@ -1886,7 +1889,13 @@
>> #endif /* APR_POOL_DEBUG */
>>
>> if (p != NULL) {
>> - c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
>> + if (p->free_cleanups) {
>> + /* reuse a cleanup structure */
>> + c = p->free_cleanups;
>> + p->free_cleanups = c->next;
>> + } else {
>> + c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
>> + }
>> c->data = data;
>> c->plain_cleanup_fn = plain_cleanup_fn;
>> c->child_cleanup_fn = child_cleanup_fn;
>
>
> c->next = NULL; ????
>
Nevermind...
Bill
Re: huge memory leak in 2.0.x
Posted by Bill Stoddard <bi...@wstoddard.com>.
> @@ -1340,7 +1343,7 @@
>
> /* Run cleanups */
> run_cleanups(&pool->cleanups);
> - pool->cleanups = NULL;
> + pool->free_cleanups = pool->cleanups = NULL;
>
> /* If new child pools showed up, this is a reason to raise a flag */
> if (pool->child)
> @@ -1886,7 +1889,13 @@
> #endif /* APR_POOL_DEBUG */
>
> if (p != NULL) {
> - c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
> + if (p->free_cleanups) {
> + /* reuse a cleanup structure */
> + c = p->free_cleanups;
> + p->free_cleanups = c->next;
> + } else {
> + c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
> + }
> c->data = data;
> c->plain_cleanup_fn = plain_cleanup_fn;
> c->child_cleanup_fn = child_cleanup_fn;
c->next = NULL; ????
Re: huge memory leak in 2.0.x
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 15 Jun 2004, Joe Orton wrote:
> apr_brigade_create() does that already.
Oh, duh, of course it does. As many times as I've looked at that line,
you'd think I'd have it memorized by now. :)
Re: huge memory leak in 2.0.x
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 15 Jun 2004, Joe Orton wrote:
> apr_brigade_create() does that already.
Oh, duh, of course it does. As many times as I've looked at that line,
you'd think I'd have it memorized by now. :)
Re: huge memory leak in 2.0.x
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 15, 2004 at 03:14:53PM -0400, Cliff Woolley wrote:
> On Tue, 15 Jun 2004, Joe Orton wrote:
>
> > 2) allocate brigade structures using the bucket allocator
>
> If you're going to do this, then surely you need to call
> apr_pool_cleanup_register() somewhere?
apr_brigade_create() does that already.
joe
Re: huge memory leak in 2.0.x
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 15, 2004 at 03:14:53PM -0400, Cliff Woolley wrote:
> On Tue, 15 Jun 2004, Joe Orton wrote:
>
> > 2) allocate brigade structures using the bucket allocator
>
> If you're going to do this, then surely you need to call
> apr_pool_cleanup_register() somewhere?
apr_brigade_create() does that already.
joe
Re: huge memory leak in 2.0.x
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 15 Jun 2004, Joe Orton wrote:
> 2) allocate brigade structures using the bucket allocator
If you're going to do this, then surely you need to call
apr_pool_cleanup_register() somewhere?
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.60
diff -u -r1.60 apr_brigade.c
--- buckets/apr_brigade.c 13 Feb 2004 09:55:25 -0000 1.60
+++ buckets/apr_brigade.c 15 Jun 2004 15:34:47 -0000
@@ -30,7 +30,9 @@
static apr_status_t brigade_cleanup(void *data)
{
- return apr_brigade_cleanup(data);
+ apr_status_t rv = apr_brigade_cleanup(data);
+ apr_bucket_free(data);
+ return rv;
}
APU_DECLARE(apr_status_t) apr_brigade_cleanup(void *data)
@@ -42,14 +44,12 @@
e = APR_BRIGADE_FIRST(b);
apr_bucket_delete(e);
}
- /* We don't need to free(bb) because it's allocated from a pool. */
return APR_SUCCESS;
}
APU_DECLARE(apr_status_t) apr_brigade_destroy(apr_bucket_brigade *b)
{
- apr_pool_cleanup_kill(b->p, b, brigade_cleanup);
- return apr_brigade_cleanup(b);
+ return apr_pool_cleanup_run(b->p, b, brigade_cleanup);
}
APU_DECLARE(apr_bucket_brigade *) apr_brigade_create(apr_pool_t *p,
@@ -57,7 +57,7 @@
{
apr_bucket_brigade *b;
- b = apr_palloc(p, sizeof(*b));
+ b = apr_bucket_alloc(sizeof *b, list);
b->p = p;
b->bucket_alloc = list;
Re: huge memory leak in 2.0.x
Posted by Marc Heckmann <mh...@nadir.org>.
On Wed, Jun 16, 2004 at 03:12:21PM +0100, Joe Orton wrote:
> On Wed, Jun 16, 2004 at 10:02:21AM -0400, Marc Heckmann wrote:
> > On Wed, Jun 16, 2004 at 02:46:58PM +0100, Joe Orton wrote:
> > yes I believe that is what it does. It basically hammers the httpd
> > with dozens of simultaneous requests to download a file in many
> > pieces. I'll see what I can do about reproducing this later on today.
>
> Ah, I think you're seeing a different and less subtle problem here: the
> byterange filter in 2.0 will suck the entire CGI script output into
> memory too. :(
Well, Joe's patch for this (PR 29528) works. problem solved. I no
longer need to modify my servers to use 1.3.x. This is great.
I'll be running with the patch from now on, so I'll let you all know
if there are any side effects.
see http://issues.apache.org/bugzilla/show_bug.cgi?id=29528 for the
details.
Any chance PR 23567 can get fixed completely at some point in the near
future as well?
Cheers,
-m
Re: huge memory leak in 2.0.x
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jun 16, 2004 at 10:02:21AM -0400, Marc Heckmann wrote:
> On Wed, Jun 16, 2004 at 02:46:58PM +0100, Joe Orton wrote:
> >
> > Thanks for testing this out Marc: unfortunately these patches break
> > assumptions that various filters are making; applying them alone is
> > likely to cause lots of random module segfaults. It's yet to be decided
> > whether the patch is wrong or the modules are wrong...
> >
> > Do you know what "WinGet" does that's special, lots of ranged GET
> > requests?
>
> yes I believe that is what it does. It basically hammers the httpd
> with dozens of simultaneous requests to download a file in many
> pieces. I'll see what I can do about reproducing this later on today.
Ah, I think you're seeing a different and less subtle problem here: the
byterange filter in 2.0 will suck the entire CGI script output into
memory too. :(
joe
Re: huge memory leak in 2.0.x
Posted by Marc Heckmann <mh...@nadir.org>.
On Wed, Jun 16, 2004 at 02:46:58PM +0100, Joe Orton wrote:
>
> Thanks for testing this out Marc: unfortunately these patches break
> assumptions that various filters are making; applying them alone is
> likely to cause lots of random module segfaults. It's yet to be decided
> whether the patch is wrong or the modules are wrong...
>
> Do you know what "WinGet" does that's special, lots of ranged GET
> requests?
yes I believe that is what it does. It basically hammers the httpd
with dozens of simultaneous requests to download a file in many
pieces. I'll see what I can do about reproducing this later on today.
-m
Re: huge memory leak in 2.0.x
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jun 16, 2004 at 09:33:52AM -0400, Marc Heckmann wrote:
> So patched my httpd with Joe's patches and let it run overnight.
>
> So good news is that these patches seem to solve the problem for the
> test cgi mentioned in the original PR (#23567).
>
> However, as soon as someone uses the "WinGet" download accelerator to
> download Mp3 files from my CGI prog (see PR 29528), httpd memory usage
> goes through the roof (90+MB resident per child). I don't have a
> windows machine here to reproduce. I'll try to do that later on
> today. Once again, this is with the prefork MPM on Linux.
Thanks for testing this out Marc: unfortunately these patches break
assumptions that various filters are making; applying them alone is
likely to cause lots of random module segfaults. It's yet to be decided
whether the patch is wrong or the modules are wrong...
Do you know what "WinGet" does that's special, lots of ranged GET
requests?
joe
Re: huge memory leak in 2.0.x
Posted by Marc Heckmann <mh...@nadir.org>.
Hi,
So patched my httpd with Joe's patches and let it run overnight.
So good news is that these patches seem to solve the problem for the
test cgi mentioned in the original PR (#23567).
However, as soon as someone uses the "WinGet" download accelerator to
download Mp3 files from my CGI prog (see PR 29528), httpd memory usage
goes through the roof (90+MB resident per child). I don't have a
windows machine here to reproduce. I'll try to do that later on
today. Once again, this is with the prefork MPM on Linux.
Also, I now see segfaults in the error log, which I never got before:
[Wed Jun 16 03:48:48 2004] [notice] child pid 3609 exit signal Segmentation fault (11)
[Wed Jun 16 04:19:40 2004] [notice] child pid 3603 exit signal Segmentation fault (11)
[Wed Jun 16 04:28:54 2004] [notice] child pid 3606 exit signal Segmentation fault (11)
The original PRs are at:
http://issues.apache.org/bugzilla/show_bug.cgi?id=23567
and http://nagoya.apache.org/bugzilla/show_bug.cgi?id=29528
Cheers,
-m
On Tue, Jun 15, 2004 at 07:59:18PM +0100, Joe Orton wrote:
> On Tue, Jun 15, 2004 at 03:23:42PM +0100, Joe Orton wrote:
> > http://issues.apache.org/bugzilla/show_bug.cgi?id=23567
>
> OK, the issues I was having with using the bucket allocator to allocate
> the brigade were just my own screwups. So, the patches needed to fix
> this issue are attached:
>
> 1) re-use pool cleanup structures
> 2) allocate brigade structures using the bucket allocator
>
> these combine to prevent unbounded memory consumption when using
> apr_brigade_split() from an output filter.
>
> (2) means that use of a brigade after it has been destroyed will now
> result in a segfault. The test suite catches a particularly nice place,
> which leads to:
>
> 3) don't destroy the brigade being flushed in ap_filter_flush; pass down
> a temporary brigade.
>
> (looks like mod_bucketeer is also screwed like this, still working on
> that one)
>
> Comments?
>
> joe
Re: huge memory leak in 2.0.x
Posted by Bill Stoddard <bi...@wstoddard.com>.
> @@ -1340,7 +1343,7 @@
>
> /* Run cleanups */
> run_cleanups(&pool->cleanups);
> - pool->cleanups = NULL;
> + pool->free_cleanups = pool->cleanups = NULL;
>
> /* If new child pools showed up, this is a reason to raise a flag */
> if (pool->child)
> @@ -1886,7 +1889,13 @@
> #endif /* APR_POOL_DEBUG */
>
> if (p != NULL) {
> - c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
> + if (p->free_cleanups) {
> + /* reuse a cleanup structure */
> + c = p->free_cleanups;
> + p->free_cleanups = c->next;
> + } else {
> + c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
> + }
> c->data = data;
> c->plain_cleanup_fn = plain_cleanup_fn;
> c->child_cleanup_fn = child_cleanup_fn;
c->next = NULL; ????
Re: huge memory leak in 2.0.x
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 15 Jun 2004, Joe Orton wrote:
> 2) allocate brigade structures using the bucket allocator
If you're going to do this, then surely you need to call
apr_pool_cleanup_register() somewhere?
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.60
diff -u -r1.60 apr_brigade.c
--- buckets/apr_brigade.c 13 Feb 2004 09:55:25 -0000 1.60
+++ buckets/apr_brigade.c 15 Jun 2004 15:34:47 -0000
@@ -30,7 +30,9 @@
static apr_status_t brigade_cleanup(void *data)
{
- return apr_brigade_cleanup(data);
+ apr_status_t rv = apr_brigade_cleanup(data);
+ apr_bucket_free(data);
+ return rv;
}
APU_DECLARE(apr_status_t) apr_brigade_cleanup(void *data)
@@ -42,14 +44,12 @@
e = APR_BRIGADE_FIRST(b);
apr_bucket_delete(e);
}
- /* We don't need to free(bb) because it's allocated from a pool. */
return APR_SUCCESS;
}
APU_DECLARE(apr_status_t) apr_brigade_destroy(apr_bucket_brigade *b)
{
- apr_pool_cleanup_kill(b->p, b, brigade_cleanup);
- return apr_brigade_cleanup(b);
+ return apr_pool_cleanup_run(b->p, b, brigade_cleanup);
}
APU_DECLARE(apr_bucket_brigade *) apr_brigade_create(apr_pool_t *p,
@@ -57,7 +57,7 @@
{
apr_bucket_brigade *b;
- b = apr_palloc(p, sizeof(*b));
+ b = apr_bucket_alloc(sizeof *b, list);
b->p = p;
b->bucket_alloc = list;
Re: huge memory leak in 2.0.x
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 15, 2004 at 03:23:42PM +0100, Joe Orton wrote:
> http://issues.apache.org/bugzilla/show_bug.cgi?id=23567
OK, the issues I was having with using the bucket allocator to allocate
the brigade were just my own screwups. So, the patches needed to fix
this issue are attached:
1) re-use pool cleanup structures
2) allocate brigade structures using the bucket allocator
these combine to prevent unbounded memory consumption when using
apr_brigade_split() from an output filter.
(2) means that use of a brigade after it has been destroyed will now
result in a segfault. The test suite catches a particularly nice place,
which leads to:
3) don't destroy the brigade being flushed in ap_filter_flush; pass down
a temporary brigade.
(looks like mod_bucketeer is also screwed like this, still working on
that one)
Comments?
joe
Re: huge memory leak in 2.0.x
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 15, 2004 at 03:23:42PM +0100, Joe Orton wrote:
> http://issues.apache.org/bugzilla/show_bug.cgi?id=23567
OK, the issues I was having with using the bucket allocator to allocate
the brigade were just my own screwups. So, the patches needed to fix
this issue are attached:
1) re-use pool cleanup structures
2) allocate brigade structures using the bucket allocator
these combine to prevent unbounded memory consumption when using
apr_brigade_split() from an output filter.
(2) means that use of a brigade after it has been destroyed will now
result in a segfault. The test suite catches a particularly nice place,
which leads to:
3) don't destroy the brigade being flushed in ap_filter_flush; pass down
a temporary brigade.
(looks like mod_bucketeer is also screwed like this, still working on
that one)
Comments?
joe