1

I would like to start an asynchronous operation with the creation of an object and for it to run at intervals for the lifetime of the object. I have one implementation of how this might be done (see code below); but would like feedback on how it might be improved upon.

An example of how I might use this class would would be an object that manages & tracks the state of some piece of hardware and needs to query its state (e.g. via serial or tcp queries) at intervals. I don't want the call/creator of the object to have to manage this.

I am trying to follow the best practices of Steven Cleary as laid out in his many articles on the subject. These would seem to indicate I should avoid the use of Task.Start and .ContinueWith for this. At the same time I want to be able to sensibly handle errors that may occur while the background task is being running.

class Worker
{
    private Task _BGWork;

    public Worker()
    {
        _BGWork = BackgroundWork();
        // If i were to use ContinueWith, next would be ...
        // _BGWORk.ContinueWith(HandleError, TaskContinuationOptions.OnlyOnFaulted);
    }

    private async Task BackgroundWork()
    {
        try
        {
            while (true)
            {
                // do work... update Worker state etc...
                // use .ConfigureAwait(false) here when possible
                await Task.Delay(TimeSpan.FromSeconds(1));
            }
        }
        catch (Exception) // if not using ContinueWith, can't let any errors escape as they will be silently ignored
        {
            HandleError();
        }
    }

    private void HandleError()
    { // 'Fix' the glitch
    }
    public void Dispose()
    { // clean up the task if needed
    }

Thanks for any feedback.

Adam V. Steele
  • 559
  • 4
  • 18

2 Answers2

2

I like your implementation. Using a loop with Task.Delay has some advantages over a Timer. A timer will fire every second even if each work lasts more than a second, resulting to overlapping invocations of the callback function. On the contrary your approach ensures that there will be exactly one second of idle time between one work and the next, giving time to the CPU to do other things.

Possible improvements:

  1. Create the Task.Delay task before starting each job, and await it after the job is done, to maintain a consistent interval between subsequent jobs.
  2. Move the try-catch block inside the loop, so that an exception won't kill the task.
  3. Start the worker task with Task.Run, otherwise the first loop will run in the UI thread.
  4. Provide a way for the loop to end. The Dispose method seems suitable for this.
  5. Pay attention to thread synchronization, because the Task.Delay will cause each loop to run in a different thread-pool thread. Use lock, volatile or Interlocked for variables that can be accessed by multiple threads.
    class Worker : IDisposable
    {
        private readonly Task _mainTask;
        private readonly CancellationTokenSource _cts = new CancellationTokenSource();

        private volatile Exception _lastException;
        private int _exceptionsCount;

        public Task Completion => _mainTask;
        public Exception LastException => _lastException;
        public int ExceptionsCount => Interlocked.Add(ref _exceptionsCount, 0);

        public Worker()
        {
            _mainTask = Task.Run(BackgroundWork);
        }

        private async Task BackgroundWork()
        {
            while (!_cts.IsCancellationRequested)
            {
                var delayTask = Task.Delay(1000, _cts.Token);

                try
                {
                    Console.WriteLine("Do work");
                }
                catch (Exception ex)
                {
                    _lastException = ex;
                    Interlocked.Increment(ref _exceptionsCount);
                }

                try
                {
                    await delayTask.ConfigureAwait(false);
                }
                catch (OperationCanceledException)
                {
                    break;
                }
            }
        }

        public void Dispose()
        {
            _cts.Cancel();
        }
    }
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • This code looks good. If you ever need to, you can solve the overlapping invocations by a `Timer` by using [`System.Timers.Timer`](https://docs.microsoft.com/en-us/dotnet/api/system.timers.timer?view=netcore-2.2), (instead of [`System.Threading.Timer`](https://docs.microsoft.com/en-us/dotnet/api/system.threading.timer?view=netcore-2.2)) setting [`AutoReset`](https://docs.microsoft.com/en-us/dotnet/api/system.timers.timer.autoreset?view=netcore-2.2) to `false`, and reset the timer when the invocation is complete. You just have to be extra sure to catch all exceptions or your timer won't reset. – Gabriel Luci Sep 12 '19 at 12:51
  • 1
    You dont need `Use lock, volatile or Interlocked ` if this codepath is accessed only by one thread at a time, it does not matter on which thread it resumes, as long as it is always one thread the read and writes move predictably. Any kind of locking will hurt performance. And you can pass token as a parameter to the task. – KreonZZ Apr 12 '21 at 12:13
  • @KreonZZ yes, when I wrote this answer I was not aware that certain operations [generate implicit memory barriers](https://stackoverflow.com/questions/6581848/memory-barrier-generators). – Theodor Zoulias Apr 12 '21 at 12:45
1

Use a Timer, which will run your code on a thread pool thread, off of the main thread.

When it comes to whether you need to use async/await: If it's a desktop or console app, then it doesn't matter as long as you're off the UI thread. The only concern here is not locking the UI thread. You can create lots of threads and lock them up and it won't make a difference as long as it's not the UI thread.

If this is ASP.NET Core, then you don't want to hold up threads since you have a very small number of threads. But you can still use a Timer. The Microsoft article called Background tasks with hosted services in ASP.NET Core shows how to setup a background task that starts a timer. A thread will only be used when the timer is elapsed.

You can make the timer callback method async, but it would be async void, so you would have to make extra sure that no unhandled exceptions happen there or it'll kill your whole app.

Gabriel Luci
  • 38,328
  • 4
  • 55
  • 84