Re: memory leaks (was Re: Commented: (MODPYTHON-172) Memory leak with util.fieldstorage using mod_python 3.2.8 on apache 2.0.55)

[Harold Ship <ha...@giant-steps-networks.com>]

Jim Gallacher <jpg <at> jgassociates.ca> writes:

> 
> I've run some tests to evaluate the memory leaks. The tests are brute
> force - make requests and watch the memory changes with top -b.
>
 
...

> First up - our leaky 3.2.9, and man does it leak! The readlines request
> has a body of 1M bytes, and it fails in a hurry.
> 

...

> Everything is looking much better, with readlines looking very stable.
> Cfgtree_walk and parse_qsl are also looking much better but there may
> still be a problem. To confirm this I ran the tests against 3.2.x build
> 20060709 again, but with 100,000 requests.
> 

Did you have a chance to re-run the tests?

There are some things I've found, although I don't think they cause a leak.
For example, in parse_qs() there is:

        pair = PyString_FromStringAndSize(NULL, len);
        if (pair == NULL)
            return NULL;

this should be:

        pair = PyString_FromStringAndSize(NULL, len);
        if (pair == NULL) {
            Py_DECREF(pairs);
            return NULL;
        }

There are a few more like that in parse_qs() and parse_qsl(), and cfgtree_walk().

In req_readlines(), line 810/811 the intention isn't clear of:

    if (result == NULL)
        return PyErr_NoMemory();

While the exact same code on lines 814/815 are not correct.
They should be:

    if (rlargs == NULL) {     // note rlargs not result
        Py_DECREF(result);
        return PyErr_NoMemory();
    }

Re: memory leaks (was Re: Commented: (MODPYTHON-172) Memory leak with util.fieldstorage using mod_python 3.2.8 on apache 2.0.55)

[Jim Gallacher <jp...@jgassociates.ca>]

Harold Ship wrote:
> Jim Gallacher <jpg <at> jgassociates.ca> writes:
> 
>> I've run some tests to evaluate the memory leaks. The tests are brute
>> force - make requests and watch the memory changes with top -b.
>>
>  
> ...
> 
>> First up - our leaky 3.2.9, and man does it leak! The readlines request
>> has a body of 1M bytes, and it fails in a hurry.
>>
> 
> ...
> 
>> Everything is looking much better, with readlines looking very stable.
>> Cfgtree_walk and parse_qsl are also looking much better but there may
>> still be a problem. To confirm this I ran the tests against 3.2.x build
>> 20060709 again, but with 100,000 requests.
>>
> 
> Did you have a chance to re-run the tests?

I ran my test using FieldStorage but haven't had a chance to go further.
For 500k requests memory consumption increases from 1.1% to 10%
(mpm-worker, StartServers = 2) so it still leaks. This is still an
improvement as the machine would have crumpled far before the 500k
request mark, but a leak is a leak. I assume the main culprit in
FieldStorage is parse_qsl, but I don't see any obvious problem.

> There are some things I've found, although I don't think they cause a leak.
> For example, in parse_qs() there is:
> 
>         pair = PyString_FromStringAndSize(NULL, len);
>         if (pair == NULL)
>             return NULL;
> 
> this should be:
> 
>         pair = PyString_FromStringAndSize(NULL, len);
>         if (pair == NULL) {
>             Py_DECREF(pairs);
>             return NULL;
>         }
> 
> There are a few more like that in parse_qs() and parse_qsl(), and cfgtree_walk().
> 
> In req_readlines(), line 810/811 the intention isn't clear of:
> 
>     if (result == NULL)
>         return PyErr_NoMemory();

I agree it would be clearer if the result object creation was
immediately before the test. eg

     PyObject *result = PyList_New(0);

     ...

     result = PyList_New(0);
     if (result == NULL)
         return PyErr_NoMemory()

> While the exact same code on lines 814/815 are not correct.
> They should be:
> 
>     if (rlargs == NULL) {     // note rlargs not result
>         Py_DECREF(result);
>         return PyErr_NoMemory();
>     }

(Good catch on the rlargs thing).

I think these are mainly just coding inconsistencies, and agree that
they are should not be the cause of the current memory leaks. If one of
the object creation functions fails due to lack of memory, our failure
to decrement a previously created object is hardly going to matter.
Sloppy programming? - for sure.

The use of PyErr_NoMemory() in the above code fragments does concern me
a little though.

PyErr_NoMemory() is shorthand for PyErr_SetNone(PyExc_MemoryError) and
returns NULL. If a call such as PyList_New(0) fails, doesn't it have the
responsibility to set the exception, and our calling function should
simply return NULL? I had the idea that we should only call
PyErr_NoMemory if we are doing a malloc in our code and it fails. Is
there something unique about PyList_New and PyTuple_New that I'm missing?

By the way Harold, it would be helpful if when talking about specific
code lines of code if you were referencing the current development in
svn trunk. Generally we've been making changes in trunk, and backporting
to branches/3.2.x as required. Anyway, thanks for your continuing
efforts. :)

Jim