C# pattern to prevent an event handler hooked twice


Answers

How about just removing the event first with -= , if it is not found an exception is not thrown

/// -= Removes the event if it has been already added, this prevents multiple firing of the event
((System.Windows.Forms.WebBrowser)sender).Document.Click -= new System.Windows.Forms.HtmlElementEventHandler(testii);
((System.Windows.Forms.WebBrowser)sender).Document.Click += new System.Windows.Forms.HtmlElementEventHandler(testii);
Question

This question already has an answer here:

Duplicate of: How to ensure an event is only subscribed to once and Has an event handler already been added?

I have a singleton that provides some service and my classes hook into some events on it, sometimes a class is hooking twice to the event and then gets called twice. I'm looking for a classical way to prevent this from happening. somehow I need to check if I've already hooked to this event...




You really should handle this at the sink level and not the source level. That is, don't prescribe event handler logic at the event source - leave that to the handlers (the sinks) themselves.

As the developer of a service, who are you to say that sinks can only register once? What if they want to register twice for some reason? And if you are trying to correct bugs in the sinks by modifying the source, it's again a good reason for correcting these issues at the sink-level.

I'm sure you have your reasons; an event source for which duplicate sinks are illegal is not unfathomable. But perhaps you should consider an alternate architecture that leaves the semantics of an event intact.




I recently came to a similar situation where I needed to register a handler for an event only once. I found that you can safely unregister first, and then register again, even if the handler is not registered at all:

myClass.MyEvent -= MyHandler;
myClass.MyEvent += MyHandler;

Note that doing this every time you register your handler will ensure that your handler is registered only once. Sounds like a pretty good practice to me :)




How to ensure an event is only subscribed to once

If you are talking about an event on a class that you have access to the source for then you could place the guard in the event definition.

private bool _eventHasSubscribers = false;
private EventHandler<MyDelegateType> _myEvent;

public event EventHandler<MyDelegateType> MyEvent
{
   add 
   {
      if (_myEvent == null)
      {
         _myEvent += value;
      }
   }
   remove
   {
      _myEvent -= value;
   }
}

That would ensure that only one subscriber can subscribe to the event on this instance of the class that provides the event.

EDIT please see comments about why the above code is a bad idea and not thread safe.

If your problem is that a single instance of the client is subscribing more than once (and you need multiple subscribers) then the client code is going to need to handle that. So replace

not already subscribed

with a bool member of the client class that gets set when you subscribe for the event the first time.

Edit (after accepted): Based on the comment from @Glen T (the submitter of the question) the code for the accepted solution he went with is in the client class:

if (alreadySubscribedFlag)
{
    member.Event += new MemeberClass.Delegate(handler);
}

Where alreadySubscribedFlag is a member variable in the client class that tracks first subscription to the specific event. People looking at the first code snippet here, please take note of @Rune's comment - it is not a good idea to change the behavior of subscribing to an event in a non-obvious way.

EDIT 31/7/2009: Please see comments from @Sam Saffron. As I already stated and Sam agrees the first method presented here is not a sensible way to modify the behavior of the event subscription. The consumers of the class need to know about its internal implementation to understand its behavior. Not very nice.
@Sam Saffron also comments about thread safety. I'm assuming that he is referring to the possible race condition where two subscribers (close to) simultaneously attempt to subscribe and they may both end up subscribing. A lock could be used to improve this. If you are planning to change the way event subscription works then I advise that you read about how to make the subscription add/remove properties thread safe.




You can implement your own storage of the delgates, and check for uniqueness when adding them to the event. See EventOwner2 class below for an example. I don't know how this is doing performance wise, but than again, that is not always an issue.

using System;
using System.Collections.Generic;

namespace EventExperiment
{
    class Program
    {
        static void Main(string[] args)
        {
            IEventOwner e=new EventOwner2();
            Subscriber s=new Subscriber(e);
            e.RaiseSome();
            Console.ReadKey();
        }
    }

    /// <summary>
    /// A consumer class, subscribing twice to the event in it's constructor.
    /// </summary>
    public class Subscriber
    {
        public Subscriber(IEventOwner eventOwner)
        {
            eventOwner.SomeEvent += eventOwner_SomeEvent;
            eventOwner.SomeEvent += eventOwner_SomeEvent;
        }

        void eventOwner_SomeEvent(object sender, EventArgs e)
        {
            Console.WriteLine(DateTimeOffset.Now);
        }

    }

    /// <summary>
    /// This interface is not essensial to this point. it is just added for conveniance.
    /// </summary>
    public interface IEventOwner
    {
        event EventHandler<EventArgs> SomeEvent;
        void RaiseSome();
    }

    /// <summary>
    /// A traditional event. This is raised for each subscription.
    /// </summary>
    public class EventOwner1 : IEventOwner
    {
        public event EventHandler<EventArgs> SomeEvent = delegate { };
        public void RaiseSome()
        {
            SomeEvent(this,new EventArgs());
        }
    }
    /// <summary>
    /// A custom event. This is raised only once for each subscriber.
    /// </summary>
    public class EventOwner2 : IEventOwner
    {
        private readonly List<EventHandler<EventArgs>> handlers=new List<EventHandler<EventArgs>>();
        public event EventHandler<EventArgs> SomeEvent
        {
            add
            {
                lock (handlers)
                    if (handlers!=null&&!handlers.Contains(value))
                    {
                        handlers.Add(value);
                    }
            }
            remove
            {
                handlers.Remove(value);
            }
        }
        public void RaiseSome()
        {
            EventArgs args=new EventArgs();
            lock(handlers)
            foreach (EventHandler<EventArgs> handler in handlers)
            {
                handler(this,args);
            }
        }
    }
}



As others have shown, you can override the add/remove properties of the event. Alternatively, you may want to ditch the event and simply have the class take a delegate as an argument in its constructor (or some other method), and instead of firing the event, call the supplied delegate.

Events imply that anyone can subscribe to them, whereas a delegate is one method you can pass to the class. Will probably be less surprising to the user of your library then, if you only use events when you actually wnat the one-to-many semantics it usually offers.




Preventing same Event handler assignment multiple times

Baget is right about using an explicitly implemented event (although there's a mixture there of explicit interface implementation and the full event syntax). You can probably get away with this:

private EventHandler foo;

public event EventHandler Foo
{
    add
    {
        // First try to remove the handler, then re-add it
        foo -= value;
        foo += value;
    }
    remove
    {
        foo -= value;
    }
}

That may have some odd edge cases if you ever add or remove multicast delegates, but that's unlikely. It also needs careful documentation as it's not the way that events normally work.