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.