Ayende @ Rahien

Refunds available at head office

Deterministic Disposable

Here is a challenge, get this to work:

///<summary>
/// Executes the given handler when the instance is disposed
/// of using the Dispose(instance);
/// Note: Doesn't cause memory leak
///</summary>
public void OnDisposable(object instance, Action<object> action);

///<summary>
/// Executes the previously registered Action for this
/// instance
///</summary>
public void Dispose(object instance);

The key part here is to get this to work without causing a memory leak. Furthermore, assume that you need to handle this scenario as well without causing a leak:

object instance = new object();
OnDisposable(instance, delegate(object obj)
{
    Console.WriteLine("Disposing of {0}", obj);
});

Comments

Peter ritchie
04/02/2008 01:35 PM by
Peter ritchie

Why the new object? Why isn't the second snippet:

OnDisposable(this, delegate(object)

{

Console.WriteLine("Disposing if {0}", obj);

});

This is before any managed or unmanaged memory or resources have been freed?

Ayende Rahien
04/02/2008 01:36 PM by
Ayende Rahien

You are doing it to other objects, not to yourself

Peter ritchie
04/02/2008 01:53 PM by
Peter ritchie

Define "leak": extra handle usage before GC.Collect, virtual memory increase, rooted object increase, etc?

Ayende Rahien
04/02/2008 01:56 PM by
Ayende Rahien

Leak: memory consumption of the application goes up and never goes down.

Eyal
04/02/2008 02:20 PM by
Eyal

Dictionary<object, Action> actions = new Dictionary<object, Action>();

    public void OnDisposable(object instance, Action<object> action)

    {

        actions[new WeakReference(instance)] = action;

    }
Alex Simkin
04/02/2008 02:22 PM by
Alex Simkin

Something to do with the fact that delegates are strong references?

Peter ritchie
04/02/2008 02:25 PM by
Peter ritchie

That's still pretty vague. The Windows memory manager will show an application's usage of memory based on the number of pages that application has mapped to physical memory, despite the application having freed any and all memory in some of those pages. The memory manager won't recycle those pages until it needs to. You can force that to happen by minimizing and restoring the application.

Have a look at some other application's memory usage in task manager while they're not minimized then try minimizing then restoring them--you'll see the memory usage go way down (normally). this is because it has forced the memory manager to take those unused pages away from the application.

Ayende Rahien
04/02/2008 02:26 PM by
Ayende Rahien

Eyal,

You are leaking references to the Action

Ayende Rahien
04/02/2008 02:27 PM by
Ayende Rahien

Peter,

Don't get technical on me. :-)

I am talking about memory that is not reclaimable by the GC. Is this tight enough definition?

Damien Guard
04/02/2008 02:33 PM by
Damien Guard

By listening to an object using the standard event mechanism you are preventing it from ever being collected or disposed - i.e a lapsed listener.

In .NET 3.5 you can use the WeakEventManager and WeakEvent classes to get round this.

[)amien

Peter Ritchie
04/02/2008 02:34 PM by
Peter Ritchie

Sounds like newly-rooted objects. How are you detecting these?

Geert Baeyaert
04/02/2008 02:47 PM by
Geert Baeyaert

I'm guessing this is what you're after?

public class WeakDelegate

{

public WeakDelegate(Delegate d)

{

  _method = d.Method;

  _target = new WeakReference(d.Target);

}


MethodInfo _method;

WeakReference _target;


public void Invoke(object[] args)

{

  object target = _target.Target;

  if (target != null) _method.Invoke(target, args);

}

}

Eyal
04/02/2008 02:52 PM by
Eyal

Ayende: good point. I need to think about it some more..

Geert: But where do you keep the instance of that WeakDelegate?

In a dictionary? You'll leak a DictionaryEntry in that case.

A list? You'll leak a list slot.

Jay
04/02/2008 03:06 PM by
Jay

Why aren't you using IDisposable? Something like this:

public class Disposable : IDisposable {

public Disposable(T instance, Action action) { ... }

...

}

object instance = ...;

Action action = ...;

using(new Disposable(instance, myAction)) {

...

}

Nathan
04/02/2008 03:22 PM by
Nathan

Eyal, maybe he could modify WeakDelegate to raise an event in the case that target == null in his Invoke method and then handle that event to clear the list slot or dictionary entry.

Eyal
04/02/2008 03:24 PM by
Eyal

Which means Invoke must be called in order to clean this up. The scenario Ayende gave is that nothing should leak even if Dispose isn't called.

I'm probably missing something here..

Nathan
04/02/2008 03:26 PM by
Nathan

You're right. That would require that invoke be called in order to clean it up.

The only other way I can think of to make sure that the list entries are not leaked is to occasionally query the weak delegate to see if its taret has been GC'd. If so, then clean up the list entry.

However, that seems like a lot of overkill for this problem.

There must be a better way.

Stefan Wenig
04/02/2008 03:43 PM by
Stefan Wenig

Without thinking this through, a solution could be to create a new object with a finalizer and wire it to the disposable object. When that object's finalizer gets called, I can assume that my disposable object has been finalized too or (the difficult part) will be finalized within the same GC cycle. So the first time my finalizer gets called, I have to resurrect myself (can I call GC.KeepAlive(this) in the finalizer?), or new up yet another object that gets GC'ed the next time around, and then do the call to the action delegate and remove my WeakReference stuff. Please take this with a grain of salt, but it could work. Keep in mind that objects with finalizers take two GC cycles to dissaper (finalize, then free the memory), so this might add up to several cycles before the memory actually gets freed.

Jay
04/02/2008 04:32 PM by
Jay

If OnDisposable is called on an object, is Dispose guaranteed to be called on that object?

If Dispose is called on an object, is it guaranteed that OnDisposable has been called on that object before?

Is it guaranteed that for any object, OnDisposable and Dispose are called exactly zero or one times?

How open-ended is the scenario?

Ayende Rahien
04/02/2008 05:02 PM by
Ayende Rahien

Jay,

No, Dispose() is not guaranteed to be called.

I guess that you could say that if you call Dispose you called OnDisposable, but that is not something that I want to be on.

Both may be call zero or more times.

OnDisposable overrwrite the action, Dispose run the action the first time and no op all else

Alex Simkin
04/02/2008 09:21 PM by
Alex Simkin

As usual I probably do not understand the question, but nothing is leaking in this implementation:

using System;

using System.Collections.Generic;

using System.Linq;

using System.Text;

namespace MemoryLeak

{

public class Oren

{

    static Dictionary<object, Action<object>> _dict = new Dictionary<object,Action<object>>();


    public void OnDispose(object instance, Action<object> action)

    {

        Action<object> actions;

        if (_dict.TryGetValue(instance, out actions))

        {

            _dict[instance] = action + action;

        }

        else

        {

            _dict[instance] = action;

        }

    }


    public void Dispose(object instance)

    {

        Action<object> action;

        if (_dict.TryGetValue(instance, out action))

        {

            action(instance);

            _dict.Remove(instance);

        }

    }

}


class Program

{

    static void Main(string[] args)

    {

        Oren instance;


        for (int i = 0; i < 10000; i++)

        {


            instance = new Oren();


            instance.OnDispose(instance, delegate(object obj)

            {

                Console.WriteLine("Disposing of {0}", obj);

            });


            instance.Dispose(instance);


            instance = null;


        }


        GC.Collect(3);

        GC.WaitForPendingFinalizers();


        Console.ReadLine();

    }

}

}

Daniel Grunwald
04/02/2008 09:23 PM by
Daniel Grunwald

So the task is to implement a WeakDictionary<Key, Value> that has a weak reference to the key and frees the dictionary slot and value when the key has been garbage collected?

This is easy to do in Java (WeakHashMap is part of the standard library, but even if it wasn't, it could be easily implemented thanks to the ReferenceQueue class), but I don't know any good solution in .NET.

All .NET solutions involve some sort of periodically called dictionary cleanup, basically

foreach (WeakReference r in Keys) {

if (r.Target == null) Remove(r);

}

which of course isn't efficient when the dictionary is large.

Alex Simkin
04/02/2008 09:31 PM by
Alex Simkin

Oh, now I get it.

Nick Guerrera implemented a generic dictionary, which allows both its keys and values to be garbage collected if there are no other references to them than from the dictionary itself back in 2006.

http://blogs.msdn.com/nicholg/archive/2006/06/04/617466.aspx

Ayende Rahien
04/02/2008 09:32 PM by
Ayende Rahien

Alex,

What happens if you don't call Dispose() ?

Eyal
04/03/2008 06:51 AM by
Eyal

As daniel said, you can create a thread that checks for collected refernces (which is what ASP.NET Cache does) but it seems like too much overhead.

Another solution is to create a CollectionNotifier clas:

class CollectionNotifier

{

~CollectionNotifier(Subscriber)

{

Subscriber.GCCalled();

}

}

Now you can do something like this:

new WeakRefernce(new CollectionNotifier(this));

And get notified when GC was called and references were removed and scan your own list for removed references.

What's your solution Ayende?

Stefan Wenig
04/03/2008 04:08 PM by
Stefan Wenig

just in case anybody is still trying to make sense of my comment: there's none, I just really didn't think it through (confused two sides of a reference in regard to who's kept alive, now how embarrassing is that...)

Lior
04/03/2008 07:53 PM by
Lior

I don't get it - Is there an answer to this question?? Or are you just posting questions you need help with? :)

Comments have been closed on this topic.