c# event handler is called multiple times when event is raised once

Question

Below is my code, first is where I raise the event and second section is where I consume it in another class. It seems pretty straight forward, but the logs are showing that even though the event is raised once, the event is firing 20+ times on the class that consumes this event. Any ideas?

IBSerialPort class:

public delegate void PacketReceivedHandler(object sender, PacketReceivedEventArgs e);
public event PacketReceivedHandler OnPacketReceived;

public class PacketReceivedEventArgs : EventArgs
{
  public Packet PacketReceived { get; private set; }

  public PacketReceivedEventArgs(Packet packet)
  {
    PacketReceived = packet;
  }
}

// raise event
if (OnPacketReceived != null)
{
    Log("This is only called ONCE!");
    PacketReceivedEventArgs args = new PacketReceivedEventArgs(data);
    OnPacketReceived(this, args);
}

Class that uses IBSerialPort and consumes its OnPacketReceived Event:

IBSerialPort ibSerialPort = null;
..
if (ibSerialPort == null)
{
  Log("This is only called once");

  ibSerialPort = IBSerialPort.Instance;

  ibSerialPort.OnPacketReceived += ibSerialPort_OnPacketReceived;
}

void ibSerialPort_OnPacketReceived(object sender, IBSerialPort.PacketReceivedEventArgs args)
{
   Log("This is called ~25 times!!!!");
}

Misdeed

Are you sure that you're only subscribing to the event once? – Neoarsphenamine

Where are you calling ibSerialPort.OnPacketReceived += ibSerialPort_OnPacketReceived – Yawn

Keep in mind that even if you think your subscriber is released, a reference will be held for the subscription. – Thinner

Answer 1

Try this, this will unregister any prev subscriber:

ibSerialPort.OnPacketReceived -= ibSerialPort_OnPacketReceived;   // unregister
ibSerialPort.OnPacketReceived += ibSerialPort_OnPacketReceived;  //register

Incivility

yes it would, if this is method code then it would re-register the event every time called. – Incivility

ibSerialPort is an instance declaration. A COM object is using this class, I wonder if ibSerialPort.OnPacketReceived += ibSerialPort_OnPacketReceived; is being called multiple times. Thanks for the quick responses! – Misdeed

Good answer, same as mine. – Nf

I didn't copy you I assure you, it's a pretty common technique – Incivility

Not accusing of stealing, just sayin...awesome answer! – Nf

Answer 2

How many times is this being called? If this gets called multiple times then your event will be called multiple times.

 ibSerialPort.OnPacketReceived += ibSerialPort_OnPacketReceived;

As a test, you could remove the delegate just before you add it:

ibSerialPort.OnPacketReceived -= ibSerialPort_OnPacketReceived;
ibSerialPort.OnPacketReceived += ibSerialPort_OnPacketReceived;

Nf

this litterally worked like a charm for me...didnt noticed i kept adding more and more events to the control – Roberson

Answer 3

I wonder if your class that defines ibSerialPort_OnPacketReceived is used (even in separated instances) 25 times and you think you are releasing it. Consider this code:

class EventSender
{
    public Action MyEvent;
}

class Subscriber
{
    public void OnEvent()
    {
        Console.WriteLine("OnEvent");
    }
}

class Program
{
    static void Main(string[] args)
    {
        EventSender es = new EventSender();

        Subscriber s = new Subscriber();
        es.MyEvent += s.OnEvent;

        s = new Subscriber();
        es.MyEvent += s.OnEvent;

        es.MyEvent();

        Console.ReadKey();
    }
}

Here, "OnEvent" will be printed twice. A reference to the subscription is held even though it appears I've released the handle to it. This is due to how delegates keep a list of their subscribers.

If this is the problem, you'll need to unsubscribe each time:

es.MyEvent -= s.OnEvent

This should be done before you lose your handle to your subscriber (i.e. before s is out of scope or null). You could consider keeping track of your event source in the subsriber, and have a Dispose method that unsubscribes for you.

Also, as others have noted, you could unsubscribe before subscribing :) I'm sure by now you have the solution you need.

Thinner

Answer 4

I had the same problem, register your event in a synchronous method ( I put it in form_loaded)

    private async void Window_Loaded(object sender, RoutedEventArgs e)
    {
        RefreshHierarchy.COIDConflict += RefreshHierarchy_COIDConflict;
    }

Conde