Thread safe destructible event firing class in C#

Question

Recently, I was asked to implement a class as part of a selection process. I did the program as requested. However, I failed in the test. I am really curious to know what is wrong in my solution. Any help is much appreciated. The question and my solution are given below

Question:

Implement a thread safe class which fires an event every second from construction. There need to be a function for finding the seconds elapsed. This class has to implement IDisposable and any calls to seconds elapsed function after calling dispose should fail.

My solution:

namespace TimeCounter
{
public delegate void SecondsElapsedHandler(object o, EventArgs e);
/// <summary>
/// Summary description for SecondCounter
/// </summary>
public class SecondCounter : IDisposable
{
    private volatile int nSecondsElapsed;
    Timer myTimer;
    private readonly object EventLock = new object();
    private SecondsElapsedHandler secondsHandler;
    public SecondCounter()
    {
        nSecondsElapsed = 0;
        myTimer = new Timer();
        myTimer.Elapsed += new ElapsedEventHandler(OneSecondElapsed);
        myTimer.Interval = 1000;
        myTimer.AutoReset = false;
        myTimer.Start();
    }

    public void OneSecondElapsed(object source, ElapsedEventArgs e)
    {
        try
        {
            SecondsElapsedHandler handlerCopy;
            lock (EventLock)
            {
                handlerCopy = secondsHandler;
                nSecondsElapsed++;

            }
            if (secondsHandler != null)
            {
                secondsHandler(this, e);
            }
        }
        catch (Exception exp)
        {
            Console.WriteLine("Exception thrown from SecondCounter OneSecondElapsed " + exp.Message);
        }
        finally
        {
            if (myTimer != null)
            {
                myTimer.Enabled = true;
            }
        }
    }

    public event SecondsElapsedHandler AnotherSecondElapsed
    {
        add
        {
            lock (EventLock)
            {
                secondsHandler += value;
            }
        }
        remove
        {
            lock (EventLock)
            {
                secondsHandler -= value;
            }

        }
    }

    public int SecondsElapsed()
    {
        if (this.IsDisposed)
        {
            throw new ObjectDisposedException("SecondCounter");
        }
        return nSecondsElapsed;

    }

    private bool IsDisposed = false;
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    private void Dispose(bool Disposing)
    {
        if (!IsDisposed)
        {
            if (Disposing)
            {

            }
            if (myTimer != null)
            {
                myTimer.Dispose();
            }

        }
        secondsHandler = null;
        IsDisposed = true;

    }
    ~SecondCounter()
    {
        Dispose(false);
    }
}
}

Answer 1

There are a few problems:

1. You might have been penalized for general Exception swallowing though that's not specifically related to threading issues.

2. There's a race condition on your timer.Dispose, as you could Dispose the timer before it is set Enabled again, resulting in an Exception.

3. You never set myTimer to null in Dispose.

4. You're accessing the managed class myTimer from the finalizer (disposing=false), which is a bad idea.

5. The explicit implementation of the event with locking is unnecessary. Delegates are immutable and adding/removing an event will never result in an invalid delegate state, though there can be race conditions if delegates are added/removed around the same time that the callback is fired. If you use the standard 'public event' declaration without an explicit backing private delegate, the synchronization will be handled automatically.

6. (minor point) If you're implementing the full Dispose pattern, it's customary to mark the Dispose(bool disposing) method as protected virtual, so that deriving classes can hook into the disposal mechanism. Better yet, mark your class sealed and you can eliminate the finalizer entirely.

Answer 2

Your finalizer is probably broken. It correctly passes false as the Disposing parameter. This should tell Dispose(bool) to avoid attempting to dispose other managed objects. But in that method you put:

if (Disposing)
{

}
if (myTimer != null)
{
    myTimer.Dispose();
}

So you ignore the value of Disposing. This means that you call the timer's Dispose method from the finalizer thread, when that object may already have been finalized (if it has a finalizer, which it probably does). Finalizers run in an unpredictable order. It's generally recommended to not make calls to other GC-managed objects from a finalizer.

In fact, it's usually recommended that you don't write finalizers at all these days. The question didn't ask you to write one! It's unfortunate that most tutorials about IDisposable also talk about finalizers. They're different subjects.

You also catch Exception, the universal exception base class. This means you catch things like NullReferenceException. Not usually a good idea. You also log to the console, which is not worth much in a GUI or server-based application.

You can replace:

myTimer.Elapsed += new ElapsedEventHandler(OneSecondElapsed);

with:

myTimer.Elapsed += OneSecondElapsed;

Your variable naming is inconsistent. Refer to the Microsoft guidelines.