"timer + Task.Run" vs "while loop + Task.Delay" in asp.net core hosted service

Question

I have a requirement that background service should run Process method every day at 0:00 a.m.

So, one of my team member wrote the following code:

public class MyBackgroundService : IHostedService, IDisposable
{
    private readonly ILogger _logger;
    private Timer _timer;

    public MyBackgroundService(ILogger<MyBackgroundService> logger)
    {
        _logger = logger;
    }

    public void Dispose()
    {
        _timer?.Dispose();
    }

    public Task StartAsync(CancellationToken cancellationToken)
    {
        TimeSpan interval = TimeSpan.FromHours(24);
        TimeSpan firstCall = DateTime.Today.AddDays(1).AddTicks(-1).Subtract(DateTime.Now);

        Action action = () =>
        {
            Task.Delay(firstCall).Wait();

            Process();

            _timer = new Timer(
                ob => Process(),
                null,
                TimeSpan.Zero,
                interval
            );
        };

        Task.Run(action);
        return Task.CompletedTask;
    }

    public Task StopAsync(CancellationToken cancellationToken)
    {
        _timer?.Change(Timeout.Infinite, 0);

        return Task.CompletedTask;
    }

    private Task Process()
    {
        try
        {
            // perform some database operations
        }
        catch (Exception e)
        {
            _logger.LogError(e, e.Message);
        }
        return Task.CompletedTask;
    }
}

This code works as expected. But I don't like that it synchronously waits till calling Process first time, so a thread is blocked and not performing any useful work (correct me if I am wrong).

I could make an action async and await in it like this:

public Task StartAsync(CancellationToken cancellationToken)
{
    // code omitted for brevity

    Action action = async () =>
    {
        await Task.Delay(firstCall);

        await Process();
        
        // code omitted for brevity
}

But I am not sure that using Task.Run is a good thing here as Process method should perform some I/O operations (query DB and insert some data), and because it's not recommended to use Task.Run in ASP.NET environment.

I refactored StartAsync as follows:

public async Task StartAsync(CancellationToken cancellationToken)
{
    TimeSpan interval = TimeSpan.FromHours(24);
    TimeSpan firstDelay = DateTime.Today.AddDays(1).AddTicks(-1).Subtract(DateTime.Now);

    await Task.Delay(firstDelay);

    while (!cancellationToken.IsCancellationRequested)
    {
        await Process();

        await Task.Delay(interval, cancellationToken);
    }
}

And this allows me not to use timer in MyBackgroundService at all.

Should I use the first approach with "timer + Task.Run" or the second one with "while loop + Task.Delay"?

Berkelium

I'm not going to comment on "should I", but instead point out a minor ... flaw? issue? If Process take 5 minutes, your schedule will be skewed by 5 minutes on every execution. That is, until your service is restarted. – Newness

Why first delay is even needed? Timer already supports parameter indicating a delay before the first run. Then you can remove everything except timer creation. – Turf

Or you could just use a library designed to do exactly this? codeburst.io/… – Husted

@LasseV.Karlsen, This is a real significant flaw. Thanks you have noticed this. – Berkelium

Answer 1

The while loop approach is simpler and safer. Using the Timer class has two hidden gotchas:

1. Subsequent events can potentially invoke the attached event handler in an ovelapping manner.
2. Exceptions thrown inside the handler are swallowed, and this behavior is subject to change in future releases of the .NET Framework. (from the docs)

Your current while loop implementation can be improved in various ways though:

1. Reading the DateTime.Now multiple times during a TimeSpan calculation may produce unexpected results, because the DateTime returned by DateTime.Now can be different each time. It is preferable to store the DateTime.Now in a variable, and use the stored value in the calculations.
2. Checking the condition cancellationToken.IsCancellationRequested in the while loop could result to inconsistent cancellation behavior, if you also use the same token as an argument of the Task.Delay. Skipping this check completely is simpler and consistent. This way cancelling the token will always produce an OperationCanceledException as a result.
3. Ideally the duration of the Process should not affect the scheduling of the next operation. One way to do it is to create the Task.Delay task before starting the Process, and await it after the completion of the Process. Or you can just recalculate the next delay based on the current time. This has also the advantage that the scheduling will be adjusted automatically in case of a system-wise time change.

Here is my suggestion:

public async Task StartAsync(CancellationToken cancellationToken)
{
    TimeSpan scheduledTime = TimeSpan.FromHours(0); // midnight
    TimeSpan minimumIntervalBetweenStarts = TimeSpan.FromHours(12);

    while (true)
    {
        var scheduledDelay = scheduledTime - DateTime.Now.TimeOfDay;

        while (scheduledDelay < TimeSpan.Zero)
            scheduledDelay += TimeSpan.FromDays(1);

        await Task.Delay(scheduledDelay, cancellationToken);

        var delayBetweenStarts =
            Task.Delay(minimumIntervalBetweenStarts, cancellationToken);

        await ProcessAsync();

        await delayBetweenStarts;
    }
}

The reason for the minimumIntervalBetweenStarts is to protect from very dramatic system-wise time changes.

Playground

Thanks for all your suggestions. You said that I read DateTime.Now multiple times. But I use DateTime.Now only once when I calculate firstDelay. Am I missing something? – Berkelium

@DmitryStepanov my pleasure! Yes, the DateTime.Today also implies a reading of the DateTime.Now property (source code). – Playground

@Theodor Zoulias Hi there, Would you mind sharing how you would amend this if you needed to run ProcessAsync() every second (or less)? Appreciate any guidance you can offer. – Goodkin

Hi @ledragon. Maybe this answer is close to what you want (the PeriodicAsync method). – Playground

Instead of calculating the intervals manually, it is also possible to use a specialized library like the Cronos. You can look here for an implementation of a timer (CronosTimer) that is based on Cronos. – Playground