Memory leak in vector_read for pyxrootd

[nikoladze]

There seems to be a memory leak in the vector_read function in the python bindings. To reproduce:

# debug_vector_read.py
import gc
import pyxrootd.client

filename = "root://eospublic.cern.ch//eos/opendata/atlas/OutreachDatasets/2020-01-22/4lep/MC/mc_345060.ggH125_ZZ4lep.4lep.root"

def test():
    f = pyxrootd.client.File()
    res = f.open(filename)
    if not res[0]["ok"]:
        raise IOError(res)
    chunksize = 1024 * 256
    chunks = [(i * chunksize, chunksize) for i in range(5)]
    res = f.vector_read(chunks)
    print(res[0]["message"])
    f.close()

for i in range(20):
    print(i)
    test()
    gc.collect()

When running this through memory-profiler with

mprof run debug_vector_read.py
mprof plot

i get this plot:

(Tested with xrootd 5.0.3 from conda-forge)

It seems the problem are these buffers which are never deleted:

xrootd/bindings/python/src/PyXRootDFile.cc

Line 611 in 551521e

char *buffer = new char[length];

Not sure if or when it is allowed to delete them, but if i collect the buffers into an array and delete them after the call to self->file->VectorRead(...) the memory leak seems to be gone:

[nikoladze]

Another suggestions of type "i don't know what i'm doing": Maybe the delete has to be here after this in the conversion for VectorReadInfo

xrootd/bindings/python/src/Conversions.hh

Line 271 in 551521e

PyObject *buffer = PyBytes_FromStringAndSize( (const char *) chunk.buffer,

analogous to what is done here for ChunkInfo:

xrootd/bindings/python/src/Conversions.hh

Lines 248 to 257 in 551521e

	template<> struct PyDict<XrdCl::ChunkInfo>
	{
	static PyObject* Convert( XrdCl::ChunkInfo *chunk )
	{
	PyObject *o = PyBytes_FromStringAndSize( (const char*)chunk->buffer,
	chunk->length );
	delete[] (char*) chunk->buffer;
	return o;
	}
	};

[nsmith-]

Would it be better to allocate using a python allocator rather than malloc here? Also there might be an opportunity for copy elision if we can pass a set of pointers into vector_read to be filled.

[nsmith-]

But yeah looking at https://docs.python.org/3/c-api/bytes.html#c.PyBytes_FromStringAndSize that's a copy, and as far as I can see the original pointer gets forgotten. I wonder why it doesn't just use the existing ChunkInfo convert routine?

[simonmichal]

Thanks guys for reporting it, I will have a look!