CODEWITHSUNDEEP
c#.netevents
Edward
Edward

Reputation: 7424

Custom Event handler is getting called twice?

I've created an event handler that simply returns a list of objects that I receive from a web service when the call completes.

Now I went ahead and ran the app in debug mode and found out that the first time the event is called it works perfectly, but immediately after it completes the event is being fired for a second time. I've checked and am absolutely sure I am not calling the event more than once in the receiver class.

This is my first shot at creating custom event handlers inside my applications so I am not entirely sure the implementation is 100% accurate.

Any ideas of what might be causing this? Is the way I created the event handler accurate?

This is the DataHelper class

public class DataHelper
{
    public delegate void DataCalledEventHandler(object sender, List<DataItem> dateItemList);
    public event DataCalledEventHandler DataCalled;

    public DataHelper()
    {

    }

    public void CallData()
    {
        List<DataItem> dataItems = new List<DataItem>();
        //SOME CODE THAT RETURNS DATA
        DataCalled(this, dataItems);
    }
}

This is where I subscribed to my event:

protected override void OnNavigatedTo(System.Windows.Navigation.NavigationEventArgs e)
{
   GetNewDataItems();
}
private void GetNewDataItems()
        {

                try
                {
                    DataHelper dataHelper = new DataHelper();
                    dataHelper.CallData();
                    dataHelper.DataCalled += new DataHelper.DataCalledEventHandler(dataHelper_DataCalled);

                }
                catch
                {
                   //Handle any errors
                }
            }
    }

    void dataHelper_DataCalled(object sender, List<DataItem> dataItemsList)
    {
        //Do something with results
        //NOTE: THIS IS WHERE THE EXCEPTION OCCURS WHEN EVENT IS FIRED FOR SECOND TIME
    }

Upvotes: 5

Views: 12538

Answers (2)

Ankur
Ankur

Reputation: 33637

The below lines on your code should be flipped. That is

These lines 

dataHelper.CallData();
dataHelper.DataCalled += new DataHelper.DataCalledEventHandler(dataHelper_DataCalled);

Should be:

dataHelper.DataCalled += new DataHelper.DataCalledEventHandler(dataHelper_DataCalled);
dataHelper.CallData();

Because you first need to attach the event handler and then call other methods on the object which can raise the event

Upvotes: 2

Salvatore Previti
Salvatore Previti

Reputation: 9050

Probably you added the delegate twice, is it possible?

In this case the problem is not in who calls the delegate but in who adds the delegate to the event.

Probably you did something like...

private Class1 instance1;

void callback(...)
{
}

void myfunction()
{
    this.instance1.DataCalled += this.callback;
    this.instance1.DataCalled += this.callback;
}

If not, try to add a breakpoint where you subscribe to the event and see if it is called twice.

As a side note, you should always check for null when calling an event, if there is no subscriber you can get a NullReferenceException. I would also suggest you to use a variable to store the event delegate to avoid the risk of multithreading failure.

public void CallData()
{
    List<DataItem> dataItems = new List<DataItem>();
    var handler = this.DataCalled;
    if (handler != null)
        handler(this, dataItems);
}

Edit: since now I see the code, is obvious that each time you call the GetNewDataItems method you are subsribing every time to the event. Do in such a way you subscribe only once, for example, in constructor, or store your variable somewhere or deregister the event when you finish.

This code contains also a probable memory leak: every time you add a delegate you keep alive both the instance that contains the event and the instance that contains the subscribed method, at least, until both are unreferenced.

You can try to do something like this...

void dataHelper_DataCalled(object sender, List<DataItem> dataItemsList)
{
    // Deregister the event...
    (sender as Class1).DataCalled -= dataHelper_DataCalled; 

    //Do something with results
}

In this way however you must ensure that if there is not an exception during the event registration the event will be fired or you have again memory leaks.

Instead of an event perhaps you need just a delegate. Of course you should set your delegate field to null when you want to release the delegate.

// in data helper class

private DataHelper.DataCalledEventHandler myFunctor;

public void CallData(DataHelper.DataCalledEventHandler functor)
{
    this.myFunctor = functor;
    //SOME CODE THAT RETURNS DATA
}

// when the call completes, asynchronously...
private void WhenTheCallCompletes()
{
    var functor = this.myFunctor;
    if (functor != null)
    {
        this.myFunctor = null;
        List<DataItem> dataItems = new List<DataItem>();
        functor(this, dataItems);
    }
}
    
// in your function
...    dataHelper.CallData(this.dataHelper_DataCalled);    ...

Upvotes: 9

Related Questions