1

How dispose System.Threading.Timer correctly?

I have got this result:

pub_MyEvent1
pub_MyEvent1
pub_MyEvent1

pub_MyEvent1
new pub_MyEvent
pub_MyEvent1
new pub_MyEvent

Code:

class Program
{
    static void Main(string[] args)
    {
        Publisher pub = new Publisher();
        pub.MyEvent += new EventHandler(pub_MyEvent1);
        Console.ReadLine();
        pub = new Publisher();
        pub.MyEvent += new EventHandler(pub_MyEvent);
        Console.ReadLine();
    }
    static void pub_MyEvent(object sender, EventArgs e)
    {
        Console.WriteLine("new pub_MyEvent");
    }
    static void pub_MyEvent1(object sender, EventArgs e)
    {
        Console.WriteLine("pub_MyEvent1");
    }
}

public class Publisher : IDisposable
{
    private System.Threading.Timer _timer;
    private EventHandler _myEvent;
    public event EventHandler MyEvent
    {
        add { _myEvent += value; }
        remove { _myEvent -= value; }
    }
    protected void OnMyEvent(Object state)
    {
        if (_myEvent != null)
        {
            foreach (EventHandler handler in _myEvent.GetInvocationList())
            {
                handler(this, new EventArgs());
            }
        }
    }
    public Publisher()
    {
        _timer = new System.Threading.Timer(new System.Threading.TimerCallback(OnMyEvent), null, 1000, 1000);
    }
    public void Dispose()
    {
        if (_timer != null)
            _timer.Dispose();
    }
}

2 Answers 2

2

You should dispose of each publisher separately.

static void Main(string[] args)  
{
  using (Publisher pub = new Publisher())  
  { 
    pub.MyEvent += new EventHandler(pub_MyEvent1); 
    Console.ReadLine(); 
  }  
  using (Publisher pub = new Publisher())  
  {
    pub.MyEvent += new EventHandler(pub_MyEvent); 
    Console.ReadLine();  
  }  
}
1
  • It should also be pointed out specifically that his dealings with timer isn't what is wrong, but his dealings with publisher.
    – McAden
    Dec 29, 2009 at 22:35
0

You could use

using(Publisher pub = new Publisher();) 
{
    pub.MyEvent += new EventHandler(pub_MyEvent1);
    Console.ReadLine();
    pub = new Publisher();
    pub.MyEvent += new EventHandler(pub_MyEvent);
    Console.ReadLine();
}

which is slightly safer than calling pub.Dispose() after your Console.ReadLine(); because the user could close the application instead of hitting enter. Also if any exception occurs, the using statement would catch the exception, dispose the timer and rethrow the exception back to you.

2
  • I agree with you about this being safer for dealing with exceptions. However, the using statement will not call dispose if the application is closed. When the process ends, that memory will be cleaned up anyways. Also, within the using statement pub is a read-only variable and cannot be reassigned. Dec 29, 2009 at 22:32
  • you're reassigning pub in the middle of your using - asside from the fact that I don't think that's allowed, that still leaves an instance of pub dangling.
    – McAden
    Dec 29, 2009 at 22:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.