views:

509

answers:

4

There have been a lot of discussion around this and everyone tend to agree that you should always call Delegate.EndInvoke to prevent a memory leak (even Jon Skeet said it!).

I always followed this guideline without questioning, but recently I implemented my own AsyncResult class and saw that the only resource that could leak is the AsyncWaitHandle.

(In fact it doesn't really leak because the native resource used by the WaitHandle is encapsulated in a SafeHandle which has a Finalizer, it will add pressure on the finalize queue of the garbage collector though. Even so, a good implementation of AsyncResult will only initialize the AsyncWaitHandle on demand...)

The best way to know if there is a leak is just to try it:

Action a = delegate { };
while (true)
    a.BeginInvoke(null, null);

I ran this for a while and the memory stay between 9-20 MB.

Let's compare with when Delegate.EndInvoke is called:

Action a = delegate { };
while (true)
    a.BeginInvoke(ar => a.EndInvoke(ar), null);

With this test, the memory play between 9-30 MG, weird eh? (Probably because it takes a bit longer to execute when there is an AsyncCallback, so there will be more queued delegate in the ThreadPool)

What do you think... "Myth busted"?

P.S. ThreadPool.QueueUserWorkItem is a hundred more efficient than Delegate.BeginInvoke, its better to use it for fire & forget calls.

+1  A: 

In some situations, BeginInvoke doesn't need EndInvoke (particularly in WinForms window messaging). But, there are definitely situations where this matters - like BeginRead and EndRead for async communication. If you want to do a fire-and-forget BeginWrite, you'll probably end up in serious memory trouble after a while.

So, your one test can't be conclusive. You need to deal with many different types of asynchronous event delegates to deal with your question properly.

John Fisher
You are probably right that some asynchronous operations really does require a call to EndInvoke, I was talking about Delegate.BeginInvoke specifically.
SelflessCoder
+1  A: 

Consider the following example, which ran on my machine for a few minutes and reached a working set of 3.5 GB before I decided to kill it.

Action a = delegate { throw new InvalidOperationException(); };
while (true)
    a.BeginInvoke(null, null);

NOTE: Make sure to run it without a debugger attached or with "break on exception thrown" and "break on user-unhandled exception" disabled.

EDIT: As Jeff points out, the memory issue here is not a leak, but simply a case of overwhelming the system by queuing work faster than it can be processed. Indeed, the same behaviour can be observed by replacing the throw with any suitably long operation. And the memory usage is bounded if we leave enough time between BeginInvoke calls.

Technically, that leaves the original question unanswered. However, regardless of whether or not it can cause a leak, not calling Delegate.EndInvoke is a bad idea since it can cause exceptions to be ignored.

Nick Guerrera
Interesting, but have you tried this?Action a = delegate { throw new InvalidOperationException(); };while (true) a.BeginInvoke(ar => { try { a.EndInvoke(ar); } catch { } }, null);The memory consumption looks the same when calling EndInvoke, the memory probably get high only because it queue more work item in the queue than it can execute them when you throw an exception.
SelflessCoder
You're right. I've corrected my answer.
Nick Guerrera
+3  A: 

I did run a little test to call an Action delegate and throw an exception inside it. Then I do make sure that I do not flood the thread pool by maintaining only a specified amount of threads running at one time and fill the thread pool continously when a delete call has ended. Here is the code:

 static void Main(string[] args)
    {

        const int width = 2;
        int currentWidth = 0;
        int totalCalls = 0;
        Action acc = () =>
            {
                try
                {
                    Interlocked.Increment(ref totalCalls);
                    Interlocked.Increment(ref currentWidth);
                    throw new InvalidCastException("test Exception");
                }
                finally
                {
                    Interlocked.Decrement(ref currentWidth);
                }
            };

        while (true)
        {
            if (currentWidth < width)
            {
                for(int i=0;i<width;i++)
                    acc.BeginInvoke(null, null);
            }

            if (totalCalls % 1000 == 0)
                Console.WriteLine("called {0:N}", totalCalls);

        }
    }

After letting it run for about 20 minutes and over 30 million BeginInvoke calls later the private byte memory consumption was constant (23 MB) as well as the Handle count. There seems no leak to be present. I have read Jeffry Richters book C# via CLR where he states that there is a memory leak. At least this seems no longer to be true with .NET 3.5 SP1.

Test Environment: Windows 7 x86 .NET 3.5 SP1 Intel 6600 Dual Core 2.4 GHz

Yours, Alois Kraus

Alois Kraus
+7  A: 

Whether or not it currently leaks memory is not something you should depend on. The framework team could, in the future, change things in a way that could cause a leak, and because the official policy is "you must call EndInvoke" then it's "by design".

Do you really want to take the chance that your app will suddenly start leaking memory sometime in the future because you chose to rely on observed behavior over documented requirements?

Mystere Man
+1. Furthermore, there's no need to speculate about the dangers in a future revision of the framework. As I pointed out in my answer, leaving aside the memory leak possibility, not calling `Delegate.EndInvoke` will cause exceptions to be ignored and give you no way to know whether or not the action completed successfully.
Nick Guerrera