(async) Task “Keeper” for keeping the “fresh” data - How do I improve this?

Question

Background

I retrieve data from a 3rd party API, over HTTP. It's often slow, and sometimes just fails. I needed a class that would keep getting data from that source, and keep it in memory.

So, this "TaskKeeper" class's purpose is:

• Have a timer (interval lengths are parameters of the class)
• Every timer-tick, perform a "getter" function, on a separate thread, as a "task" (don't hang the current thread).
• save the data as a property. If the data was received, the old data is discarded, if not, keep the old data.

The Question

I'm asking, more specifically, if I can improve the code which handles the "Task" object, but I would appreciate any helpful comments, about any of the code!

The Code

public class TaskKeeper<TParam, TResult> : IDisposable
    where TParam : class
{
    // ReSharper disable StaticFieldInGenericType
    //By design: Different Locker object, per type of task keeper.
    private static readonly object Locker = new object();
    private static readonly object Locker2 = new object();
    // ReSharper restore StaticFieldInGenericType

    private readonly Func<ITaskKeeperFuncParam<TParam>, ITaskKeeperFuncResult<TResult>> _dataGetter;
    private readonly TParam _parameter;
    private CancellationTokenSource _cancellationTokenSource;
    private Task<ITaskKeeperFuncResult<TResult>> _task;
    private Timer _timer;

    public TaskKeeper(Func<ITaskKeeperFuncResult<TResult>> dataGetter, int timerInterval, int dueTime,
                      TParam parameter)
    {
        _parameter = parameter;
        Func<ITaskKeeperFuncParam<TParam>, ITaskKeeperFuncResult<TResult>> noParamFunc = arg => dataGetter();
        _dataGetter = noParamFunc;
        SetTimer(timerInterval, dueTime);
    }

    public TaskKeeper(Func<ITaskKeeperFuncParam<TParam>, ITaskKeeperFuncResult<TResult>> dataGetter,
                      int timerInterval, int dueTime, TParam parameter)
    {
        _dataGetter = dataGetter;
        _parameter = parameter;

        SetTimer(timerInterval, dueTime);
    }

    public TResult Data { get; private set; }

    public DateTime LastSuccess { get; private set; }

    #region IDisposable Members

    public void Dispose()
    {
        //stop timer
        _timer.Change(Timeout.Infinite, Timeout.Infinite);
        _timer.Dispose();
        //cancel task
        _cancellationTokenSource.Cancel();
    }

    #endregion

    public event EventHandler<EventArgs<TResult>> DataChangedHandler;

    public void OnDataChangedHandler(EventArgs<TResult> e)
    {
        //from C# 4.0 in a nutshell:
        ////In multithreaded scenarios (Chapter 21), you need to assign the
        ////delegate to a temporary variable before testing and invoking it
        ////in order to be thread-safe:
        ////var temp = PriceChanged;
        ////if (temp != null) temp (this, e);
        EventHandler<EventArgs<TResult>> handler = DataChangedHandler;
        if (handler != null) handler(this, e);
    }

    private void SetTimer(int timerInterval, int dueTime)
    {
        _timer = new Timer(CheckForNewData, _parameter, dueTime, timerInterval);
    }

    /// <summary>
    /// If the previous task has finished, executes it anew.
    /// </summary>        
    public void CheckForNewData(object param)
    {
        lock (Locker)
        {
            if (_task != null && !_task.IsCompleted && !_task.IsCanceled) return;
            var closure = new Func<ITaskKeeperFuncResult<TResult>>(() => Wrapper(param));
            _cancellationTokenSource = new CancellationTokenSource();
            _task = Task<ITaskKeeperFuncResult<TResult>>.Factory.StartNew(closure, _cancellationTokenSource.Token);
            _task.ContinueWith(RunWhenFuncFinished);
        }
    }

    private ITaskKeeperFuncResult<TResult> Wrapper(object paramObject)
    {
        var taskKeeperFuncParam = new TaskKeeperFuncParam<TParam>(LastSuccess, (TParam)paramObject);
        return _dataGetter(taskKeeperFuncParam);
    }

    private void RunWhenFuncFinished(Task<ITaskKeeperFuncResult<TResult>> task)
    {
        if (!task.IsFaulted)
        {
            ITaskKeeperFuncResult<TResult> result = task.Result;
            if (!result.Success) return;
            lock (Locker2)
            {
                Data = result.Result;
                LastSuccess = DateTime.Now;
            }
            EventArgs<TResult> args = DataChangedHandler.CreateArgs(Data);
            OnDataChangedHandler(args);
        }
        else
        {
            Logger.Current.LogError(task.Exception);
        }
    }
}

EDIT: @almaz asked "Why do you submit last success date to the method that is supposed just to query the data via HTTP?" - The answer is, that it's one of the parameters needed for the HTTP request. It tells the 3rd party repository to return all data "since" the last time we succeeded in retrieving the information.

Answer 1

The main issue here is that you don't expose the actual slow part as a Task. What you currently do is you create a new thread each time you need to send HTTP request instead of just using asynchronous execution that doesn't require new threads (except the one your code runs on).

Other issues worth noting:

• The presence of first .ctor that doesn't actually use TParam means that you need to create a class hierarchy here, with one class having a single TResult generic parameter and derived one with both parameters.

• Interval is better represented with TimeSpan struct, or at least rename timerInterval to timerIntervalInMilliseconds or something like that.

• CancellationTokenSource is usually created just once, but you create it for every call.
• Locking is done quite strange, why do you use static Locker objects? Why do you want to block different instances of the same class from querying (potentially) different data at the same time?
• I don't think you need Locker2 since users of your class won't be able to lock on it, and you don't use values of Data and LastSuccess so that assignment may cause a racing condition
• If you want to check for "good" task status then it's better to look at task.IsCompleted rather than !task.IsFaulted.
• And finally I don't see the reason for those ITaskKeeperFuncParam<T> and ITaskKeeperFuncResult<T>, what is the purpose of those? Why do you submit last success date to the method that is supposed just to query the data via HTTP? If HTTP request is not successful I assume there would be exception, in which case task will be faulted and there is no need in ITaskKeeperFuncResult<T>.Success property.

Full implementation after these changes will look like:

    public class DataChangedEventArgs<TResult> : EventArgs
    {
        public TResult Data { get; set; }
        public DateTime LastSuccess { get; set; }
    }

    public class TaskKeeper<TResult> : IDisposable
    {
        private readonly CancellationTokenSource _cancellationTokenSource = new CancellationTokenSource();
        private readonly Func<CancellationToken, Task<TResult>> _dataGetter;
        private readonly Timer _timer;
        private Task _dataLoadingTask;

        public TaskKeeper(Func<CancellationToken, Task<TResult>> dataGetter, TimeSpan whenToStart, TimeSpan interval)
        {
            _dataGetter = dataGetter;
            _timer = new Timer(CheckForNewData, null, whenToStart, interval);
        }

        public TResult Data { get; private set; }
        public DateTime LastSuccess { get; private set; }

        public event EventHandler<DataChangedEventArgs<TResult>> DataChangedHandler;

        #region IDisposable Members

        public void Dispose()
        {
            _cancellationTokenSource.Cancel();
            _timer.Dispose();
        }

        #endregion

        protected void OnDataChangedHandler(DataChangedEventArgs<TResult> eventArgs)
        {
            var eventHandler = DataChangedHandler;

            if (eventHandler != null)
                eventHandler(this, eventArgs);
        }

        /// <summary>
        /// If the previous task has finished, executes it as new.
        /// </summary>        
        private void CheckForNewData(object _)
        {
            var existingTask = _dataLoadingTask;
            if (existingTask != null)
                existingTask.Wait(); //TODO: or maybe just quit?

            //TODO: Here you may want to introduce double-check locking  to make sure that there won't be 2 tasks running at the same time.
            _dataLoadingTask = _dataGetter(_cancellationTokenSource.Token).ContinueWith(RunWhenFuncFinished);
        }

        private void RunWhenFuncFinished(Task<TResult> task)
        {
            if (task.IsCompleted) //if not - then either there was an exception or task was cancelled
            {
                Data = task.Result;
                LastSuccess = DateTime.Now;
                OnDataChangedHandler(new DataChangedEventArgs<TResult> { Data = Data, LastSuccess = LastSuccess });
            }
            else if (task.IsFaulted)
                Logger.Current.LogError(task.Exception);

            _dataLoadingTask = null;
        }
    }

    //TODO: Not sure if you actually need it :)
    public class TaskKeeper<TParam, TResult> : TaskKeeper<TResult>
    {
        public TaskKeeper(Func<TParam, CancellationToken, Task<TResult>> dataGetter, TimeSpan whenToStart, TimeSpan interval, TParam param)
            : base(ct => dataGetter(param, ct), whenToStart, interval) { }
    }

Answer 2

In the dispose why don't you dispose the timer _timer.Dispose();?

Answer 3

Assign a empty delegate to the event to avoid race condition , much better version

public event EventHandler<EventArgs<TResult>> DataChangedHandler= delegate { };

Rest code looks okay. just wondering why you just not cache the data for a predefined interval rather that doing this.I am not sure which kind of application you are working on.