systemd / pystemd

A thin Cython-based wrapper on top of libsystemd, focused on exposing the dbus API via sd-bus in an automated and easy to consume way.
GNU Lesser General Public License v2.1
411 stars 36 forks source link

Memory leak in pystemd #6

Closed dreuter closed 6 years ago

dreuter commented 6 years ago

First of all thanks for this great project.

Unfortunately we are unable to use it, since it leaks memory whenever we are accessing the ActiveState of a Unit (I haven't tested any other property yet).

Here is a minimal example to show the issue:

#!/usr/bin/env python3
from pystemd.systemd1 import Unit
import os
import psutil

if __name__ == '__main__':
    unit = Unit(b'ntpd.service')
    unit.load()
    while True:
        if unit.Unit.ActiveState == 'active':
            pass
        process = psutil.Process(os.getpid())
        print(process.memory_info().rss)

When I run this example the memory usage raises drastically. It seems to be in the C/Cython land (at least that was my conclusion after a little bit of investigation). This problem was reproduceable with different services on different machines and different flavours of Linux, as well as on python2 and python3.

aleivag commented 6 years ago

Thanks @dreuter for submitting this, i will try it myself and see if i find the memory leak.

would you mind specify systemd version and os version that you used?

Thanks!

aleivag commented 6 years ago

just checked and yes there is definetly a memory grow

dreuter commented 6 years ago

I tested it on a custom yocto linux (rocko), Ubuntu 16.04/18.04 and Arch Linux.

On my Arch laptop the systemd version is:

$ systemctl --version
systemd 238
+PAM -AUDIT -SELINUX -IMA -APPARMOR +SMACK -SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ +LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD -IDN2 +IDN +PCRE2 default-hierarchy=hybrid

However I am inclined to think, that it does not depend on the exact version. If you need any further information feel free to ask me.

aleivag commented 6 years ago

@dreuter Thanks!!!, yeah i agree with you, i found out some pointers that we did not dereference, i already commited one of the fixes for one method, fixes will start flowing soon, will inform this issue with progress...

Thanks again for reporting!

aleivag commented 6 years ago

@dreuter i landed a change that i think fix the issue you reported

to test i did:

 ~ > cd /tmp 
 /tmp > python -m venv new 
 /tmp > cd new 
 /tmp/new > source bin/activate
(new)  /tmp/new > pip install cython psutil
....
(new)  /tmp/new > mkdir src
(new)  /tmp/new > cd src 
(new)  /tmp/new/src > git clone https://github.com/facebookincubator/pystemd
...
(new)  /tmp/new/src > cd pystemd 
(new)  /tmp/new/src/pystemd > python setup.py install
...
(new)  /tmp/new/src/pystemd > cd ../..
(new)  /tmp/new  vim test.py
(new)  /tmp/new  python test.py
16326656
16326656
16326656
16326656
16326656
...
16326656
16326656
16326656
16326656
16326656

where test.py is your same script...

can you please check on your end if this is fix for you?... doing some extra digging i found out some other places where we could improve memory management, but they should not affect your immediate usecase.

aleivag commented 6 years ago

all the test we have done indicate that this leak is gone... i'll close the issue, but i'll love to hear your results ...

:D

dreuter commented 6 years ago

@aleivag I tested the changes on my machine as well and it seems to not leak any more.

Thank you very much for resolving this issue in such a timely manner