You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by bo...@apache.org on 2008/07/20 23:29:13 UTC
svn commit: r678323 - /apr/apr-util/trunk/memcache/apr_memcache.c
Author: bojan
Date: Sun Jul 20 14:29:13 2008
New Revision: 678323
URL: http://svn.apache.org/viewvc?rev=678323&view=rev
Log:
Close memory leak.
Modified:
apr/apr-util/trunk/memcache/apr_memcache.c
Modified: apr/apr-util/trunk/memcache/apr_memcache.c
URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/memcache/apr_memcache.c?rev=678323&r1=678322&r2=678323&view=diff
==============================================================================
--- apr/apr-util/trunk/memcache/apr_memcache.c (original)
+++ apr/apr-util/trunk/memcache/apr_memcache.c Sun Jul 20 14:29:13 2008
@@ -359,6 +359,8 @@
/* Return values not checked, since we just want to make it go away. */
apr_socket_sendv(conn->sock, vec, 2, &written);
apr_socket_close(conn->sock);
+
+ apr_pool_destroy(conn->p);
return APR_SUCCESS;
}
Re: svn commit: r678323
- /apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-08-08 at 01:18 +0200, Peter Poeml wrote:
> Anyway, there must be something that I overlook, or there may be other
> circumstances that influence the behaviour (1.2/1.3 difference in pool
> handling??). The patch from trunk fixes the issue for me although I
> can't say why. I can only say for sure that I have a large memleak with
> the current 1.3.x code.
The patch from trunk may work by accident, depending on how
apr_reslist_create() was called and the order of pool destruction.
However, it may create double free situation in some circumstances (this
is what big discussion about pre_cleanups was all about), so it is not
safe.
--
Bojan
Re: svn commit: r678323
- /apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-08-08 at 03:45 +0200, Peter Poeml wrote:
> So the usual stability of the
> API isn't so important (yet) in my opionion.
The new API in reslist that switches the behaviour of cleanups cannot be
made public in 1.3.x, due to our versioning rules. But, we could do that
internally, if that's what the list prefers. I leave it to others to
revert my commit and apply the new behaviour.
> But there may be other apr_reslist uses that I am not aware of.
That's exactly why pre_cleanup default was not accepted. Both behaviours
can lead to crashes (of different kind). So, we must stick with the old
crashes, in order to avoid breaking existing software. Just another
API/ABI rule we have.
In any event, it's good that we have solutions for this. Thanks for
testing!
--
Bojan
Re: svn commit: r678323 - /apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Peter Poeml <po...@suse.de>.
On Fri, Aug 08, 2008 at 09:50:24AM +1000, Bojan Smojver wrote:
> On Fri, 2008-08-08 at 01:40 +0200, Peter Poeml wrote:
>
> > That patch does the job as well.
>
> OK, thanks. Committed to 1.3.x now in r683756.
Thanks!
Runs fine so far, and seems to use less memory than ever :-)
> Folks, this commit is quite obviously a hack around limitations of
> reslist in 1.3.x. If the list decides it is better that we have
> apr_reslist_cleanup_order_set() in 1.3.x as non-public API, please feel
> free to revert it and apply the fix from the trunk.
From a module programmer's viewpoint I am -- in the case of
apr_dbd/mod_dbd and apr_memcache -- very much in favour of cleanup and
moving forward, because so far, it is (was) pretty difficult to use them
without running in all sorts of problems. So the usual stability of the
API isn't so important (yet) in my opionion.
Even with the changes that 1.2/1.3 got in this area, I had to adapt some
things in the course.
But there may be other apr_reslist uses that I am not aware of.
(I cannot argue about apr_reslist itself really, I can only comment on
what I encountered on the "surface".)
Peter
--
"WARNING: This bug is visible to non-employees. Please be respectful!"
SUSE LINUX Products GmbH
Research & Development
Re: svn commit: r678323
- /apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2008-08-08 at 01:40 +0200, Peter Poeml wrote:
> That patch does the job as well.
OK, thanks. Committed to 1.3.x now in r683756.
Folks, this commit is quite obviously a hack around limitations of
reslist in 1.3.x. If the list decides it is better that we have
apr_reslist_cleanup_order_set() in 1.3.x as non-public API, please feel
free to revert it and apply the fix from the trunk.
--
Bojan
Re: svn commit: r678323 - /apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Peter Poeml <po...@suse.de>.
On Fri, Aug 08, 2008 at 01:18:19AM +0200, Peter Poeml wrote:
> Anyway, there must be something that I overlook, or there may be other
> circumstances that influence the behaviour (1.2/1.3 difference in pool
> handling??). The patch from trunk fixes the issue for me although I
> can't say why. I can only say for sure that I have a large memleak with
> the current 1.3.x code.
>
> Bojan, I'll test the other patch you posted in a bit.
That patch does the job as well.
Peter
--
"WARNING: This bug is visible to non-employees. Please be respectful!"
SUSE LINUX Products GmbH
Research & Development
Re: svn commit: r678323 - /apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Peter Poeml <po...@suse.de>.
On Mon, Jul 21, 2008 at 06:46:48PM +1000, Bojan Smojver wrote:
> On Mon, 2008-07-21 at 07:34 +0200, Mladen Turk wrote:
>
> > Although IMHO it makes reslist API behave like it should
>
> Agreed.
>
> > it might
> > break some of the existing usages where users were doing nasty tricks
> > with reslist to make the things not to leak memory.
>
> Yeah, that's what I was worried about as well. Let's see what others on
> the list think about it.
Hi, (as mentioned in another thread some minutes ago) I am using
apr_memcache quite a lot. I am experiencing a memleak. I am facing the
following puzzling situation.
I have been apr_memcache.c from trunk since over a year, and it worked
fine for me, with the 1.2.x. branch.
In June, I upgraded to apr/apu 1.3.0 and httpd-2.2.9, and had a big
memleak since then. I had to downgrade before I could investigate it,
but I revisited it today, and here is what I found:
My Apache leaks memory when I call apr_memcache_getp(), and I can
reproduce this with
* apr/apu 1.3.0
* apr/apu 1.3.2
* apr/apu pre-1.3.3 (current svn branch)
I can NOT reproduce it with
* 1.2.12 and the apr_memcache.c version which has been in trunk a long
time (I picked it up from r577947 in September last year)
I saw the commit r678323 from apr-util trunk and I just tried it out on
1.3.2. Indeed, it fixes the memleak for me.
(the one that adds an apr_pool_destroy() call to the end of
mc_conn_destruct()
What's puzzling to me is
1) that the apr_memcache.c file in the released
apr-util 1.3.2 tarball is more or less identical to the one I have
been using since nearly a year -- still I didn't have the memleak
with the latter! (Identical except for some type changes and
renames)
2) another module I'm using is mod_ip_count. It also calls
apr_memcache_getp() but I'm not seeing a memleak in this case. (At
least I am not noticing it... but that is machine with much less
traffic)
I use mod_memcache in all cases to configure the memcache context and
acquire it during request processing.
Anyway, there must be something that I overlook, or there may be other
circumstances that influence the behaviour (1.2/1.3 difference in pool
handling??). The patch from trunk fixes the issue for me although I
can't say why. I can only say for sure that I have a large memleak with
the current 1.3.x code.
Bojan, I'll test the other patch you posted in a bit.
Thanks,
Peter
--
"WARNING: This bug is visible to non-employees. Please be respectful!"
SUSE LINUX Products GmbH
Research & Development
Re: svn commit: r678323
- /apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2008-07-21 at 07:34 +0200, Mladen Turk wrote:
> Although IMHO it makes reslist API behave like it should
Agreed.
> it might
> break some of the existing usages where users were doing nasty tricks
> with reslist to make the things not to leak memory.
Yeah, that's what I was worried about as well. Let's see what others on
the list think about it.
--
Bojan
Re: svn commit: r678323 - /apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Mladen Turk <mt...@apache.org>.
Bojan Smojver wrote:
> On Sun, 2008-07-20 at 21:29 +0000, bojan@apache.org wrote:
>
>> URL: http://svn.apache.org/viewvc?rev=678323&view=rev
>> Log:
>> Close memory leak.
>
> This, of course, has to do with pre_cleanups now working and being used
> for reslists.
>
> Mladen,
>
> I'm not sure if you're planning on backporting pre_cleanup use in
> reslist to 1.3.x, but if you do, we can backport this to 1.3.x as well.
> If not, we can then apply that other fix that I posted to the list a few
> days again.
>
I'm still not sure about backporting.
Although IMHO it makes reslist API behave like it should, it might
break some of the existing usages where users were doing nasty tricks
with reslist to make the things not to leak memory.
It would certainly simplify the mod_proxy connection pool by an order
of magnitude, by not requiring registering additional cleanup for each
socket, etc.
I'm also not sure how to document those changes, cause even before the
behavior was deeply hidden inside internal pool work logic, and you've
need to look at the pool source to figure out why the reslist destructor
makes double free.
Regards
--
^(TM)
Re: svn commit: r678323 -
/apr/apr-util/trunk/memcache/apr_memcache.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sun, 2008-07-20 at 21:29 +0000, bojan@apache.org wrote:
> URL: http://svn.apache.org/viewvc?rev=678323&view=rev
> Log:
> Close memory leak.
This, of course, has to do with pre_cleanups now working and being used
for reslists.
Mladen,
I'm not sure if you're planning on backporting pre_cleanup use in
reslist to 1.3.x, but if you do, we can backport this to 1.3.x as well.
If not, we can then apply that other fix that I posted to the list a few
days again.
--
Bojan