Is it good practice to unregister from external events in the Dispose method of an IDiposable class?

Question

I read the excellent answer explaining how to use the Dispose pattern, and also why it works that way.

Proper use of the IDisposable interface

The post clearly states that you would want to use the Dispose pattern in two different scenarios:

1. get rid of unmanaged resources (because we have to)
2. get rid of managed resources (because we want to be helpful)

My question is:

• When an object is subscribed to an external event for its entire lifetime, is it also common/good practice to unregister from that event in the Dispose method? Would you implement the IDisposable interface for that purpose?

Answer 1

Yes, you should.

This is the best way to indicate to consumers of your class that it has "resources" that must be released. (even though event subscriptions are technically not resources)

Answer 2

In many (most?) cases, an object will become eligible for garbage collection very soon after Dispose is called. For example, this will always be true for IDisposable objects instantiated with a using statement:

using(var myDisposableObject = ...)
{
    ...
} // myDisposableObject.Dispose() called here

// myDisposableObject is no longer reachable and hence eligible for garbage collection here

In this situation, I personally wouldn't clutter the code with removal of event subscriptions in the general case.

For example, an ASP.NET Page or UserControl is IDisposable, and often handles events from other controls on the web page. There is no need to remove these event subscriptions when the Page or UserControl is disposed, and in fact I've never seen an ASP.NET application where this is done.

UPDATE

Other answerers suggest you should always unsubscribe to events in the Dispose method of an IDisposable class.

I disagree with this in the general case, though there may be application-specific situations where it is appropriate.

The logical conclusion is that any class that subscribes to events should be IDisposable, so that it can unsubscribe deterministically - I see no logical reason why this recommendation should only apply to classes that happen to own unmanaged resources. I don't think this is a good general recommendation for the following reasons:

• Making a class IDisposable just so it can unsubscribe from events adds complexity for users of the class.

• Unsubscribing from events in the Dispose method requires the developer to keep a track of event subscriptions that need to be removed - somewhat fragile as it's easy to miss one (or for a maintenance developer to add one).

• In situations where a class subscribes to events from a long-lived publisher, it is probably more appropriate to use a Weak Event Pattern to ensure that the subscriber's lifetime is not affected by the event subscription.

• In many situations (e.g. an ASP.NET Page class subscribing to events from its child controls), the lifetime of publisher and subscriber are closely related, so there is no need to unsubscribe.

Answer 3

I prefer to have a two-pronged approach:

(1) An explicit method to UnregisterFromExternalEvents();

(2) A call in Dispose() to that method.

This way any code that controls instances of your class can either explicitly unregister, or trust on Dispose to properly dispose and take care of such matters.

Answer 4

Yes, it would be good practice to unregistered all external events, but though not extremely necessary, due to loose coupling nature of events. It removes the event entry point reference of the subscriber object from the event generator and yes will be helpful.

For the part to unsubscribe in the dispose method is also fine. The thumb rule of Dispose method is - "Dispose method should unload resources in a way that if the dispose is called more than once it still works, i.e you should free resource in dispose one and only one time. (which would require checks before disposing resources)"

Answer 5

Yes, it is an extremely good idea. An event publisher holds a reference to the event subscriber which will prevent the subscriber from being garbage collected. (See Do event handlers stop garbage collection from occurring?)

In addition, if your event handlers use the resources you are releasing, then the event handlers (which will continue to be called by the event publisher) may generate exceptions after the resources have been released.

It is important, therefore, to unregister from any events before releasing your resources, especially if you are making use of async or multiple threads, as it is possible in some scenarios for an event to be raised between releasing a resource and unregistering from that event.

The following code demonstrates this:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace ReleaseEvents
{
    class Program
    {
        public static event EventHandler SomethingHappened;

        static void Main( string[] args )
        {
            using ( var l_dependent = new Dependent() )
            {
                SomethingHappened( null, EventArgs.Empty );
            }

            // Just to prove the point, garbage collection
            // will not clean up the dependent object, even
            // though it has been disposed.
            GC.Collect();

            try
            {
                // This call will cause the disposed object
                // (which is still registered to the event)
                // to throw an exception.
                SomethingHappened( null, EventArgs.Empty );
            }
            catch ( InvalidOperationException e )
            {
                Console.ForegroundColor = ConsoleColor.Red;
                Console.WriteLine( e.ToString() );
            }

            Console.ReadKey( true );
        }
    }

    class Dependent : IDisposable
    {
        private object _resource;

        public Dependent()
        {
            Program.SomethingHappened += Program_SomethingHappened;
            _resource = new object();
        }

        private void Program_SomethingHappened( object sender, EventArgs e )
        {
            if ( _resource == null )
                throw new InvalidOperationException( "Resource cannot be null!" );

            Console.WriteLine( "SomethingHappened processed successfully!" );
        }

        public void Dispose()
        {
            _resource = null;
        }
    }
}

The second time that the SomethingHappened event is raised, the Dependent class will throw an InvalidOperationException. You must unregister the event to prevent this from happening:

    class Dependent : IDisposable
    {
        // ...

        public void Dispose()
        {
            Program.SomethingHappened -= Program_SomethingHappened;

            _resource = null;
        }
    }

I actually ran into this problem when I first tried to implement a MVVM architecture. When I was switching between ViewModels, I was simply releasing what I thought was my only reference (an ActiveViewModel property). I didn't realize that the events my ViewModel was subscribed to was keeping it in memory. As the application ran longer, it became slower and slower. Eventually I realized that the ViewModels I thought I had released were actually continuing to process events (expensively). I had to explicitly release the event handlers to fix this problem.

Answer 6

Thanks to this question I will write my classes as:

class Foo : IDisposable
{
    public event EventHandler MyEvent;

    /// <summary>
    /// When disposing unsubscibe from all events
    /// </summary>
    public void Dispose()
    {
        if (MyEvent != null)
        {
            foreach (Delegate del in MyEvent.GetInvocationList())
                MyEvent -= (del as EventHandler);
        }

        // do the same for any other events in the class
        //  ....
    }
}