[SciPy-Dev] Enhancements to scipy.spatial.cKDTree

[Sturla Molden]

On 13.07.2012 13:02, Sturla Molden wrote:

> ** Make sure malloc and free does not leak memory in case of a Python
> exception. This means either using try/finally religiously or wrap it in
> a Python object that use "RAII" with __cinit__, __init__ and
> __dealloc__. Currently it is littered with possible memory leaks.
>
> ** Raise MemoryError on malloc failure.

That is, it is NOT sufficient to always pair malloc and free as we can 
do in C, since Python (like C++) can raise exceptions. A Python 
exception in Cython can separate a malloc/free pair.

That is the same issue that mandates the use of RAII in robust C++ code.

This idiom is currently used all over cKDTree:

ptr = stdlib.malloc(n)
<suite>
stdlib.free(ptr)

It probably looks ok to C programmers and inexperienced C++ programmers, 
but it is not. If <suite> raises any Python exception, we can have a 
memory leak.

The other idioms that should be used instead.

Either we can use try/finally religiously in the code, which makes it 
hard to read and maintain:

cdef void *ptr = NULL # intialize to NULL outside try/finally
try:
    ptr = cpython.PyMem_Malloc(n)
    if ptr == NULL:
       raise MemoryError
    <suite>
finally:
    if ptr != NULL:
       cpython.PyMem_Free(ptr)


RAII-like coding with a Python extension object is better (IMO):

cdef class buffer:

    cdef void *ptr

    def __cinit__(buffer self, Py_ssize_t n):
       self.ptr = NULL

    def __init__(buffer self, Py_ssize_t n):
       self.ptr = cpython.PyMem_Malloc(n)
       if (self.ptr == NULL):
          raise MemoryError

    def __dealloc__(buffer self):
       if self.ptr != NULL:
           cpython.PyMem_Free(self.ptr)

    def __enter__(buffer self):
       return cpython.PyLong_FromVoidPtr(self.ptr)

    def __exit__(buffer self, type, value, traceback):

       # Tear-down is strictly speaking not required in
       # Cython, inlike in Python, since __dealloc__
       # behaves differently from __del__.

       if self.ptr != NULL:
           cpython.PyMem_Free(self.ptr)
           self.ptr = NULL

       # pass on all exceptions 	
       return (type is None)


Now we can safely do this:

    cdef buffer buf = buffer(n)
    cdef void *ptr = buf.ptr
    <suite>
    # ptr is invalid when reference to
    # buf is lost ... about here ...


or preferably:

    cdef void *ptr
    cdef object addr

    with buffer(n) as addr:
       ptr = cpython.PyLong_AsVoidPtr(addr)
       <suite>

    # ptr is invalid here


If multiple buffers are needed, then we can e.g. use contextlib.nested 
to avoid nested with blocks:


    cdef void *ptr1
    cdef void *ptr2
    cdef object addr1
    cdef object addr2

    with nested(buffer(n1),buffer(n2)) as (addr1,addr2):
       ptr1 = cpython.PyLong_AsVoidPtr(addr1)
       ptr2 = cpython.PyLong_AsVoidPtr(addr2)
       <suite>

    # ptr1,ptr2 are invalid here


I believe the issue of exceptions and malloc/free is not sufficiently 
know among scientific users of Cython, so we might put this on the 
Cookbook wiki as well.




Sturla