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

Question

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

Proper use of the IDisposable interface

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

• get rid of unmanaged resources (because we should)
• get rid of managed resources (because we want to be useful)

My question is:

• When an object subscribes to an external event throughout its life cycle, is it common practice to unregister this event from the Dispose method? Would you use the IDisposable interface for this purpose?

Answer 1

Yes you should.

This is the best way to tell consumers in your class that it has “resources” to be released. (even if subscribing to events is not technically a resource)

Answer 2

In many cases (most?), The object will be eligible for garbage collection very soon after the Dispose call. For example, this will always be true for IDisposable objects created using the 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 will not clutter up the code with the subscription to events in the general case.

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

UPDATE

Other responders suggest that you should always unsubscribe from events in the Dispose method of the IDisposable class.

I disagree with this in the general case, although there may be situations depending on the application, when appropriate.

The logical conclusion is that any class that subscribes to events must be IDisposable , so that it can unsubscribe in a deterministic way - I see no logical reason why this recommendation should be applied only to classes that have their own unmanaged resources. I do not think this is a good general recommendation for the following reasons:

• Creating an IDisposable class so that it can unsubscribe from events adds complexity to users of this class.

• Unsubscribing from events in the Dispose method requires the developer to track the subscription for events that need to be removed — somewhat fragile as it is easy to skip (or for the maintenance developer to add it).

• In situations where a class subscribes to events from a long-lived publisher, it is probably more appropriate to use the Conditional event pattern to ensure that the subscriber's lifetime is independent of the subscription to the events.

• In many situations (for example, an ASP.NET page class that subscribes to events from its child controls), the lifetime of the publisher and subscriber is closely related, so there is no need to unsubscribe.

Answer 3

I prefer to have a two-way approach:

(1) Explicit method for UnregisterFromExternalEvents();

(2) Call in Dispose() to this method.

Thus, any code that manages the instances of your class can either explicitly unregister or trust on Dispose to properly manage and take care of such issues.

Answer 4

Yes, it would be good practice to not register all external events, but, although this is not absolutely necessary, due to the weak coupling of the nature of the events. It removes the link to the entry point of the event of the subscribing object from the event generator, and yes it will be useful.

Unsubscribing from the dispose method is also great. The rule of the thumb of the Dispose method is "The Dispose method must unload the resources in such a way that if dispose is called more than once, it still works, that is, you must release the resource at the disposal one and only once (which will require checking before deleting the resources)"

Answer 5

Yes, that’s a very good idea. The event publisher has a link to the event subscriber that will prevent the collection of the subscriber from garbage. (See Event Handlers Stop Garbage Collection? )

In addition, if event handlers use the resources that you release, event handlers (which will be referred to as the event publisher) may throw exceptions when resources are released.

Therefore, it is important to unregister any events before , freeing your resources, especially if you use async or several threads, since it is possible in some scenarios for an event raised between freeing a resource and unregistering from this 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 the SomethingHappened event is raised, the Dependent class will throw an InvalidOperationException. You must unregister the event so that this does not happen:

  class Dependent : IDisposable { // ... public void Dispose() { Program.SomethingHappened -= Program_SomethingHappened; _resource = null; } } 

I really ran into this problem when I first tried to implement the MVVM architecture. When I switched between ViewModels, I just freed what I considered to be my only link (ActiveViewModel property). I did not understand that the events that my ViewModel was subscribed to were stored in memory. As the application ran longer, it became slower and slower. In the end, I realized that the ViewModels, which I thought I released, actually continue to handle events (expensively). I had to explicitly free event handlers in order 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 // .... } }