HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Refreshing customer list periodically in the background

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thebackgroundperiodicallylistrefreshingcustomer

Problem

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 ();
            _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 versio

Solution

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

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

-
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  MinimumRefreshInterval)
        {
            delay = MinimumRefreshInterval;
        }
        
        return _refreshTask = Task.Run(async delegate { await Task.Delay(delay); RefreshCustomersInternal(); })
                                  .ContinueWith(t => { lock (_refreshLock)
                                                        {
                                                            _refreshTask = null;
                                                            _lastRefreshTime = DateTime.Now;
                                                        } });
    }
}

Code Snippets

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

Context

StackExchange Code Review Q#91454, answer score: 2

Revisions (0)

No revisions yet.