Async friendly Timer

Question

I'm trying to implement a Timer that schedule an async action with the following requirements:

• thread safe
• only one action should be running at any given time (period countdown should start when the previous action has been terminated)
• the action should be cancellable

Here's the code, can you please review it and point out any flaw you can find?

using System;
using System.Threading;
using System.Threading.Tasks;

namespace Utils.Timers
{
    public sealed class TimerAsync : IDisposable
    {
      private readonly Func<CancellationToken, Task> _scheduledAction;
      private readonly TimeSpan _dueTime;
      private readonly TimeSpan _period;
      private CancellationTokenSource _cancellationSource;
      private Task _scheduledTask;
      private bool _isStarted;
      private readonly SemaphoreSlim _startSemaphore = new SemaphoreSlim(1);

      public event EventHandler<Exception> OnError;

      public TimerAsync(Func<CancellationToken, Task> scheduledAction, TimeSpan dueTime, TimeSpan period)
      {
        _scheduledAction = scheduledAction ?? throw new ArgumentNullException(nameof(scheduledAction));

        if(dueTime < TimeSpan.Zero)
          throw new ArgumentOutOfRangeException(nameof(dueTime), "due time must be equal or greater than zero");
        _dueTime = dueTime;

        if(period < TimeSpan.Zero)
          throw new ArgumentOutOfRangeException(nameof(period), "period must be equal or greater than zero");
        _period = period;
      }

      public void Start()
      {
        _startSemaphore.Wait();

        try
        {
          if (_isStarted)
            return;

          _cancellationSource = new CancellationTokenSource();

          _scheduledTask = Task.Run(async () =>
          {
            try
            {
              await Task.Delay(_dueTime, _cancellationSource.Token);

              while (true)
              {
                await _scheduledAction(_cancellationSource.Token);
                await Task.Delay(_period, _cancellationSource.Token);
              }
            }
            catch (OperationCanceledException) { }
            catch (Exception ex)
            {
              OnError?.Invoke(this, ex);
            }
          }, _cancellationSource.Token);

          _isStarted = true;
        }
        finally
        {
          _startSemaphore.Release();
        }
      }

      public async Task Stop()
      {
        await _startSemaphore.WaitAsync();

        try
        {
          if (!_isStarted)
            return;

          _cancellationSource?.Cancel();

          if (_scheduledTask != null)
            await _scheduledTask;
        }
        catch (OperationCanceledException) { }
        finally
        {
          _isStarted = false;
          _startSemaphore.Release();
        }
      }

      public void Dispose()
      {
        _cancellationSource?.Dispose();
        _startSemaphore?.Dispose();
      }
    }
}

Final implementation

using System;
using System.Threading;
using System.Threading.Tasks;

namespace Utils.Timers
{
  /// <summary>
  /// Async friendly Timer implementation.
  /// Provides a mechanism for executing an async method on
  /// a thread pool thread at specified intervals.
  ///
  /// This class cannot be inherited.
  /// </summary>
  public sealed class TimerAsync : IDisposable
  {
    private readonly Func<CancellationToken, Task> _scheduledAction;
    private readonly TimeSpan _dueTime;
    private readonly TimeSpan _period;
    private CancellationTokenSource _cancellationSource;
    private Task _scheduledTask;
    private readonly SemaphoreSlim _semaphore;
    private bool _disposed;
    private readonly bool _canStartNextActionBeforePreviousIsCompleted;

    /// <summary>
    /// Occurs when an error is raised in the scheduled action
    /// </summary>
    public event EventHandler<Exception> OnError;

    /// <summary>
    /// Gets the running status of the TimerAsync instance. 
    /// </summary>
    public bool IsRunning { get; private set; }

    /// <summary>
    /// Initializes a new instance of the TimerAsync. 
    /// </summary>
    /// <param name="scheduledAction">A delegate representing a method to be executed.</param>
    /// <param name="dueTime">The amount of time to delay befoe scheduledAction is invoked for the first time.</param>
    /// <param name="period">The time interval between invocations of the scheduledAction.</param>
    /// <param name="canStartNextActionBeforePreviousIsCompleted">
    ///   Whether or not the interval starts at the end of the previous scheduled action or at precise points in time. 
    /// </param>
    public TimerAsync(Func<CancellationToken, Task> scheduledAction, TimeSpan dueTime, TimeSpan period, bool canStartNextActionBeforePreviousIsCompleted = false)
    {
      _scheduledAction = scheduledAction ?? throw new ArgumentNullException(nameof(scheduledAction));

      if (dueTime < TimeSpan.Zero)
        throw new ArgumentOutOfRangeException(nameof(dueTime), "due time must be equal or greater than zero");
      _dueTime = dueTime;

      if (period < TimeSpan.Zero)
        throw new ArgumentOutOfRangeException(nameof(period), "period must be equal or greater than zero");
      _period = period;

      _canStartNextActionBeforePreviousIsCompleted = canStartNextActionBeforePreviousIsCompleted;

      _semaphore = new SemaphoreSlim(1);
    }

    /// <summary>
    /// Starts the TimerAsync.
    /// </summary>
    public void Start()
    {
      if (_disposed)
        throw new ObjectDisposedException(GetType().FullName);

      _semaphore.Wait();

      try
      {
        if (IsRunning)
          return;

        _cancellationSource = new CancellationTokenSource();
        _scheduledTask = RunScheduledAction();
        IsRunning = true;
      }
      finally
      {
        _semaphore.Release();
      }
    }

    /// <summary>
    /// Stops the TimerAsync.
    /// </summary>
    /// <returns>A task that completes when the timer is stopped.</returns>
    public async Task Stop()
    {
      if (_disposed)
        throw new ObjectDisposedException(GetType().FullName);

      await _semaphore.WaitAsync().ConfigureAwait(false);

      try
      {
        if (!IsRunning)
          return;

        _cancellationSource.Cancel();

        await _scheduledTask.ConfigureAwait(false);
      }
      catch (OperationCanceledException) { }
      finally
      {
        IsRunning = false;
        _semaphore.Release();
      }
    }

    private Task RunScheduledAction()
    {
      return Task.Run(async () =>
      {
        try
        {
          await Task.Delay(_dueTime, _cancellationSource.Token).ConfigureAwait(false);

          while (true)
          {
            if (_canStartNextActionBeforePreviousIsCompleted)
#pragma warning disable 4014
                _scheduledAction(_cancellationSource.Token);
#pragma warning restore 4014
              else
              await _scheduledAction(_cancellationSource.Token).ConfigureAwait(false);

            await Task.Delay(_period, _cancellationSource.Token).ConfigureAwait(false);
          }
        }
        catch (OperationCanceledException) { }
        catch (Exception ex)
        {
          try
          {
            OnError?.Invoke(this, ex);
          }
          catch
          {
              // ignored
            }
        }
        finally
        {
          IsRunning = false;
        }
      }, _cancellationSource.Token);
    }

    private void Dispose(bool disposing)
    {
      if (_disposed)
        return;

      // NOTE: release unmanaged resources here

      if (disposing)
      {
        _cancellationSource?.Dispose();
        _semaphore?.Dispose();
      }

      _disposed = true;
    }

    /// <summary>
    /// Releases all resources used by the current instance of TimerAsync.
    /// </summary>
    public void Dispose()
    {
      Dispose(true);
      GC.SuppressFinalize(this);
    }

    ~TimerAsync()
    {
      Dispose(false);
    }
  }
}

Answer 1

The API would really benefit from some inline documentation (///), but mostly looks good.

Threading

I'm no expert but it looks fine to me. The only thing that is a bit frightening is _isStarted, because it depends on the semaphore injecting serious memory barriers, and a spin-wait (which the semaphore is free to do) might not cause a full barrier, but I think in practise it does. Realised that doesn't make sense

Naming

period is not doing what I would expect it to do. "Period", to me, says that it will be run at this interval (e.g. subtract the run-time from the period when you delay), rather than that there will be a straight-up delay of this duration between executions. I'd consider renaming it, but inline documentation would of course help. dueTime is also a bit odd. To me it sounds like a point in time, not an interval.

I'd consider isRunning instead of isStarted, and _startSemaphore does more than coordinate starting, so I'd be inclined to give it a more general name.

Everything else looks good.

Func<CancellationToken, Task>

Personally I would avoid using Action and Func in all but the most general public APIs. I'd be strongly inclined to define my own generic delegate with a suitable name and use that instead.

TimerAsync.ctor

The input sanitisation looks great.

One thing, however: if the constructor throws, then you are left with an undisposed SempahoreSlim. Easily solved by moving the initialisation of the semaphore to the end of the constructor.

Start()

Is there a reason this isn't async like Stop()?

I'd be inclined to pull out the definition of _scheduledTask into a new method.

I think there may be a bug/unintended behaviour in the exception handling. If the action throws, then it doesn't mark itself as not-started, and doesn't restart itself. This would force you to call Stop before you can call Start again. Code that produces this unexpected behaviour:

public static void Testing()
{
    bool crashOnce = true;

    async Task action(CancellationToken ct)
    {
        Console.WriteLine("Running");
        if (crashOnce)
        {
            crashOnce = false;
            throw new Exception("Crash");
        }
    }

    TimerAsync timer = new TimerAsync(action, TimeSpan.Zero, TimeSpan.FromSeconds(0.1));
    timer.OnError += (o, e) => { Console.WriteLine("Error"); };
    timer.Start();
    timer.Start();

    Console.ReadKey(true);

    timer.Stop().Wait();
    timer.Start();
}

I don't know what the behaviour should be, but this seems wrong, not least because it is impossible (and wouldn't make sense) query whether the task is running. It will behave the same way if the task decides to throw OperationCanceledException() for any reason, so Stop() may not be in position to deal with it.

A solution (if there is a problem) might be as simple as adding a finally with _isStarted = false to the try within _scheduledTask.

Stop()

if (_scheduledTask != null)
    await _scheduledTask;

_scheduledTask can not be null here under correct usage, so remove the check: all it can do is obscure insidious bugs later.

Dispose

This is not a 'nice' implementation of IDisposable, but assuming the underlying types implement it well, then it's fine.

Since Start() and Stop() will no-longer work, should this also cancel the operation? Indeed, they ought to throw ObjectDisposedExceptions when called on a disposed instance.