Locking when using a System.Timers.Timer instance and event handler for the Elapsed event

Question

I've a class in a service that does interval based actions.

It uses a variable pollTimer of type System.Timers.Timer and an event method executing the actions.

Given these event methods running from a thread pool, and the accessing of the timer itself can be from multiple threads, I want to protect access to it.

Is my basic idea below is sound enough?

Below is my basic idea for both Action and Func based access and usage.

(Note: Edit history shows the step by step changes as suggested in the comments)

    private readonly object pollTimerLock = new object();
    private Timer pollTimer;

    private enum NullPollTimerFailureMode { Continue, Fail };

    private void RunOnLockedPollTimer(NullPollTimerFailureMode nullPollTimerFailureMode, Action<Timer> action)
    {
        Timer timer = pollTimer;
        if (null == timer)
        {
            if (NullPollTimerFailureMode.Fail == nullPollTimerFailureMode)
                throw new GenericException<SimplePoll>("unexpected null pollTimer");
            // else: NOP
        }
        else
        {
            lock (pollTimerLock)
                action(timer);
        }
    }

    private TResult RunOnLockedPolltimer<TResult>(TResult defaultResult, Func<Timer, TResult> action)
    {
        TResult result = defaultResult;
        RunOnLockedPolltimer(NullPollTimerFailureMode.Continue, 
            lockedPolltimer =>
            {
                result = action(lockedPolltimer);
            });
        return result;
    }

(methods are private right now, as it's internal for the class encapsulating the behaviour)

And some access patterns:

Obtaining state:

            bool result = RunOnLockedPolltimer(false, 
                lockedPolltimer => lockedPolltimer.Enabled);

Disabling timer:

        RunOnLockedPolltimer(NullPollTimerFailureMode.Continue,
            lockedPolltimer => 
                lockedPolltimer.Enabled = false);

Enabling timer:

        RunOnLockedPolltimer(NullPollTimerFailureMode.Fail,
            lockedPolltimer =>
            {
                lockedPolltimer.AutoReset = true;
                lockedPolltimer.Enabled = true;
            });

Event: should I use RunOnLockedPolltimer here or not?

    private void PollTimerElapsed(object sender, ElapsedEventArgs e)
    {
        try
        {
            if (!IsPolling)
                return;

            // Ensure only one event on the timer can run at the same time
            RunOnLockedPollTimer(NullPollTimerFailureMode.Fail,
                lockedPollTimer => LockedPollTimerElapsed());
        }
        catch (Exception exception)
        {
            try
            {
                Logger.LogException("PollTimerElapsed", InformationLevel.Critical, exception);
            }
            catch
            {
                // if logging fails, don't let exceptions exit the PollTimerElapsed eventhandler.
            }
        }
    }

It is not clear what you are protecting against, also locking on a local variable has no effect as each thread will have it's own copy, so no locking will ever occur. — Ben Robinson, Dec 12 '14 at 10:04
This excessive fear for null references is a very bad idea. Never hide a gross bug in a program. Never use *lock* on a timer, always use AutoReset = false if you think such a lock might be necessary. It usually is. And never use System.Timers.Timer without try/catch in the Elapsed event handler. — Hans Passant, Dec 12 '14 at 10:13
@HansPassant lets address those from the end, one by one. Do you mean [The Timer component catches and suppresses all exceptions thrown by event handlers for the Elapsed event. This behavior is subject to change in future releases of the .NET Framework.](http://msdn.microsoft.com/en-us/library/system.timers.timer)? If so, from which .NET version on is this needed? — Jeroen Pluimers - Binck, Dec 12 '14 at 10:52
@HansPassant then the `AutoReset`: how to handle this case then [Note If `Enabled` and `AutoReset` are both set to `false`, and the timer has previously been enabled, setting the `Interval` property causes the `Elapsed` event to be raised once, as if the `Enabled` property had been set to `true`](http://msdn.microsoft.com/en-us/library/system.timers.timer.interval.aspx) — Jeroen Pluimers - Binck, Dec 12 '14 at 11:02
@BenRobinson the `LockedPollTimerElapsed` can change the timer settings. Maybe it should do a local lock there? — Jeroen Pluimers - Binck, Dec 12 '14 at 11:11
@JeroenPluimers-Binck You should never do a "local lock", a thread will enter a lock if no other thread has a lock on the same object. If each thread locks on a local variable then they will each get their own local variable instance and always enter the lock. Lock on a field so there is one object to lock on per class instance. — Ben Robinson, Dec 12 '14 at 11:17
@BenRobinson so `Timer timer = pollTimer; lock (timer) { }` is different from `lock (pollTimer) { }` ? I expected both to be the same given they `Timer` is a reference type, note a value type. — Jeroen Pluimers - Binck, Dec 12 '14 at 11:27
Yes it is, the first won't do anything useful but I wouldn't do either, i would do something likw `lock(somededicatelockkingfield){}` — Ben Robinson, Dec 12 '14 at 11:29
@BenRobinson so make it explicit by `private readonly object pollTimerLock = new object();` then `lock(pollTimerLock) { }` right? — Jeroen Pluimers - Binck, Dec 12 '14 at 11:32
No idea what else you want beyond what's already been written in *hundreds* of posts about that cr*ppy class. Don't use it. — Hans Passant, Dec 12 '14 at 16:05
This question appears to be off-topic because it belongs on codereview.stackexchange.com — Peter Duniho, Dec 12 '14 at 18:43
@HansPassant now that's an interesting point. I tried to search for that, but could not find it (otherwise I'd have written a totally different question). What structure should I have used in stead? — Jeroen Pluimers - Binck, Dec 13 '14 at 07:12

Locking when using a System.Timers.Timer instance and event handler for the Elapsed event

0 Answers0