4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ What does RefreshCustomersInternal look like? Have tried refreshing once, waiting half the interval and then spamming refresh requests? It looks to me like you'd let extra refreshes in. \$\endgroup\$ – RobH May 22 '15 at 12:09
  • \$\begingroup\$ That method is making a standard WebRequest to download data from server and saving them into a local DB, which takes a few seconds. The RefreshCustomers method works because the first line in the lock checks if there is already a _refreshTask and returns it if there is. This ensures that all calls to this method will get the same Task instance before the task is set to null which only happens after the real refresh process is finished. And because the code sets _refreshTask uses the same lock, it guarantees thread safety of the variable. \$\endgroup\$ – Leon Zhou May 24 '15 at 23:48
  • \$\begingroup\$ The waiting bit is also thread safe IMHO as waitTaskSource.Task.ContinueWith is assigned to _refreshTask in the same scope of the first lock, and waitTaskSource is only set to finish when the background wait is elapsed. \$\endgroup\$ – Leon Zhou May 24 '15 at 23:53
  • \$\begingroup\$ Ah yes, I see it now - it does look thread safe to me too. \$\endgroup\$ – RobH May 25 '15 at 8:09
  • \$\begingroup\$ A comment like "it does look thread safe to me" doesn't give me much confidence. Writing multi threaded code is extremely difficult to get right and you either know it or you don't :) Try not to reinvent the wheel, what you're doing has been solved by others several times over. Use a well tested library such as Redis. \$\endgroup\$ – Razor Aug 28 '15 at 10:49
2
\$\begingroup\$
  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;
                                                        } });
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.