Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: neutral (nike.apache.org: local policy)
Date: Mon, 5 Jan 2009 13:07:42 +0200
From: Sami Tolvanen <sami.tolvanen@mywot.com>
To: dev@apr.apache.org
Subject: [PATCH] apr_memcache memory leak with persistent connections
Message-ID: <20090105110742.GA30166@tolvanen.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3V7upXqbjpZ4EhLz"
Content-Disposition: inline
User-Agent: Mutt/1.5.18 (2008-05-17)


--3V7upXqbjpZ4EhLz
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hello all,

We ran into a problem with apr_memcache after upgrading from httpd 2.0
to 2.2. Depending on the TTL passed to apr_memcache_server_create in our
Apache module, we were either leaking lots of memory or grinding to a
halt a few seconds after httpd was started due to an overwhelming number
of connections to memcached servers.

This turned out to be a known problem with apr_memcache, which we didn't
trigger with the apr_reslist implementation used in httpd 2.0 / APR-util
0.9.x. It seems to have been first reported ~two years ago:

  [1] http://issues.outoforder.cc/view.php?id=68
  [2] http://tinyurl.com/apr-dev-200701-memcache
  [3] http://tinyurl.com/apr-dev-200810-memcache

To sum it up, the problem is that memcache functions keep allocating
memory from a connection-specific pool during the entire lifetime of the
connection. Therefore, for a process that constantly performs requests,
any reasonable TTL parameter means there are server connections that are
never closed and just keep consuming more memory.

The patch in [3] seems to mostly mitigate the memory leak issue,
but addresses only apr_memcache_getp and doesn't work for multiline
data. I ended up writing the attached patch (against APR-util 1.3.4),
which allocates bucket brigades from a temporary pool stored in
apr_memcache_conn_t and clears it after each request. We have used this
in production for a couple of days now and the memory leak problem with
persistent connections is gone. There doesn't seem to be any noticeable
performance impact either, at least for our work load.

I would appreciate it if someone more familiar with the memcache code
could review the patch and let me know if there are any gotchas with
this approach. Unless there's a better way for fixing the memory leak
without modifying the API, I propose applying this patch to APR-util as
more than one apr_memcache user seems to have been affected over the
years.

Sami

--3V7upXqbjpZ4EhLz
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fix-apr-util-memcache-leak.patch"

Index: memcache/apr_memcache.c
===================================================================
--- memcache/apr_memcache.c	(revision 38)
+++ memcache/apr_memcache.c	(working copy)
@@ -25,8 +25,8 @@
     char *buffer;
     apr_size_t blen;
     apr_pool_t *p;
+    apr_pool_t *tp;
     apr_socket_t *sock;
-    apr_bucket_alloc_t *balloc;
     apr_bucket_brigade *bb;
     apr_bucket_brigade *tb;
     apr_memcache_server_t *ms;
@@ -224,12 +224,29 @@
 
 static apr_status_t ms_find_conn(apr_memcache_server_t *ms, apr_memcache_conn_t **conn) 
 {
+    apr_status_t rv;
+    apr_bucket_alloc_t *balloc;
+    apr_bucket *e;
+
 #if APR_HAS_THREADS
-    return apr_reslist_acquire(ms->conns, (void **)conn);
+    rv = apr_reslist_acquire(ms->conns, (void **)conn);
 #else
     *conn = ms->conn;
-    return APR_SUCCESS;
+    rv = APR_SUCCESS;
 #endif
+
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
+
+    balloc = apr_bucket_alloc_create((*conn)->tp);
+    (*conn)->bb = apr_brigade_create((*conn)->tp, balloc);
+    (*conn)->tb = apr_brigade_create((*conn)->tp, balloc);
+
+    e = apr_bucket_socket_create((*conn)->sock, balloc);
+    APR_BRIGADE_INSERT_TAIL((*conn)->bb, e);
+
+    return rv;
 }
 
 static apr_status_t ms_bad_conn(apr_memcache_server_t *ms, apr_memcache_conn_t *conn) 
@@ -243,6 +260,7 @@
 
 static apr_status_t ms_release_conn(apr_memcache_server_t *ms, apr_memcache_conn_t *conn) 
 {
+    apr_pool_clear(conn->tp);
 #if APR_HAS_THREADS
     return apr_reslist_release(ms->conns, conn);
 #else
@@ -322,8 +340,8 @@
 {
     apr_status_t rv = APR_SUCCESS;
     apr_memcache_conn_t *conn;
-    apr_bucket *e;
     apr_pool_t *np;
+    apr_pool_t *tp;
     apr_memcache_server_t *ms = params;
 
     rv = apr_pool_create(&np, pool);
@@ -331,6 +349,12 @@
         return rv;
     }
 
+    rv = apr_pool_create(&tp, np);
+    if (rv != APR_SUCCESS) {
+        apr_pool_destroy(np);
+        return rv;
+    }
+
 #if APR_HAS_THREADS
     conn = malloc(sizeof( apr_memcache_conn_t )); /* non-pool space! */
 #else
@@ -338,6 +362,7 @@
 #endif
 
     conn->p = np;
+    conn->tp = tp;
 
     rv = apr_socket_create(&conn->sock, APR_INET, SOCK_STREAM, 0, np);
 
@@ -349,16 +374,10 @@
         return rv;
     }
 
-    conn->balloc = apr_bucket_alloc_create(conn->p);
-    conn->bb = apr_brigade_create(conn->p, conn->balloc);
-    conn->tb = apr_brigade_create(conn->p, conn->balloc);
     conn->buffer = apr_palloc(conn->p, BUFFER_SIZE);
     conn->blen = 0;
     conn->ms = ms;
 
-    e = apr_bucket_socket_create(conn->sock, conn->balloc);
-    APR_BRIGADE_INSERT_TAIL(conn->bb, e);
-
     rv = conn_connect(conn);
     if (rv != APR_SUCCESS) {
         apr_pool_destroy(np);

--3V7upXqbjpZ4EhLz--