Refreshing customer list periodically in the background

Question

I have a method RefreshCustomersInternal that refreshes customer data from a server and returns a Task. Since this process is very costly, I want to prevent this method from being called too many times in a short time or prevent the process from running if there's another refresh process running already. So a wrapper method RefreshCustomers is written for this purpose:

private Task _refreshTask;
private readonly object _refreshLock = new object();
private DateTime _lastRefreshTime = DateTime.MinValue;
private static readonly TimeSpan MinimumRefreshInterval = TimeSpan.FromSeconds(5);

private Task RefreshCustomers()
{
    lock (_refreshLock)
    {
        // If there is already a refresh task, just return this one.
        if (_refreshTask != null)
            return _refreshTask;

        // Check if the time between this refresh request and the previous one is too close.
        var intervalFromLastRefresh = DateTime.Now - _lastRefreshTime;
        if (intervalFromLastRefresh < MinimumRefreshInterval)
        {
            // If this refresh request is too close to the previous one,
            // make this request wait until the minimum refresh interval is passed,
            // then perform the refresh.
            var waitTaskSource = new TaskCompletionSource<bool>();
            _refreshTask = waitTaskSource.Task.ContinueWith(_=>RefreshCustomersInternal()).Unwrap();

            // Force the wait in a background thread to ensure a non-blocking UI experience.
            ThreadPool.QueueUserWorkItem(_ =>
            {
                Thread.Sleep(MinimumRefreshInterval - intervalFromLastRefresh);
                waitTaskSource.SetResult(true);
            });
        }
        else
            // If this refresh request is far enough from the last one,
            // just perform the normal refresh.
            _refreshTask = RefreshCustomersInternal();

        // When the refresh process is done, always clear the refresh task reference
        // and update the refresh timestamp.
        return _refreshTask = _refreshTask.ContinueWith(task =>
        {
            lock (_refreshLock)
            {
                _refreshTask = null;
                _lastRefreshTime = DateTime.Now;
            }
        });
    }
}

In a previous version of this method, instead of using the ThreadPool, it was simply just calling the Thread.Sleep method in a new Task:

if (intervalFromLastRefresh < MinimumRefreshInterval)
    _refreshTask = Task.Factory.StartNew(() =>
    {
        Thread.Sleep(MinimumRefreshInterval - internvalFromLastRefresh);
        return RefreshCustomersInternal();
    }).Unwrap();

Since there is no guarantee that Task.Factory.StartNew will never start the new task on the UI thread, I updated it to use the ThreadPool to force the wait into a background thread.

The method seems working from some basic tests, but I wonder if there is any better way for the part where I had to use the ThreadPool.QueueUserWorkItem to force the wait in a background thread?

BTW, the code runs in .NET 4.0 hence the lack of all the neat async and await statements.

Answer 1

1. You are probably right in not using Task.Factory.StartNew. You should prefer Task.Run instead. Task.Run explicitly uses the default task scheduler which targets the thread pool while StartNew uses whatever the last used scheduler was.

2. I'm not sure you really need to distinguish the case between "I have to wait before refresh" and "I can run refresh now". The second one is just a special case of the first one with a delay of 0. Yes you may waste a scheduling cycle or two due to calling Delay but will 20ms or so really make a difference?

3. You have a bug: If your application runs when the system clock gets adjusted for DST the time can go backwards by 1h between one check an the next. The result will be that intervalFromLastRefresh is going to be negative in which case you will wait up to an hour extra before refreshing (we just hit this exact bug in one of our apps recently so the pain is still fresh ...).

With the above in mind you could simplify your implementation to:

private Task RefreshCustomers()
{
    lock (_refreshLock)
    {
        // If there is already a refresh task, just return this one.
        if (_refreshTask != null)
            return _refreshTask;

        // Check if the time between this refresh request and the previous one is too close.
        var intervalFromLastRefresh = DateTime.Now - _lastRefreshTime;
        
        var delay = MinimumRefreshInterval - intervalFromLastRefresh;
        if (delay < TimeSpan.Zero)
        {
            delay = TimeSpan.Zero;
        }
        else if (delay > MinimumRefreshInterval)
        {
            delay = MinimumRefreshInterval;
        }
        
        return _refreshTask = Task.Run(async delegate { await Task.Delay(delay); RefreshCustomersInternal(); })
                                  .ContinueWith(t => { lock (_refreshLock)
                                                        {
                                                            _refreshTask = null;
                                                            _lastRefreshTime = DateTime.Now;
                                                        } });
    }
}