Memory leak python version

[Hu6li]

Description

While we were checking to use ggwave in our project, we observed memory leaks using valgrind.

Circumstances

When ggwave is imported in a python script and run against python (valgrind enabled python version), we observed these leaks.

Steps to reproduce

1. Download Python from the original website
   We used python version 3.9.2

2. Compile Python using the following flags:
   ./configure --prefix=/check_ggwave/python/ --without-pymalloc --with-pydebug --with-valgrind
   make OPT=-g
   make install

3. Creating venv using new compiled python in /check_ggwave/python/python3
   python3 -m venv .venv
   . .venv/bin/activate
   pip3 install ggwave

4. Cloning repo of ggwave and running valgrind on test-ggwave.py:
   test-ggwave: https://github.com/ggerganov/ggwave/blob/master/tests/test-ggwave.py
   valgrind --leak-check=full --log-file="valgrind_report.txt" python test-ggwave.py

Results

For more details please execute the steps above and check your output, here is a summary:

==18560== LEAK SUMMARY:
==18560== definitely lost: 1,800 bytes in 9 blocks
==18560== indirectly lost: 568 bytes in 6 blocks
==18560== possibly lost: 790,082 bytes in 6,445 blocks
==18560== still reachable: 84,547 bytes in 18 blocks
==18560== suppressed: 48 bytes in 1 blocks

Observations

During our tests we observed that there are no leaks using the C or C++ implementation. Feel free to confirm this on your own system(s) as well.

Conclusion

Based on our analysis we suspect that the cause of these leaks lays in the way ggwave's python implementation is generated (bindings).
But this is only an assumption!

[ggerganov]

@Hu6li
Thanks for the detailed report!

I confirm that I can reproduce the memory leak reported by valgrind.
The strange thing is that even if I have only the import ggwave line in the python script, there is still the same leak present:

import ggwave

So it seems to be something related to the Cython wrapper. Probably I am not using it correctly.

My experience with Python/Cython is very basic, so I don't think I will be able to trace the root cause.
Hopefully, someone more experienced can help identify the problem.

[ioerror]

Hello, a little bird who doesn't have a Github account asked me to relay a fix for ggwave's binding code:

diff --git a/bindings/python/Makefile b/bindings/python/Makefile
index e277e22..679e040 100644
--- a/bindings/python/Makefile
+++ b/bindings/python/Makefile
@@ -7,7 +7,7 @@ ggwave:
 
 pyggwave.bycython.cpp: ggwave.pyx cggwave.pxd
        python -m pip install cython
-       cython --cplus ggwave.pyx -o ggwave.bycython.cpp
+       cython --cleanup=3 --cplus ggwave.pyx -o ggwave.bycython.cpp
 
 # To build package, README.rst is needed, because it goes into long description of package,
 # which is what is visible on PyPI.

To confirm that this works - the patch from cython/cython#3994 was applied then it is possible to run:

PYTHONMALLOC=malloc valgrind --tool=memcheck --leak-check=full --suppressions=/path/to/pythonsrc/python3-defaults-3.11.1/debian/valgrind-python.supp --log-file=valgrind_report.txt --show-leak-kinds=definite python3 tests/test-ggwave.py

Essentially the bindings did not cleanup at module unload time. The above should fix it, but I haven't personally verified it, I am merely the messenger.

[ggerganov]

This indeed fixes the leak - thank you very much for the help!
The new python package ggwave v0.4.2 is now up