Memory leaks in Alsa backend

[druskus20]

I am finding quite a lot of memory leaks when using Alsa, both on master and 0.15.3.

==1297036== 1,512 bytes in 21 blocks are possibly lost in loss record 142 of 164
==1297036==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1297036==    by 0x48B3D92: ??? (in /usr/lib/x86_64-linux-gnu/libasound.so.2.0.0)
==1297036==    by 0x48C4C44: ??? (in /usr/lib/x86_64-linux-gnu/libasound.so.2.0.0)
==1297036==    by 0x48C5289: ??? (in /usr/lib/x86_64-linux-gnu/libasound.so.2.0.0)
==1297036==    by 0x48CFD84: snd_config_update_r (in /usr/lib/x86_64-linux-gnu/libasound.so.2.0.0)
==1297036==    by 0x48D0447: snd_config_update_ref (in /usr/lib/x86_64-linux-gnu/libasound.so.2.0.0)
==1297036==    by 0x48FB35F: snd_pcm_open (in /usr/lib/x86_64-linux-gnu/libasound.so.2.0.0)
==1297036==    by 0x4E95A4: alsa::pcm::PCM::open (error.rs:21)
==1297036==    by 0x4E94A0: alsa::pcm::PCM::new (pcm.rs:147)
==1297036==    by 0x4D9F04: cpal::host::alsa::DeviceHandles::try_open (mod.rs:214)
==1297036==    by 0x4DA099: cpal::host::alsa::DeviceHandles::take (mod.rs:233)
==1297036==    by 0x4DA372: cpal::host::alsa::Device::build_stream_inner (mod.rs:251)
==1297036==

This can be seen in some of the examples, for example feedback.

 valgrind -s --leak-check=full --show-leak-kinds=all ./target/debug/examples/feedback

Seems to be the same as this:

jarikomppa/soloud#278

[druskus20]

Looking a bit further into this I realized it is an issue with alsa-rs itself. I opened an issue there.

[druskus20]

Issue is fixed in alsa-rs - a simple upgrade whenever a new alsa_rs version is released will fix this issue

[bluenote10]

Thanks for looking into this, and the update on it!

Doesn't the fix in diwic/alsa-rs@e560e9d suggest that update_free_global now needs to be called pro-actively by cpal?

[druskus20]

Oh yeah, fair - sorry completely my bad (my PR is the one that fixed it automatically 😅)

We should either assume a reasonable default or allow the user to free the global config on demand

[git-staus]

Has the patch upstream ever been integrated back here or does cpal<=0.16.0 still leak memory? If so, how could users avoid it?

[druskus20]

I am not tracking this anymore - I since moved to pipewire.
Iirc the memory leak is not critical, and shouldn't grow over time

[git-staus]

It seems that your PR fixed all leaks in alsa-rs and was then sort of merged by the maintainer (?). The fix has not made it into an alsa-rs release yet but should be available in alsa > "0.9.1". cpal currently uses alsa = "0.9", so I guess this should be resolved as soon as upstream releases a new version.

I also inferred from the comments that the "leak" increases performance by not re-allocating something, so indeed not really a problem, like you said!

However, due to my limited understanding of the code I cannot make sense of @bluenote10's comment which seems to indicate that the fix by the maintainer did not include all commits from the PR (?) and therefore still requires a change in the cpal code?

[roderickvd]

My understanding is that this is not actually a leak, but alsa-rs caching the ALSA configuration until the process ends. We could flush the cache earlier by calling alsa::config::update_free_global.

First question is if that's a problem worth fixing.

If yes, then the second question is: "when?"

We probably want to keep the cache as long as we may be opening devices. We don't really know what cpal users are doing. Typical usage may be to open a device, play/record stuff and be done with it. But an equal valid and present use case is to close & reopen devices - which should remain fast especially if we're doing that to e.g. change sample rates during playback.

Exposing a public API for it, for when the user wants to flush the cache, seems undesirable and very specific to ALSA.

Conclusion could be that we should do it when Host is dropped. Now, Host is a type that can be constructed many times: it's not global and there can be many by simply calling cpal::default_host() multiple times. So this would require a static and atomic reference counter, to trigger update_free_global when we're dropping the last Host instance.

Again, is that worth it?

Finally, we should recognize that calling update_free_global may interfere when a user uses cpal and alsa-rs directly. Although probably that's a use case we can ignore.

[roderickvd]

Given the above, I think we should keep the configuration cache around and consider it's a false positive report of a memory leak.