"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"?

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.