Should I block even when I'm sequentially processing

Question

I have a windows service that periodically needs to do some work. So I set up a System.Timers.Timer to do this. Lets assume that its possible that the processing time could be greater than the timer interval. Lets also assume it would be a very bad thing if this happens.

To avoid this I'm setting the AutoReset on the Timer to false and then calling start in my process.

public partial class Service : ServiceBase{

    System.Timers.Timer timer;


 public Service()
    {

    timer = new System.Timers.Timer();
    //When autoreset is True there are reentrancy problme 
    timer.AutoReset = false;


    timer.Elapsed += new System.Timers.ElapsedEventHandler(DoStuff);
}

 protected override void OnStart(string[] args)
 {

     timer.Interval = 1;
     timer.Start();

    }

 private void DoStuff(object sender, System.Timers.ElapsedEventArgs e)
 {

    Collection stuff = GetData();
    LastChecked = DateTime.Now;

    foreach (Object item in stuff)
    {
          item.Dosomthing(); //Do somthing should only be called once
     }     


    TimeSpan ts = DateTime.Now.Subtract(LastChecked);
    TimeSpan MaxWaitTime = TimeSpan.FromMinutes(5);


    if (MaxWaitTime.Subtract(ts).CompareTo(TimeSpan.Zero) > -1)
        timer.Interval = MaxWaitTime.Subtract(ts).Milliseconds;
    else
        timer.Interval = 1;

    timer.Start();





 }

Currently the code doesn't block because I know its being processed sequentially because of the AutoReset = false. But I could do it anway

lock(myLock)
{
    Collection stuff = GetData();
    LastChecked = DateTime.Now;

    foreach (Object item in stuff)
    {
          item.Dosomthing(); //Do somthing should only be called once
     }     

}

EDIT: Clarifying my question

I've contrived the service to be single threaded so I don't need a lock. If I add the lock I'm still inside my performance budget so performance isn't a reason not to.

Basically I'm weighing two sides and I'm trying to sort out what the Right Thing™ is. On the "No lock" side I'm relying on a contrivance for the correctness of my code. On the "Lock" side I would be adding unnecessary code.

Which is better?

Answer 1

You only need to lock if more than a single thread will be working with the objects. In your case, your design prevents that from ever occurring, so there is no need to lock.

The only real advantage to adding the lock, in this case, would be to prevent issues from occurring if you later change your scheduling algorithm.

Answer 2

I'd either go all-out thread safe or do no thread safety at all and just write in your documentation very clearly that the class isn't thread safe.

The worst thing is coming back a couple years later and not remembering whether or not everything's safe. Then you end up wasting a lot of time investigating your code, or worse, you end up misleading yourself.

Answer 3

Like the others have said, if its a single thread, no need for the lock. Also, you could skip the timer all together:

        TimeSpan maxInterval = new TimeSpan(0, 10, 0);
        while(true)
        {
            DateTime startTime = DateTime.UtcNow;


            //Do lots and lots of work


            TimeSpan ts = DateTime.UtcNow - startTime;
            ts = (ts > maxInterval ? new TimeSpan(0) : maxInterval-ts);
            Thread.Sleep(ts);
        }