6
\$\begingroup\$

I've created a System.Threading.Timer wrapper in C#. The tasks to be triggered when the timer elapses have highly variable execution times. My design criteria are:

  1. Provide a strongly typed timer state in the callback
  2. Run the timer callback on a periodic basis
  3. If the timer's period elapses before the timer callback completes
    1. Do not run the timer callback a second time in parallel
    2. Run time timer callback again immediately after the running callback completes
  4. Do not run the timer callback more than once per period

As an example, given a timer that elapses every 3 seconds: 3s - Timer elapse triggers task 6s - Timer elapse triggers task, but task does not run because
the previous run is not complete 9s - Timer elapse triggers task, but task does not run because
the previous run is not complete 10s - Task finishes running 10s - Task immediately runs again because the previous run crossed
a period boundary 11s - Task finishes running 12s - Timer elapse triggers task

I ended up with the following class:

public class NonOverrunningTimer<TState>
{
    private readonly Func<TState, TState> _delegate;
    private readonly TimeSpan _interval;
    private readonly System.Threading.Timer _timer;

    public NonOverrunningTimer(Func<TState, TState> @delegate, TimeSpan interval, TState initialState = default(TState))
    {
        if (interval == TimeSpan.Zero)
        {
            throw new Exception($"Cannot initialize a timer with a period of {TimeSpan.Zero}");
        }

        State = initialState;
        _delegate = @delegate;
        _interval = interval;
        _timer = new System.Threading.Timer(UntypedCallback, null, TimeSpan.Zero, interval);
    }

    public TState State { get; private set; }

    private volatile bool _running;
    private volatile bool _waiting;
    private readonly object _executeLock = new object();
    public void UntypedCallback(object state)
    {
        if (_running && _waiting)
        {
            return;
        }

        if (_waiting && !_running)
        {
            _waiting = false;
        }

        if (_running)
        {
            _waiting = true;
            return;
        }

        using (var @lock = new TryLock(_executeLock))
        {
            if (!@lock.HasLock)
            {
                return;
            }

            _running = true;

            State = _delegate(State);

            _running = false;
            if (_waiting)
            {
                _waiting = false;
                _timer.Change(TimeSpan.Zero, _interval);
            }
        }
    }

    private class TryLock : IDisposable
    {
        private object _locked;

        public bool HasLock { get; private set; }

        public TryLock(object obj)
        {
            if (Monitor.TryEnter(obj))
            {
                HasLock = true;
                _locked = obj;
            }
        }

        public void Dispose()
        {
            if (!HasLock)
            {
                return;
            }

            Monitor.Exit(_locked);
            _locked = null;
            HasLock = false;
        }
    }
}

I'm interested in general feedback as well as whether something like this already exists in the BCL/Framework libraries and I just missed it.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$ – Phrancis Aug 29 '16 at 21:59
1
\$\begingroup\$

The TryLock class shouldn't be netested.

I'd also add a factory method to it and make it much simpler like this and create an instance only when a lock could be accuired:

private class Locker : IDisposable
{
    private object _locked;

    private Locker(object obj)
    {       
        _locked = obj;
    }

    public static Locker Create(object obj)
    {
        return Monitor.TryEnter(obj) ? new Locker(obj) : null;
    }

    public void Dispose()
    {
        Monitor.Exit(_locked);
    }
}

Later instead of checking the property you check if the instance isn't null:

using (var tryLock = Locker.Create(_executeLock))
{
    if (tryLock == null)
    {
        return;
    }

    ...
}

@variable

This kind of naming can and should really be avoided. I'm sure you can come up with a better name then just a delegate which says pretty nothing about the method.

\$\endgroup\$
  • \$\begingroup\$ Good feedback; thanks! Still learning something new every day. I had no idea the using statement performed a null check before calling Dispose(). Regarding the variable name, I literally decided I was too wiped yesterday to worry about spending more energy on coming up with a better one. I'll put some more energy into that today. \$\endgroup\$ – arootbeer Aug 30 '16 at 13:24
  • \$\begingroup\$ @arootbeer the using statement doesn't perform a null check :-( I just don't create a locker when no lock could be accuired and abort the operation if the locker is null. I find it easier to work with rather then creating a locker that actually isn't locking anything and checking its property whether it really does that. \$\endgroup\$ – t3chb0t Aug 30 '16 at 13:29
  • \$\begingroup\$ I think you've misunderstood me. What I meant was that I would have expected using((IDisposable)null) { } to throw when Dispose() was called, but the compiler ensures that it does not: stackoverflow.com/a/2522834/182654. For the same reason, I was initially concerned that using (var tryLock = Locker.Create(_executeLock)) { ... } would throw when leaving the using block if the lock could not be acquired. I realize I'm still responsible for managing the state of tryLock inside the using block. \$\endgroup\$ – arootbeer Aug 30 '16 at 13:36
  • \$\begingroup\$ @arootbeer oh, yes, now I get what you meant ;-) I've been always doing this but I had never thought about it before. \$\endgroup\$ – t3chb0t Aug 30 '16 at 13:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.