C# Pattern to Prevent an Event Handler Hooked Twice

C# pattern to prevent an event handler hooked twice

Explicitly implement the event and check the invocation list. You'll also need to check for null:

using System.Linq; // Required for the .Contains call below:

...

private EventHandler foo;
public event EventHandler Foo
{
    add
    {
        if (foo == null || !foo.GetInvocationList().Contains(value))
        {
            foo += value;
        }
    }
    remove
    {
        foo -= value;
    }
}

Using the code above, if a caller subscribes to the event multiple times, it will simply be ignored.

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.

Which C# pattern has better performance to avoid duplicated event handlers?

According the documentation, invocation list is being stored as array or something similar to it, and the order of the event handlers is being stored too. May be there are inner structure to maintain fast search for a particular method there.

So in the worst case operation of the GetInvocationList().Contains(MyEventHandlerMethod); is O(1) (as we simply got the reference for the array) + O(n) for searching the method, even if there is no optimization for it. I seriously doubt that this is true, and I think that there is some optimizing code, and it is O(log_n).

Second approach has additional operation of adding which is, I think, O(1), as we adding the event handler to the end.

So, to see the difference between such actions, you need a lot of the event handlers.

But! If you use the second approach, as I said, you'll add the event handler to the end of the queue, which can be wrong in some cases. So use the first one, and has no doubt in it.

Implement Postsharp EventInterceptionAspect to prevent an event Handler hooked twice

This class does the trick.

[Serializable]
public class PreventEventHookedTwiceAttribute: EventInterceptionAspect
{
    private readonly object _lockObject = new object();
    readonly List<Delegate> _delegates = new List<Delegate>();

    public override void OnAddHandler(EventInterceptionArgs args)
    {
        lock(_lockObject)
        {
            if(!_delegates.Contains(args.Handler))
            {
                _delegates.Add(args.Handler);
                args.ProceedAddHandler();
            }
        }
    }

    public override void OnRemoveHandler(EventInterceptionArgs args)
    {
        lock(_lockObject)
        {
            if(_delegates.Contains(args.Handler))
            {
                _delegates.Remove(args.Handler);
                args.ProceedRemoveHandler();
            }
        }
    }
}

Example Usage to display the difference is given as below.

class Program
    {
        private static readonly object _lockObject = new object();
        private static int _counter = 1;

        [PreventEventHookedTwice]
        public static event Action<string> GoodEvent;

        public static event Action<string> BadEvent;

        public static void Handler (string message)
        {
            lock(_lockObject)
            {
                Console.WriteLine(_counter +": "+ message);
                _counter++;
            }
        }

        static void Main(string[] args)
        {
            GoodEvent += Handler;
            GoodEvent += Handler;
            GoodEvent += Handler;
            GoodEvent += Handler;
            GoodEvent += Handler;
            Console.WriteLine("Firing Good Event. Good Event is subscribed 5 times from the same Handler.");
            GoodEvent("Good Event is Invoked.");

            _counter = 1;
            BadEvent += Handler;
            BadEvent += Handler;
            BadEvent += Handler;
            BadEvent += Handler;
            BadEvent += Handler;
            Console.WriteLine("Firing Bad Event. Bad Event is subscribed 5 times from the same Handler.");
            BadEvent("Bad Event is Invoked.");

            _counter = 1;
            GoodEvent -= Handler;
            Console.WriteLine("GoodEvent is unsubscribed just once. Now fire the Event");
            if(GoodEvent!= null)
            {
                GoodEvent("Good Event Fired");
            }
            Console.WriteLine("Event is not received to Handler.");

            BadEvent -= Handler;
            Console.WriteLine("BadEvent is unsubscribed just once. Now fire the Event");
            BadEvent("Good Event Fired");
            Console.WriteLine("Event is fired 4 times. If u subscribe good event 5 times then u have to unscribe it for 5 times, otherwise u will be keep informed.");

            Console.ReadLine();
        }
    }

Postsharp Rocks.

c# event invoked twice

As gentlemen in comments suggested, my problems were caused by calling Foo's constructor twice, which then resulted in two separate objects of Foo. And since the event is subscribed in mentioned constructors... I got one additional subscriber.

I was pretty sure that this could not be the case, because I searched through the code looking for another call Foo's constructor: new Foo(), and found nothing. My fatal mistake was to assume that I was coherent with the way I call the constructor...

And then somewhere deep in the code I found this lonely line:

private Foo? _viewModel = new();

Too much sugar can make your teeth go bad, too much syntax sugar can make you go mad. Lesson learned: be coherent.

Prevent duplicate event handler invocations in Outlook VSTO

The event handler is not actually being attached to the MAPI item that Outlook works with. Instead, it's being attached to a .NET object, called a Runtime Callable Wrapper (RCW), which wraps a COM object. Because of the way RCWs work, getting more than one reference to what seems to be the same object - for example, by getting the activeExplorer.Selection()[1] twice - gives multiple RCWs around different COM objects. This meant that the Outlook.MailItem (or Outlook.ItemEvents_10_Event) that I was attempting to remove events from didn't actually have any events on it; it was new-made each time SelectionChangeHandler fired.

Relatedly, because the only reference to an RCW-wrapped COM object is the RCW itself, letting all variables referencing an RCW go out of scope (or otherwise cease to reference that RCW) will result in the RCW being garbage collected (during which the COM object will be released and deleted). This has two relevant impacts on the code in question:

1. Because the garbage collection is not immediate, old RCWs (and COM objects) with existing event handlers were lingering on, and Outlook could still trigger events on their COM objects. This is why the event handler would be invoked multiple times.
2. Because the RCW was going out of scope at the bottom of SelectionChangeHandler, it was only a matter of time until garbage collection swept up all of the RCWs (and event handlers) and released all their COM objects. At that point, no events would be attached to that email.
   
   
   • While in practice, my testing happened over a short enough timeframe that I was more likely to get multiple live RCWs than none, selecting a mail item and not interacting with it (or selecting anything else) for long enough to trigger a garbage collection sweep would, in fact, result in the MailItemResponseHandler not being invoked at all when I clicked Reply.

@DmitryStreblechenko gave me the push in the right direction to work this out, but it took some experimentation to figure out. First of all, the relevant MailItem needed to be globally referenced, so its reference to the evented RCW wouldn't go out of scope and, also importantly, so its RCW could still be directly referenced when the SelectionChangeHandler was invoked again. I renamed the variable selectedMail and referenced it at the class level like such:

Outlook.ItemEvents_10_Event selectedMail;

I then modified SelectionChangeHandler so that whenever it is invoked with a single MailItem currently selected, it first removes all the event handlers from selectedMail, and only then points selectedMail to the newly-selected item. The RCW previously reference by selectedMail becomes eligible for garbage collection, but it has no event handlers on it so we don't really care. SelectionChangeHandler then adds the relevant event handlers to the new RCW now referenced by selectedMail.

    private void SelectionChangeHandler()
    {
        Outlook.Selection sel = activeExplorer.Selection;
        // First make sure it's a (single) mail item
        if (1 != sel.Count)
        {   // Ignore multi-select
            return;
        }
        // Indexed from 1, not 0. Stupid VB-ish thing...
        Outlook.MailItem mail = sel[1] as Outlook.MailItem;
        if (null != mail)
        {
            if (null != selectedMail)
            {   // Remove the old event handlers, if they were set, so there's no repeated events
                selectedMail.Forward -= MailItemResponseHandler;
                selectedMail.Reply -= MailItemResponseHandler;
                selectedMail.ReplyAll -= MailItemResponseHandler;
            }
            selectedMail = mail as Outlook.ItemEvents_10_Event;
            selectedMail.Forward += MailItemResponseHandler;
            selectedMail.Reply += MailItemResponseHandler;
            selectedMail.ReplyAll += MailItemResponseHandler;
            if (DecryptOnSelect)
            {   // We've got a live mail item selected. Process it
                ProcessMailitem(mail);

            }
        }
    }

Based on Dmitri's answer and comments, I tried calling Marshal.ReleaseComObject(selectedMail) on the old value of selectedMail before I thought to remove the event handlers instead. This helped a little, but either the COM objects are not released immediately or Outlook can still invoke event handlers through them, because events were still firing multiple times if I selected a given email multiple times in a short period before hitting Reply.

There's still one glitch to work out. If I open an inspector and hit Reply in there without changing my selection in the Explorer, it works fine (the MailItemResponseHandler is invoked). However, if I leave the inspector open, switch back to the explorer and select a different email, and then return to the inspector and hit Reply, it doesn't work. If there's an inspector open for the relevant email when that email gets de-selected, I need to avoid removing the event handlers (and then I do need to remove them when the inspector gets closed, unless the email is still selected in the explorer). Messy, but I'll work it out.

Add/remove EventHandler multiple times in WPF

Nothing really happens when you try to unsubscribe from event multiple times.

When you want to subscribe to event with your handler you can make sure it's not already subscribed. You should do that. Because it's possible to subscribe multiple times. of course only if it's not something you want to do...

What happens when you subscribe to the event twice with same handler? It's simple... handler is twice in event's InvocationList so it's called twice. When you subscribe again then it's called three times... every time the event is raised subscribed Handlers in InvocationList are called.

You can look here ...as you can see... It's duplicate of at least two already asked questions. So there are many answers :)

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.

Has an event handler already been added?

From outside the defining class, as @Telos mentions, you can only use EventHandler on the left-hand side of a += or a -=. So, if you have the ability to modify the defining class, you could provide a method to perform the check by checking if the event handler is null - if so, then no event handler has been added. If not, then maybe and you can loop through the values in
Delegate.GetInvocationList. If one is equal to the delegate that you want to add as event handler, then you know it's there.

public bool IsEventHandlerRegistered(Delegate prospectiveHandler)
{   
    if ( this.EventHandler != null )
    {
        foreach ( Delegate existingHandler in this.EventHandler.GetInvocationList() )
        {
            if ( existingHandler == prospectiveHandler )
            {
                return true;
            }
        }
    }
    return false;
}

And this could easily be modified to become "add the handler if it's not there". If you don't have access to the innards of the class that's exposing the event, you may need to explore -= and +=, as suggested by @Lou Franco.

However, you may be better off reexamining the way you're commissioning and decommissioning these objects, to see if you can't find a way to track this information yourself.

Click event Firing twice

How does your codebehind look? Are you sure you don't have the event hooked up there as well?

You have OnClick="btnUpload_Click" in your markup which would hook up the event handler once, so if you also have a btnUpload.Click += btnUpload_Click; in the codebehind, the event handler would fire twice.

---

Related Topics