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

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 : 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, ITaskKeeperFuncResult> _dataGetter;
private readonly TParam _parameter;
private CancellationTokenSource _cancellationTokenSource;
private Task> _task;
private Timer _timer;

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

public TaskKeeper(Func, ITaskKeeperFuncResult> 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 Disp

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 and ITaskKeeperFuncResult, 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.Success property.

Full implementation after these changes will look like:

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

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

        public TaskKeeper(Func> 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> DataChangedHandler;

        #region IDisposable Members

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

        #endregion

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

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

        /// 
        /// If the previous task has finished, executes it as new.
        ///         
        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 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 { 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 : TaskKeeper
    {
        public TaskKeeper(Func> dataGetter, TimeSpan whenToStart, TimeSpan interval, TParam param)
            : base(ct => dataGetter(param, ct), whenToStart, interval) { }
    }

StackExchange Code Review Q#19015, answer score: 2