Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <48FF7881.8050703@apache.org>
Date: Wed, 22 Oct 2008 21:01:21 +0200
From: Ruediger Pluem <rpluem@apache.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.1.17) Gecko/20080829 SeaMonkey/1.1.12
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Re: Memory leak in mod_proxy_http and in mpm_event components.
References: <d68cf13d0810190631s44af6230n35ac17a9ddb2bffb@mail.gmail.com>
	<d68cf13d0810220513j577c36b0s6d8cf87ffe0634cd@mail.gmail.com>
 <20081022134922.229ade11@grimnir> <48FF414C.9000600@movik.net>
 <CDC394C7-8093-4557-9D80-8F85FD203508@jaguNET.com>
In-Reply-To: <CDC394C7-8093-4557-9D80-8F85FD203508@jaguNET.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit


On 10/22/2008 05:33 PM, Jim Jagielski wrote:
> 
> On Oct 22, 2008, at 11:05 AM, Manik Taneja wrote:
> 
>>
>> With regards to the patch, it seems that the changes in event.c have
>> no effect. We have anyway seen the leak with both worker and event
>> mpm. The changes in scoreboard.c just ensure that memory pointed to by
>> new_sbh is not lost everytime ap_create_sb_handle is called.
>>
> 
> But this implies that any code which calls ap_create_sb_handle() needs
> to ensure that *new_sbh isn't NULL, and, iirc, most just define them
> (eg: ap_sb_handle_t *sbh;) so the value is garbage. Thus, this seems
> to also imply a change to the API (since current code will break
> with the ap_create_sb_handle() change.
> 
>> I have tested this fix and the memory consumption has dropped
>> drastically, however I still do observe a slow leak at times. I
>> haven't got the opportunity to spend a whole lot of time testing the
>> fix. Its on my plate however, and I'll send out my findings to the
>> group sometime next week.
>>
> 
> Agreed that the mod_proxy_http.c patch looks reasonable at 1st
> blush, but need to look into all possible interaction with the
> change to a shorter-lived pool.

There is an explanation above the code why we should take connection->pool,
but IMHO it is outdated and goes back to the times where there was a direct
connection between the backend connection and the frontend connection.
With the usage of the connection pools this is no longer the case and I
cannot think of any reason why not using r->pool instead.

I used a slightly modified version of the patch:

Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c      (Revision 707022)
+++ modules/proxy/mod_proxy_http.c      (Arbeitskopie)
@@ -1869,23 +1869,9 @@
     const char *u;
     proxy_conn_rec *backend = NULL;
     int is_ssl = 0;
-
-    /* Note: Memory pool allocation.
-     * A downstream keepalive connection is always connected to the existence
-     * (or not) of an upstream keepalive connection. If this is not done then
-     * load balancing against multiple backend servers breaks (one backend
-     * server ends up taking 100% of the load), and the risk is run of
-     * downstream keepalive connections being kept open unnecessarily. This
-     * keeps webservers busy and ties up resources.
-     *
-     * As a result, we allocate all sockets out of the upstream connection
-     * pool, and when we want to reuse a socket, we check first whether the
-     * connection ID of the current upstream connection is the same as that
-     * of the connection when the socket was opened.
-     */
-    apr_pool_t *p = r->connection->pool;
+    apr_pool_t *p = r->pool;
     conn_rec *c = r->connection;
-    apr_uri_t *uri = apr_palloc(r->connection->pool, sizeof(*uri));
+    apr_uri_t *uri = apr_palloc(p, sizeof(*uri));

     /* find the scheme */
     u = strchr(url, ':');
@@ -1893,7 +1879,7 @@
        return DECLINED;
     if ((u - url) > 14)
         return HTTP_BAD_REQUEST;
-    scheme = apr_pstrndup(c->pool, url, u - url);
+    scheme = apr_pstrndup(p, url, u - url);
     /* scheme is lowercase */
     ap_str_tolower(scheme);
     /* is it for us? */

and it passes all perl framework tests.

Regards

R�diger