Free pa_context on close()

[avian2]

This is an attempt to fix a memory leak in Pulse class - memory allocated for Pulseaudio context wasn't freed after the connection was closed.

To reproduce, run this and observe RSS of Python process:

import pulsectl
from time import sleep

def main():
    while True:
        pulse = pulsectl.Pulse()
        sleep(1)
        pulse.close()

main()

[mk-fg]

Ah, indeed, seem to be a rookie mistake of forgetting to free the thing. Apparently there from the start, so probably copied it like that from pulsemixer code (maybe mangling in the process) and never revisited. Thanks!

[mk-fg]

Also thought that it'd be cool to add memleak-check wrapper around every test, but it just looks too complicated and boring of a task with python and ctypes allocations interleaved like they are here.

[mk-fg]

...or actually, maybe not, as it's libpulse that allocates everything relevant here, so just need to tell valgrind to track only that. Will look into it a bit more.

[avian2]

Thanks for the very quick response! I didn't look into unittests because I couldn't get them to run at all (Debian Stretch, pulseaudio 10.0-1+deb9u1). There seem to be some problem in starting and configuring the test pulseaudio instance on my system.

[mk-fg]

Huh, weird, though knowing debian, suspect they might just have daemon binary in some weird place. No worries though, doubt these tests are of much use to anyone but me :)

[mk-fg]

While never figured-out valgrind stuff (seem to be missing mallocs from libpulse, but catching them if you do it via ctypes directly), did find a bug in the patch - pa_context_unref should be called after pa_mainloop_free, as otherwise you get assertion aborts like these:

Assertion '!m->rebuild_pollfds' failed at pulse/mainloop.c:841, function pa_mainloop_poll(). Aborting.

(as pa_mainloopfree still does some cleanup*_events stuff, which apparently uses context)

Not sure why pulse refcounting doesn't handle this properly, but would suggest updating if you have any possibility of close() with pending pulse stuff in the code (e.g. close() on exceptions, events, etc), as it'd get sudden SIGABRT there from libpulse.

[mk-fg]

find a bug in the patch - pa_context_unref should be called after pa_mainloop_free

Looking a bit more into it, found that nope, your original ordering was correct, and that assert error is due to race condition only when using threads, sorry for confusion.