Memory leak apache.table()

• Key: MODPYTHON-184
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Jim Gallacher
• Reporter: Jim Gallacher
• Component/s: core
• Affects Version/s: 3.3.x, 3.2.10
• Fix Version/s: 3.3.1

Description

There is a memory leak in apache.table().

from mod_python import apache

def handler(req):
    req.content_type = 'text/plain'
    t = apache.make_table()

    req.write('ok table:')
    return apache.OK

Using mpm-worker with StartServers 2, and 20000 requests results in memory consumption going from 1.2% to 9.3% per process. (ie approx 8k per request)

This will have an impact on FieldStorage which makes use of apache.make_table(), which is the deprecated name for apache.table()

Comments

[Graham Dumpleton]

The cause of this seems to result from the following.

1. The apache.make_table attribute is actually an alias for the type object for "mp_table".

2. Calling apache.make_table() invokes the table_new() method of the "mp_table" type.

3. The table_new() method in turn calls table_alloc() of the "mp_table" type.

4. In table_alloc() it calls MpTable_New() which creates a whole and complete "mp_table" object instance.

5. After table_alloc() returns to table_new(), the code following duplicates the creation process again creating a new "mp_table" object instance which replaces the first one created. The first one created is now no longer accessible.

The problematic code is:

    self = type->tp_alloc(type, 0);
    if (self != NULL) {
        apr_pool_t *p;
        tableobject *t = (tableobject *)self;
        apr_pool_create_ex(&p, NULL, NULL, NULL);
        t->pool = p;
        t->table = apr_table_make(p, 2);
    }

I think it may be a simple case of the 'if' clause being the wrong way around. Ie., should be:

    self = type->tp_alloc(type, 0);
    if (!self) {
        apr_pool_t *p;
        tableobject *t = (tableobject *)self;
        apr_pool_create_ex(&p, NULL, NULL, NULL);
        t->pool = p;
        t->table = apr_table_make(p, 2);
    }

In fact, since table_alloc() always returns a non NULL value, it could just be:

    self = type->tp_alloc(type, 0);

The issue now is what is the difference between tp_alloc and tp_new hooks in a Python type object object. I haven't used these in a while, so will have to research some more as to what is meant to occur in each.

[Graham Dumpleton]

Jim, can you test out this patch for the leak. It does away with tp_alloc altogether and tp_new simply calls MpTable_New(). I think this should be okay.

[Jim Gallacher]

Patch MP184-2006-08-25-grahamd-1.diff seems to fix the leak.

I don't see any leak for 100k requests. (Pre-patch would leak approx 50% of memory for 100k requests with the same test).

   [[ Old comment, sent by email on Tue, 15 Aug 2006 15:54:50 -0400 ]]

Jim,

This are my results for the memory leak search in apache.table().

The table object creates a memory pool by using apr_pool_create_ex()
and destroys the pool using apr_pool_destroy(). I added a line in
MpTable_New() before "return (PyObject*)t" to destroy the pool and
ran 1M iterations and I notice that there was no memory leak.
Therefore the apache functions seems to be working fine.

I couldn't fix the problem but here is a work around. In mod_python/
util.py instead of using apache.make_table() use a regular Python
dictionary. So the line that looks like:

headers = apache.make_table()

now looks like:

headers = {}

The apache table is basically used a Python dictionary. The only
functionality that is lost is that apache tables are case
insensitive, and that can be easily fixed by creating a class in
Python that inherits from dict type and override the __getitem__ and
__setitem__ methods.

For the moment I'm going to keep this changes until modpython.org
release a patch. I spent quite sometime trying to investigate and
solve the memory leak problem but the best I was able to do was to
work around it.

BTW, apache.table, apache.make_table or _apache.table is only being
used in mod_python/util.py.


/amn

[Jim Gallacher]

   [[ Old comment, sent by email on Wed, 16 Aug 2006 17:44:21 -0400 ]]


Actually I don't think apr_pool_destroy() in table_dealloc is actually
destroying the pool. I've been poking around in the code and there is
something odd going on here.

I tried registering a cleanup in MpTable_New() using:

apr_pool_cleanup_register(t->pool,
"pool cleanup called", cleanup_test,
apr_pool_cleaunp_null);

The cleanup_test callback just logs the "pool cleanup called" message to
a file.

apr_pool_destroy() is getting called in table_dealloc, but cleanup_test
never gets called which indicates that the pool is *not* being
destroyed, and hence our memory leak.

I tried your trick of immediately calling apr_pool_destroy in
MpTable_New(), and cleanup_test does get called there.

So, the big question is... why is the pool not being destroyed?

Can anyone offer some insight here?

The attached diff is for trunk if anyone wants to play around with it.

Jim
Index: tableobject.c
===================================================================
--- tableobject.c (revision 431994)
+++ tableobject.c (working copy)
@@ -59,6 +59,19 @@
     return (PyObject *)result;
 }
 
+
+
+apr_status_t cleanup_test(void *msg)
+{
+ FILE *f;
+ f = fopen("/tmp/debug_table.log", "a+");
+ fprintf(f, "%s\n", (char *)msg);
+ fclose(f);
+
+ return 0;
+}
+
+
 /**
  ** MpTable_New
  **
@@ -78,6 +91,8 @@
     tableobject *t;
     apr_pool_t *p;
 
+ cleanup_test("MpTable_New() called");
+
     /* XXX need second arg abort function to report mem error */
     apr_pool_create_ex(&p, NULL, NULL, NULL);
     
@@ -86,7 +101,12 @@
 
     /* remember the pointer to our own pool */
     t->pool = p;
+ apr_pool_cleanup_register(p, " pool cleanup called", cleanup_test, apr_pool_cleanup_null);
 
+ /* Uncomment this to test that cleanup_test is getting called correctly.
+ apr_pool_destroy(t->pool);
+ */
+
     return (PyObject *)t;
 
 }
@@ -99,10 +119,13 @@
 
 static void table_dealloc(register tableobject *self)
 {
+ cleanup_test("table_dealloc:");
 
     if (MpTable_Check(self)) {
- if (self->pool)
+ if (self->pool) {
+ cleanup_test(" preparing to destroy the pool");
             apr_pool_destroy(self->pool);
+ }
         PyObject_Del(self);
     }
     else