Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.x, 3.2.10
    • Fix Version/s: 3.3.1
    • Component/s: core
    • Labels:
      None

      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()

        Activity

        Hide
        grahamd Graham Dumpleton added a comment -

        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.

        Show
        grahamd Graham Dumpleton added a comment - 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.
        Hide
        grahamd Graham Dumpleton added a comment -

        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.

        Show
        grahamd Graham Dumpleton added a comment - 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.
        Hide
        jgallacher Jim Gallacher added a comment -

        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).

        Show
        jgallacher Jim Gallacher added a comment - 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).
        Hide
        amarrero@mitre.org Alexis Marrero added a comment -

        [[ 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

        Show
        amarrero@mitre.org Alexis Marrero added a comment - [[ 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
        Hide
        jpg@jgassociates.ca Jim Gallacher added a comment -

        [[ 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

        Show
        jpg@jgassociates.ca Jim Gallacher added a comment - [[ 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

          People

          • Assignee:
            jgallacher Jim Gallacher
            Reporter:
            jgallacher Jim Gallacher
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development