Proper use of timer in windows service

Question

I have following code to use timer (System.Timers.Timer) in windows service. The goal is that new time handler should not occur if previous one didn't finish its job. Here is how I achieve this:

protected override void OnStart(string[] args)
{

            try
            {
                //
                // Create and start a timer.
                //
                m_mainTimer = new System.Timers.Timer();
                m_mainTimer.Interval = 60000;   // every one min
                m_mainTimer.Elapsed += new System.Timers.ElapsedEventHandler(this.timer1_Tick);
                m_mainTimer.AutoReset = false;  // makes it fire only once
                m_mainTimer.Enabled = true;
            }
            catch (Exception ex)
            {
              // omitted
            }

}

And:

protected override void OnStop()
    {
        try
        {
            // Service stopped. Also stop the timer.
            m_mainTimer.Enabled = false;
            m_mainTimer = null;

        }
        catch (Exception ex)
        {

        }


    }

Also in handler:

private void timer1_Tick(object sender, ElapsedEventArgs e)
        {


            try
            {
            }catch(Exceptione x)
            {
            }
            finally
            {

            if (null != m_mainTimer)
            {
                m_mainTimer.Start(); // re - enable the timer
            }
            }
}

It works but my question is is this approach safe? Maybe I should make m_mainTimer volatile? Because of the null check inside finally?

Answer 1

Ok, had a few suggestions with the code for you to avoid some confusion and maintain some consistency. I'll start off with the code sample and add some comments after the code block:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Diagnostics;
using System.Linq;
using System.ServiceProcess;
using System.Text;
using System.Threading.Tasks;

namespace WindowsService1
{
    public partial class Service1 : ServiceBase
    {
        public Service1()
        {
            InitializeComponent();
        }

        private System.Timers.Timer m_mainTimer;
        private bool m_timerTaskSuccess;

        protected override void OnStart(string[] args)
        {
            try
            {
                //
                // Create and start a timer.
                //
                m_mainTimer = new System.Timers.Timer();
                m_mainTimer.Interval = 60000;   // every one min
                m_mainTimer.Elapsed += m_mainTimer_Elapsed;
                m_mainTimer.AutoReset = false;  // makes it fire only once
                m_mainTimer.Start(); // Start

                m_timerTaskSuccess = false;
            }
            catch (Exception ex)
            {
                // omitted
            }
        }

        protected override void OnStop()
        {
            try
            {
                // Service stopped. Also stop the timer.
                m_mainTimer.Stop();
                m_mainTimer.Dispose();
                m_mainTimer = null;
            }
            catch (Exception ex)
            {
                // omitted
            }
        }

        void m_mainTimer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
        {
            try
            {
                // do some work

                m_timerTaskSuccess = true;
            }
            catch (Exception ex)
            {
                m_timerTaskSuccess = false;
            }
            finally
            {
                if (m_timerTaskSuccess)
                {
                    m_mainTimer.Start();
                }
            }
        }        
    }
}

System.Windows.Forms.Timer versus System.Timers.Timer

At one point in your code it is clear you dragged a Timer component into the design grid of the service, clicked it creating the Tick event, and then replaced with a System.Timers.Timer instead. I recommend sticking with either one or the other, and thus to maintain clarity I replaced your timer1_Tick event handler with m_mainTimer_Elapsed, since System.Timers.Timer utilize the Elapsed event instead of the Tick event.

Use of a boolean success indicator

Versus setting the timer to null when the Service is stopped, which at that point it is already too late to affect your running event, utilize a private boolean to hold whether success is achieved (dependent on how you define success in your project's domain). If an exception occurs during the running of the task, set it to false. Then utilize an if block to only restart the timer if the last run was successful.

You do NOT need volatile

You are only ever modifying the timer in one thread. volatile is utilized when multiple threads could potentially modify the same reference variable.