Memory leak in anonymous function

Question

TLDR;

Non-trivial memory leak, can be seen easily in Resharper. See minimal example below.

---

I'm seeing a memory leak in the following program but failing to see why.

The program sends pings to a number of hosts asynchronously and determines if at least one is ok. To do that, a method (SendPing()) that runs these async operations is repeatedly called which runs them in a background thread (it doesn't have to, but in the actual application SendPing() will be called by the main UI thread which shouldn't be blocked).

The task seems pretty trivial but I think the leak occurs due to the way I create lambdas inside the SendPing() method. The program can be changed to not use lambdas but I'm more interested in understanding what causes the leak here.

public class Program
{

    static string[] hosts = { "www.google.com", "www.facebook.com" };

    static void SendPing()
    {
        int numSucceeded = 0;
        ManualResetEvent alldone = new ManualResetEvent(false);

        ManualResetEvent[] handles = new ManualResetEvent[hosts.Length];
        for (int i = 0; i < hosts.Length; i++)
            handles[i] = new ManualResetEvent(false);

        BackgroundWorker worker = new BackgroundWorker();
        worker.DoWork += (sender, args) =>
        {
            numSucceeded = 0;
            Action<int, bool> onComplete = (hostIdx, succeeded) =>
            {
                if (succeeded) Interlocked.Increment(ref numSucceeded);
                handles[hostIdx].Set();
            };

            for (int i = 0; i < hosts.Length; i++)
                SendPing(i, onComplete);

            ManualResetEvent.WaitAll(handles);
        };

        worker.RunWorkerCompleted += (sender, args) =>
        {
            Console.WriteLine("Succeeded " + numSucceeded);
            BackgroundWorker bgw = sender as BackgroundWorker;
            alldone.Set();
        };

        worker.RunWorkerAsync();
        alldone.WaitOne();
        worker.Dispose();
    }

    static void SendPing(int hostIdx, Action<int, bool> onComplete)
    {
        Ping pingSender = new Ping();
        pingSender.PingCompleted += (sender, args) =>
        {
            bool succeeded = args.Error == null && !args.Cancelled && args.Reply != null && args.Reply.Status == IPStatus.Success;
            onComplete(hostIdx, succeeded);
            Ping p = sender as Ping;
            p.Dispose();
        };

        string data = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
        byte[] buffer = Encoding.ASCII.GetBytes(data);
        PingOptions options = new PingOptions(64, true);
        pingSender.SendAsync(hosts[hostIdx], 2000, buffer, options, hostIdx);
    }

    private static void Main(string[] args)
    {
        for (int i = 0; i < 1000; i++)
        {
            Console.WriteLine("Send ping " + i);
            SendPing();
        }
    }
}

Resharper shows the leaks are due to uncollected closure objects (c__DisplayClass...).

From what I understand, there shouldn't be a leak because there are no circular references (as far as I see) and therefore GC should take of the leaks. I also call Dispose to release the thread (bgw) + sockets (Ping object) promptly. (Even if I didn't GC will clean them up eventually, won't it?)

Suggested changes from comments

• Remove event handles before Disposing
• Dispose ManualResetEvent

But the leak is still there!

Changed program:

public class Program
{

    static string[] hosts = { "www.google.com", "www.facebook.com" };

    static void SendPing()
    {
        int numSucceeded = 0;
        ManualResetEvent alldone = new ManualResetEvent(false);

        BackgroundWorker worker = new BackgroundWorker();
        DoWorkEventHandler doWork = (sender, args) =>
        {
            ManualResetEvent[] handles = new ManualResetEvent[hosts.Length];
            for (int i = 0; i < hosts.Length; i++)
                handles[i] = new ManualResetEvent(false);

            numSucceeded = 0;
            Action<int, bool> onComplete = (hostIdx, succeeded) =>
            {
                if (succeeded) Interlocked.Increment(ref numSucceeded);
                handles[hostIdx].Set();
            };

            for (int i = 0; i < hosts.Length; i++)
                SendPing(i, onComplete);

            ManualResetEvent.WaitAll(handles);
            foreach (var handle in handles)
                handle.Close();

        };

        RunWorkerCompletedEventHandler completed = (sender, args) =>
        {
            Console.WriteLine("Succeeded " + numSucceeded);
            BackgroundWorker bgw = sender as BackgroundWorker;
            alldone.Set();
        };

        worker.DoWork += doWork;
        worker.RunWorkerCompleted += completed;

        worker.RunWorkerAsync();
        alldone.WaitOne();
        worker.DoWork -= doWork;
        worker.RunWorkerCompleted -= completed;
        worker.Dispose();
    }

    static void SendPing(int hostIdx, Action<int, bool> onComplete)
    {
        Ping pingSender = new Ping();
        PingCompletedEventHandler completed = null;
        completed = (sender, args) =>
        {
            bool succeeded = args.Error == null && !args.Cancelled && args.Reply != null && args.Reply.Status == IPStatus.Success;
            onComplete(hostIdx, succeeded);
            Ping p = sender as Ping;
            p.PingCompleted -= completed;
            p.Dispose();
        };

        pingSender.PingCompleted += completed;

        string data = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
        byte[] buffer = Encoding.ASCII.GetBytes(data);
        PingOptions options = new PingOptions(64, true);
        pingSender.SendAsync(hosts[hostIdx], 2000, buffer, options, hostIdx);
    }


    private static void Main(string[] args)
    {
        for (int i = 0; i < 1000; i++)
        {
            Console.WriteLine("Send ping " + i);
            SendPing();
        }
    }
}

Answer 1

There is no memory leak. dotMemory that you use analyzes the snapshots and indeed, in the context of one snapshot the auto-generated class created by the compiler for the completed event handler will still be in memory. Rewrite your main application like this:

private static void Main(string[] args)
{
    for (int i = 0; i < 200; i++)
    {
        Console.WriteLine("Send ping " + i);
        SendPing();
    }

    Console.WriteLine("All done");
    Console.ReadLine();
}

Run the profiler, allow the application to reach the point where it outputs "All done", wait a few seconds and take a new snapshot. You will see there is no longer any memory leak.

It is worth mentioning that the class generated by the compiler for the PingCompleted event handler (that is c_DisplayClass6) will linger in memory after the method static void SendPing(int hostIdx, Action<int, bool> onComplete) exits. What happens there is that when pingSender.PingCompleted += (sender, args) =>... is executed the pingSender instance will take a reference to c_DisplayClass6. During the call to pingSender.SendAsync, the framework will retain a reference to pingSender in order to deal with running the async method and its completion. The async method you initiate by calling pingSender.SendAsync still runs when method SendPing exits. Because of that pingSender will survive a little while longer, hence c_DisplayClass6 will survive a little while longer too. However, after the pingSender.SendAsync operation completes, the framework will release its references to pingSender. At this point both pingSender and c_DisplayClass6 become garbage collectable and eventually the garbage collector will collect them. You can see this if you take a last snapshot like I was mentioning above. In that snapshot dotMemory will no longer detect a leak.

Answer 2

ManualResetEvent implements Dispose(). You are instantiating a number of ManualResetEvents and never calling dispose.

When an object implements dispose you need to call it. If you do not call it, there'll quite likely be memory leaks. You should use using statements, and try finally to dispose objects Simarly you should also have a using statement around Ping.

EDIT: This may be useful....

When should a ManualResetEvent be disposed?

EDIT: As stated here...

https://msdn.microsoft.com/en-us/library/498928w2(v=vs.110).aspx

When you create objects that include unmanaged resources, you must explicitly release those resources when you finish using them in your app.

EDIT: As stated here...

https://msdn.microsoft.com/en-us/library/system.threading.manualresetevent(v=vs.100).aspx

Dispose() Releases all resources used by the current instance of the WaitHandle class. (Inherited from WaitHandle.)

The ManualResetEvent has unmanaged resources associated with it, which is fairly typical of most of the classes in the .NET Framework libraries which implements IDisposable.

EDIT: Try using this...

public class Program
{
    static string[] hosts = { "www.google.com", "www.facebook.com" };

    static void SendPing()
    {
        int numSucceeded = 0;
        using (ManualResetEvent alldone = new ManualResetEvent(false))
        {
            BackgroundWorker worker = null;
            ManualResetEvent[] handles = null;
            try
            {
                worker = new BackgroundWorker();
                DoWorkEventHandler doWork = (sender, args) =>
                {
                    handles = new ManualResetEvent[hosts.Length];
                    for (int i = 0; i < hosts.Length; i++)
                        handles[i] = new ManualResetEvent(false);

                    numSucceeded = 0;
                    Action<int, bool> onComplete = (hostIdx, succeeded) =>
                    {
                        if (succeeded) Interlocked.Increment(ref numSucceeded);
                        handles[hostIdx].Set();
                    };

                    for (int i = 0; i < hosts.Length; i++)
                        SendPing(i, onComplete);

                    ManualResetEvent.WaitAll(handles);
                    foreach (var handle in handles)
                        handle.Close();

                };

                RunWorkerCompletedEventHandler completed = (sender, args) =>
                {
                    Console.WriteLine("Succeeded " + numSucceeded);
                    BackgroundWorker bgw = sender as BackgroundWorker;
                    alldone.Set();
                };

                worker.DoWork += doWork;
                worker.RunWorkerCompleted += completed;

                worker.RunWorkerAsync();
                alldone.WaitOne();
                worker.DoWork -= doWork;
                worker.RunWorkerCompleted -= completed;
            }
            finally
            {
                if (handles != null)
                {
                    foreach (var handle in handles)
                        handle.Dispose();
                }
                if (worker != null)
                    worker.Dispose();
            }
        }
    }

    static void SendPing(int hostIdx, Action<int, bool> onComplete)
    {
        using (Ping pingSender = new Ping())
        {
            PingCompletedEventHandler completed = null;
            completed = (sender, args) =>
            {
                bool succeeded = args.Error == null && !args.Cancelled && args.Reply != null && args.Reply.Status == IPStatus.Success;
                onComplete(hostIdx, succeeded);
                Ping p = sender as Ping;
                p.PingCompleted -= completed;
                p.Dispose();
            };

            pingSender.PingCompleted += completed;

            string data = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
            byte[] buffer = Encoding.ASCII.GetBytes(data);
            PingOptions options = new PingOptions(64, true);
            pingSender.SendAsync(hosts[hostIdx], 2000, buffer, options, hostIdx);
        }
    }


    private static void Main(string[] args)
    {
        for (int i = 0; i < 1000; i++)
        {
            Console.WriteLine("Send ping " + i);
            SendPing();
        }
    }
}