morphx666 / CoreAudio

Windows CoreAudio wrapper for .NET
https://whenimbored.xfx.net/2011/01/core-audio-for-net/
MIT License
95 stars 20 forks source link

Bug: possible memory leak #27

Closed amadeo-alex closed 4 months ago

amadeo-alex commented 7 months ago

Hello,

I've been experimenting with various "CoreAudio" wrapper libraries and it looks like most of them, including this one, appear to be leaking memory - slowly but constantly (or it's me using the library wrong, not excluding that :D)

I have a memory dump created at the end of the tests however github allows attachments only up to 25MB - compressed dump is 30MB+. I can deliver it via preferred method if required.

Test project is a clean new one with CoreAudio installed via nuget. Code used for testing (put directly into main function, please ignore the var name typo):

        var enumerator = new MMDeviceEnumerator(Guid.Empty);
        while (true)
        {
            foreach (var device in enumerator.EnumerateAudioEndPoints(DataFlow.All, DeviceState.Active))
            {
                var termpVar = device.DeviceFriendlyName;
                device.Dispose();
            }
        }

Results: oJeViP OvLLpo Yk0j7R 2SNesS

morphx666 commented 7 months ago

Yes, continuously calling enumerator.EnumerateAudioEndPoints will cause memory consumption to go up. Not sure if it's a problem with the library or the native code itself.

However, it is not recommended to do so. Instead, in order to keep an updated list of devices you should use MMNotificationClient and listen for notifications (events).

Something like this:

using CoreAudio;

internal class Program {
    private static void Main(string[] args) {
        var devices = new List<MMDevice>();
        var enumerator = new MMDeviceEnumerator(Guid.Empty);

        foreach(var device in enumerator.EnumerateAudioEndPoints(DataFlow.All,DeviceState.Active)) {
            devices.Add(device);
        }

        var nc = new MMNotificationClient(enumerator);
        nc.DeviceAdded += (sender, e) => {
            lock(devices) {
                MMDevice device = enumerator.GetDevice(e.DeviceId);
                devices.Add(device);
            }
        };
        nc.DeviceRemoved += (sender, e) => {
            lock(devices) {
                MMDevice device = enumerator.GetDevice(e.DeviceId);
                devices.Remove(device);
            }
        };
        nc.DeviceStateChanged += (sender, e) => {
            // What to do here is really up to you...
            lock(devices) {
                MMDevice device = enumerator.GetDevice(e.DeviceId);
                if((e.DeviceState & DeviceState.Active) == DeviceState.Active) {
                    devices.Add(device);
                } else {
                    devices.Remove(device);
                }
            }
        };

        Task.Run(async () => {
            while(true) {
                lock(devices) {
                    foreach(var device in devices) {
                        var deviceName = device.DeviceFriendlyName;

                        // Not required because we never called GetAudioEndpointVolume or GetAudioSessionManager2
                        //device.Dispose(); 

                        // Do something with the device's name...
                    }
                }

                await Task.Delay(10);
            }
        }).Wait();
    }
}

Please avoid using lock the way I did. Use a thread-safe collection instead.

amadeo-alex commented 7 months ago

Thank very much for a quick answer! I really like the proposed solution with events, didn't know it can be done this (cleaner) way. I'll play with it, this library is one currently used in the project I'm maintaining so I'd be really glad to keep it.

amadeo-alex commented 7 months ago

Is session handling supposed to be done in similar fashion - OnSessionCreated? I've created a simple example based on the code above that iterates the sessions and I can still see memory usage creeping up slowly despite disposing "everything". Code (put instead of the Task from your example above):

        while (true)
        {
            lock (devices)
            {
                foreach (var device in devices)
                {
                    var deviceName = device.DeviceFriendlyName;
                    var sessionManager = device.AudioSessionManager2;
                    if(sessionManager == null)
                        continue;

                    var sessions = sessionManager.Sessions;
                    if (sessions == null)
                        continue;

                    foreach (var session in sessions)
                    {
                        var s = session;
                        s.Dispose();
                    }

                    sessionManager.Dispose();
                    device.Dispose();
                }
            }
        }
amadeo-alex commented 7 months ago

I haven't confirmed that yet but it looks like even the basic setup with events leak a bit of memory? Not certain at this point, I'll let it run for a few hours and report back.

amadeo-alex commented 7 months ago

I'll be honest, I'm confused at this point :D Code:

internal class Program
{
    private static ConcurrentDictionary<string, MMDevice> _devices = [];

    private static void AddDevice(MMDevice device)
    {
        Console.WriteLine($"adding device: {device.DeviceFriendlyName}");
        _devices[device.DeviceFriendlyName] = device;
    }

    private static void RemoveDevice(MMDevice device)
    {
        Console.WriteLine($"removing device: {device.DeviceFriendlyName}");
        _devices.Remove(device.DeviceFriendlyName, out var removedDevice);
        removedDevice?.Dispose();
        device?.Dispose();
    }

    static void Main(string[] args)
    {
        var enumerator = new MMDeviceEnumerator(Guid.Empty);

        foreach (var device in enumerator.EnumerateAudioEndPoints(DataFlow.All, DeviceState.Active))
        {
            AddDevice(device);
        }

        var nc = new MMNotificationClient(enumerator);
        nc.DeviceAdded += (sender, e) =>
        {
            var device = enumerator.GetDevice(e.DeviceId);
            AddDevice(device);
        };
        nc.DeviceRemoved += (sender, e) =>
        {
            var device = enumerator.GetDevice(e.DeviceId);
            RemoveDevice(device);
        };
        nc.DeviceStateChanged += (sender, e) =>
        {
            var device = enumerator.GetDevice(e.DeviceId);
            if ((e.DeviceState & DeviceState.Active) == DeviceState.Active)
            {
                AddDevice(device);
            }
            else
            {
                RemoveDevice(device);
            }
        };

        Console.WriteLine("---------------------------------------");
        foreach (var device in _devices.Values)
        {
            Console.WriteLine($"device: {device.DeviceFriendlyName}");
        }
        Console.WriteLine("---------------------------------------");

        while (true)
        {

        }
    }
}

Information from VS: UypX4o jGNycO 7Ag0YK

Opposing information from Task Manager: MBjQ8g

morphx666 commented 7 months ago

At first glance, your code looks fine... I will run it locally to see if I can figure out what's going on.

morphx666 commented 7 months ago

To be honest, I'm not sure what's going on... but I feel that this appears to be normal behavior.

I followed the instructions found here and couldn't find anything useful.

After all, the app sits there, doing nothing in an infinite loop... there's no chance that the CoreAudio library is leaking memory if it's not being used.

Here's one test you can do... remove the code related to CoreAudio and run the app with just the infinite loop and you will see that, although not as fast, the process memory slowly, but steadily, keeps growing.

image

Maybe someone with a deeper understanding of dotnet's inner workings (I'm thinking of @Elfocrash) could shed some light on this issue...

amadeo-alex commented 7 months ago

Ohhh, that is more than interesting! Thank you for your tests and investigation - I'll try that right away.

amadeo-alex commented 7 months ago

You seem to be quite correct (it started at 12MB): 4OD52h This kind of behaviour is very interesting and confusing at the same time :D

morphx666 commented 7 months ago

Consider this code:

internal class Program {
    private static void Main(string[] args) {
        Task.Run(async () => {
            while(true) {
                await Task.Delay(10);
            }
        }).Wait();
    }
}

I just asked CoPilot why this code is leaking memory... the answer is just what one would've expected:

The code you've provided is a simple infinite loop that waits for 10 milliseconds on each iteration. It doesn't seem to be leaking memory directly, as it doesn't allocate any unmanaged resources or create any objects that aren't being cleaned up.

However, if this code is part of a larger program, it could potentially contribute to a memory leak if it's preventing other tasks from completing or if it's keeping some objects alive longer than necessary.

For example, if this loop is running on the main thread and there are other tasks that are waiting to run on the main thread, those tasks won't be able to complete until this loop finishes. If those tasks are holding onto some resources, those resources won't be freed until the tasks complete, which could lead to a memory leak.

Also, if there are any local variables in the Main method that are being captured by the lambda expression, those variables will be kept alive for as long as the loop is running, which could also lead to a memory leak.

Without more context, it's hard to say for sure why your program might be leaking memory. You might want to use a memory profiler to get a better idea of what's going on.

However, it got me thinking: what if we are blocking some internal dotnet process? After all, the loop lives inside the main program thread/loop.

So, changing the code slightly, running the infinite loop on a separate thread seems to solve the problem:

internal class Program {
    private static void Main(string[] args) {
        Thread t = new(() => {
            while(true) {
                Thread.Sleep(10);
            }
        });
        t.Start();
    }
}

Another thing is that running this in VS still presents the apparent memory leak. But running it with dotnet run -c Release doesn't, so I guess VS is also to blame for this behavior.

amadeo-alex commented 7 months ago

Your observations seems to be again quite correct + looks like abusing GC a little bit causes the memory to be constant (started at 16): oSPQua