Is my AsyncTimer class implementation thread safe?

Question

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();
            }
        }
    }
}

Answer 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.