1

I'm writing a simplified asynchronous event driven Timer class. Just wondering if this will work under all conditions and if it's thread safe. IE, any chance of it failing during invoke or reading the Enabled property or setting the AutoReset feature.

namespace Sandbox
{
    public class AsyncTimer
    {
        public volatile bool AutoReset = true;
        volatile bool enabled = false;
        public volatile int Interval;

        CancellationTokenSource? cts;

        public volatile Action? Elapsed;

        public bool Enabled { get { return enabled; } }

        public AsyncTimer (int interval) => Interval = interval;

        public void Start(bool startElapsed = false)
        {
            if (startElapsed) Elapsed?.Invoke();
            enabled = true;
            cts = new();
            _ = Task.Run(() => RunTimerAsync());
        } 

        public void Stop()
        {
            enabled = false;
            cts?.Cancel();
        }

        async void RunTimerAsync()
        {
            while (enabled && !cts!.IsCancellationRequested)
            {
                await Task.Delay(Interval);
                Elapsed?.Invoke();
                if (!AutoReset) cts.Cancel();
            }
        }
    }
}
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
Chroma
  • 77
  • 1
  • 6
  • I suggest you don’t use `async void` – stuartd Feb 18 '22 at 14:43
  • 2
    How is this different than the [Timer classes](https://learn.microsoft.com/en-us/dotnet/standard/threading/timers) that already exist? – Gabriel Luci Feb 18 '22 at 14:43
  • I believe the System.Timers class uses a thread for updating the timers. I just trying a different method. – Chroma Feb 18 '22 at 14:47
  • What do you mean by "updating the timers"? It does run the Elapsed event on a different thread, but so are you by using `Task.Run`. – Gabriel Luci Feb 18 '22 at 14:48
  • "I suggest you don’t use async void" What's the benefit of putting Task there instead of void? – Chroma Feb 18 '22 at 14:52
  • You use the returned `Task` so you know when the task finishes, or _if_ it finishes. Using `async void` is what is commonly called "fire and forget". It should only be used for events. – Gabriel Luci Feb 18 '22 at 15:05
  • Microsoft has some pretty well-written articles on [Asynchronous programming with async and await](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/async/). I suggest you read through those. The fourth article is about [Async return types](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/async/async-return-types), which addresses your question about `Task` vs `void`. – Gabriel Luci Feb 18 '22 at 15:07
  • In .NET 6 there is also a new [`PeriodicTimer`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.periodictimer) class, that you might want to check out. [Here](https://stackoverflow.com/questions/30462079/run-async-method-regularly-with-specified-interval/62724908#62724908) is a usage example of this class. – Theodor Zoulias Feb 18 '22 at 15:47

1 Answers1

1

As far as I can see, this is just a wrapper around Threading.Timer with a bunch of extra stuff around it that does not add any actual functionality. Your timer works by calling Task.Delay, but this is just a wrapper around Threading.Timer, so you might as well cut out the middleman.

Most the functionality you expose is already provided by this timer by calling the .Change method. If you want to provide a more intuitive interface I would suggest wrapping this timer, or provide some extension methods, instead.

If you want the behavior that guarantees that the event is not raised concurrently, and that the execution-time is added to the period time, you should wrap the timer and set some due-time and an infinite period. Then at the end of your event handler you would call .Change again to restart the timer.

If you write a simple wrapper around Threading.Timer you will have a much easier time ensuring thread safety, since the Threading.Timer is thread safe.

As it is, I think your class is probably kind of thread safe. But I'm fairly sure it can cause some behavior that is unexpected. For example, calling .Start() multiple times would cause multiple loops to be started. I would have expected such a method to be idempotent.

JonasH
  • 28,608
  • 2
  • 10
  • 23
  • Well I just looked at System.Threading.Timer and it's pretty straight forward. Seems like the best option for what I'm trying to do. No sense in reinventing the wheel again. Thanks for the advice. – Chroma Feb 18 '22 at 15:22