Thread Safe Timed Events

Question

I asked a question earlier on this thread Timed events overlapping during execution The answer I got didnt really work out as I expected. I modified it a little bit based on the msdn https://msdn.microsoft.com/en-us/library/system.threading.interlocked(v=vs.110).aspx article and it worked. Or so I think. Here is my modified code

 public partial class Service1 : ServiceBase
{

    private Timer timer1 = null;
    private long isTaskRunning = 0;

    public Service1()
    {
        InitializeComponent();
    }

    protected override void OnStart(string[] args)
    {
        timer1 = new Timer();
        this.timer1.Interval = 1000;
        this.timer1.Elapsed += new  System.Timers.ElapsedEventHandler(this.timer1_Tick);
        timer1.Enabled = true;
        Library.WriteErrorLog("service has started");
    }

    private void timer1_Tick(object sender, ElapsedEventArgs e)
    {
     Console.WriteLine("Acquiring Lock");
        try
        {

        if (Interlocked.Exchange(ref isTaskRunning, 1)==1)
        {
         Console.WriteLine("Lock Denied");
         return;
        }

        else
        { 
         Console.WriteLine("Lock Acquired");
         //retrieve data from database
        //read rows

        //Loop through rows
        //send values through network
        //receive response and update db
         Interlocked.Exchange(ref isTaskRunning, 0);
        }

       }

        catch (Exception ex)
        {
            Library.WriteErrorLog(ex);
        }

    }
}

    protected override void OnStop()
    {
        timer1.Enabled = false;
        Library.WriteErrorLog("service has stopped");
    }
}

My question now is, is this code Thread-safe? And is there any possibility that this code might crash. The code is supposed to run on Virtual Private Server and is a part of business critical system. Any help would be appreciated.

Answer 1

In the first place, you want Interlocked.CompareExchange there, rather than Interlocked.Exchange. If you use Interlocked.Exchange, your code will allow reentrancy every other time. And could fail to allow the code to run when it should.

I don't know if you intended this, but if your processing throws an exception then the isTaskRunning is never reset to 0.

You can do the same thing a lot easier with a Monitor:

private object _timerLock = new object();

private void timer1_Tick(object sender, ElapsedEventArgs e)
{
    Console.WriteLine("Acquiring Lock");
    if (!Monitor.TryEnter(_timerLock))
    {
        Console.WriteLine("Lock Denied");
        return;
    }

    try
    { 
        Console.WriteLine("Lock Acquired");
        //retrieve data from database
        //read rows

        //Loop through rows
        //send values through network
        //receive response and update db
        Monitor.Exit(_timerLock);
    }
    catch (Exception ex)
    {
        Library.WriteErrorLog(ex);
    }
}

Again, this will continue to hold the lock if an exception is thrown. You could move the Monitor.Exit into a finally block, but that raises other issues. See Eric Lippert's Locks and exceptions do not mix. Using an Interlocked.CompareExchange to do the locking doesn't change the fundamental problem.

Answer 2

After reading Jim Mischel's answer and reading the linked post (Locks and exceptions do not mix), I put together a hypothetical model of how this might work.

I'd recommend that the class setting the timer and handling its events be separate from the one that invokes whatever behavior happens. Going a step further, perhaps the ComponentUsed class (didn't think too hard about that name) should itself just be a wrapper for the class that invokes some actual behavior.

That keeps responsibilities separate. One class is responsible for doing something when a timer elapses, not knowing about locks and failed states. This is important in case that behavior must be replicated somewhere other than that timer event.

The responsibility of locking, handling exceptions, and determining/knowing whether the component is in a failed state is separate.

In this case if the component fails and enters a failed state where subsequent calls shouldn't be processed, it raises an EnteredFailedState event. RunsOnTimer can respond to this by stopping its timer and doing whatever else it must do to respond. If it's a Windows service it might log the exception and shut down.

But whether the timer continues to elapse or not, the inner component is managing responsibility for its own failed state. It knows that it should not continue to handle requests.

This is just conceptual, but I might build on this if I have such a scenario where I have to both lock and handle exceptions. It doesn't matter much if the process can safely continue even if the process within the lock throws an exception.

public class RunsOnTimer
{
    private readonly Timer _timer = new Timer(1000);
    private readonly ComponentUsed _component = new ComponentUsed();

    public RunsOnTimer()
    {
        _component.EnteredFailedState += Component_EnteredFailedState;
        _timer.Elapsed += _timer_Elapsed;
    }

    private void Component_EnteredFailedState(ComponentUsed sender, EnteredFailedStateEventArgs e)
    {
        _timer.Stop();
    }

    private void _timer_Elapsed(object sender, ElapsedEventArgs e)
    {
        _component.DoSomething();
    }
}

// Some component containing the actual behavior being invoked
// when the timer elapses.

public class ComponentUsed
{
    private readonly object _doingSomethingLock = new object();
    private bool _failedState;

    public event EnteredFailedStateEventHandler EnteredFailedState;
    public void DoSomething()
    {
        if (_failedState) return; //Perhaps throw an exception.
        if (!Monitor.TryEnter(_doingSomethingLock)) return;
        try
        {
            // Do whatever the component does
        }
        catch (Exception ex)
        {
            // Is the exception fatal? Should we stop executing this?
            EnterFailedState(ex);
        }
        finally
        {
            Monitor.Exit(_doingSomethingLock);
        }
    }

    private void EnterFailedState(Exception ex)
    {
        _failedState = true;
        if (EnteredFailedState != null) EnteredFailedState(this, new EnteredFailedStateEventArgs(ex));
    }
}