views:

953

answers:

7

I have a class that handles events from a WinForms control. Based on what the user is doing, I am deferencing one instance of the class and creating a new one to handle the same event. I need to unsubscribe the old instance from the event first - easy enough. I'd like to do this in a non-proprietary manner if possible, and it seems like this is a job for IDisposable. However, most documentation recommends IDisposable only when using unmanaged resources, which does not apply here.

If I implement IDisposable and unsubscribe from the event in Dispose(), am I perverting its intention? Should I instead provide an Unsubscribe() function and call that?


Edit: Here's some dummy code that kind of shows what I'm doing (using IDisposable). My actual implementation is related to some proprietary data binding (long story).

class EventListener : IDisposable
{
    private TextBox m_textBox;

    public EventListener(TextBox textBox)
    {
        m_textBox = textBox;
        textBox.TextChanged += new EventHandler(textBox_TextChanged);
    }

    void textBox_TextChanged(object sender, EventArgs e)
    {
        // do something
    }

    public void Dispose()
    {
        m_textBox.TextChanged -= new EventHandler(textBox_TextChanged);
    }
}

class MyClass
{
    EventListener m_eventListener = null;
    TextBox m_textBox = new TextBox();

    void SetEventListener()
    {
        if (m_eventListener != null) m_eventListener.Dispose();
        m_eventListener = new EventListener(m_textBox);
    }
}

In the actual code, the "EventListener" class is more involved, and each instance is uniquely significant. I use these in a collection, and create/destroy them as the user clicks around.


Conclusion

I'm accepting gbjbaanb's answer, at least for now. I feel that the benefit of using a familiar interface outweighs any possible downside of using it where no unmanaged code is involved (how would a user of this object even know that?).

If anyone disagrees - please post/comment/edit. If a better argument can be made against IDisposable, then I'll change the accepted answer.

+3  A: 

My personal vote would be to have an Unsubscribe method in order to remove the class from events. IDisposable is a pattern intended for deterministic release of unmanaged resources. In this case you not managing any unmanaged resources and therefore should not be implementing IDisposable.

IDisposable can be used to manage event subscriptions but probably shouldn't. For an example I point you to WPF. This is a library rife with events and event handlers. Yet virtually no class in WPF implements IDisposable. I would take that as an indication that events should be managed another way.

JaredPar
WPF has hardly any IDisposable controls because it uses the WeakEvent Pattern to get round leaks: http://msdn.microsoft.com/en-us/library/aa970850.aspx
gbjbaanb
A: 

IDisposable is firmly about resources, and the source of enough problems to not muddy the waters further I think.

I'm voting for an Unsubscribe method on your own Interface too.

annakata
+10  A: 

Yes, go for it. Although some people think IDisposable is implemented only for unmanaged resources, this is not the case - unmanaged resources just happens to be the biggest win, and most obvious reason to implement it. I think its acquired this idea because people couldn't think of any other reason to use it. Its not like a finaliser which is a performance problem and not easy for the GC to handle well.

Put any tidy-up code in your dispose method. It'll be clearer, cleaner and significantly more likely to prevent memory leaks and a damn sight easier to use correctly than trying to remember to un-do your references.

The intention of IDisposable is to make your code work better without you having to do lots of manual work. Use its power in your favour and get over some artificial "design intention" nonsense.

I remember it was difficult enough to persuade Microsoft of the usefulness of deterministic finalisation back when .NET first came out - we won the battle and persuaded them to add it (even if it was only a design pattern at the time), use it!

gbjbaanb
I strongly disagree. You are breaking the "contract" in "design by contract" when you do that.
Domenic
what contract? Nowhere does it say "IDisposable is for unmanaged resources only". Often it says "can be used for" but that's quite a difference.
gbjbaanb
@Domenic: I agree with gbjbaanb. Even if the docs did say that IDisposable is intended only for the release unmanaged resources, you wouldn't really break any hard contract (as in preconditions, postconditions and class invariants) when you use it for other clean-up.
Andreas Huber
Wouldn't letting go of listeners be considered cleaning up?
toast
A: 

One option may be not to unsubscribe at all - just to change what the subscription means. If the event handler could be made smart enough to know what it's meant to do based on the context, you don't need to unsubscribe in the first place.

That may or may not be a good idea in your particular case - I don't think we've really got enough information - but it's worth considering.

Jon Skeet
In my case I think it means the object would live forever and I'd have a memory leak, unfortunately.
Jon B
+1  A: 

Another option would be to use weak delegates or something like WPFs weak events, instead of having to unsubscribe explicitly.

P.S. [OT] I consider the decision to only provide strong delegates the single most expensive design mistake of the .NET platform.

Andreas Huber
+3  A: 

I think disposable is for anything that GC can't take care of automatically, and event references count in my book. Here is a helper class I came up with.

public class DisposableEvent<T> : IDisposable
    {

        EventHandler<EventArgs<T>> Target { get; set; }
        public T Args { get; set; }
        bool fired = false;

        public DisposableEvent(EventHandler<EventArgs<T>> target)
        {
            Target = target;
            Target += new EventHandler<EventArgs<T>>(subscriber);
        }

        public bool Wait(int howLongSeconds)
        {
            DateTime start = DateTime.Now;
            while (!fired && (DateTime.Now - start).TotalSeconds < howLongSeconds)
            {
                Thread.Sleep(100);
            }
            return fired;
        }

        void subscriber(object sender, EventArgs<T> e)
        {
            Args = e.Value;
            fired = true;
        }

        public void Dispose()
        {
            Target -= subscriber;
            Target = null;
        }

    }

which lets you write this code :

Class1 class1 = new Class1();
            using (var x = new DisposableEvent<object>(class1.Test))
            {
                if (x.Wait(30))
                {
                    var result = x.Args;
                }
            }

One side effect, you must not use the event keyword on your events, since that prevents passing them as a parameter to the helper constructor, however, that seems to not have any ill effects.

Jason Coyne
+1  A: 

One thing that bothers me about using IDisposable pattern for unsubscribe from events is Finalization issue.

Dispose() function in IDisposable is supposed to be call by the developer, HOWEVER, if it isn't called by the developer, it is a understood that the GC will call this function (by the standard IDisposable pattern, at least). In your case, however, if you don't call Dispose() no one else will - The event remains and the strong reference holds GC from calling the finalizer.

The mere fact that Dispose() won't be called automatically by GC seems to me enough to not to use IDosposable in this case. Perhaps it calls for a new application specific interface that says that this type of object must have a Cleanup function called to be disposed by GC.

VitalyB