0
\$\begingroup\$

I need to handle concurrent requests by waiting for the result of an already running operation. Is this code safe or does it contain bugs?

private readonly object _gettingDataTaskLock = new object();
private Task<Data> _gettingDataTask;

public virtual Data GetData()
{
    Task<Data> inProgressDataTask = null;

    lock (_gettingDataTaskLock)
    {
        if (_gettingDataTask == null)
        {
            _gettingDataTask = Task.Factory.StartNew(() => GetDataInternal());
            _gettingDataTask.ContinueWith((task) =>
            {
                lock (_gettingDataTaskLock)
                {
                    _gettingDataTask = null;
                }
             },
             TaskContinuationOptions.ExecuteSynchronously);
         }

         inProgressDataTask = _gettingDataTask;
     }  

     try
     {
         return inProgressDataTask.Result;
     }
     catch (AggregateException ex)
     {
         _logger.ErrorException(ex, "An error occurs during getting full data");
         throw ex.InnerException;
     }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

It is safe, but can be refactored using .NET built-in Lazy<T> class.

There can be several solutions depending on whether results of the GetDataInternal execution can be cached.

Simplest solution is when you can cache the results of the GetDataInternal execution for the lifetime of your parent object. In this case the code can be as simple as (move _lazyData initialization to constructor if GetDataInternal is not static):

private readonly Lazy<Data> _lazyData = new Lazy<Data>(GetDataInternal);

public virtual Data GetData()
{
    return _lazyData.Value; //skipped the logging of the error here as it doesn't add value
}

If you cannot use the cached results (your current logic), then the implementation can look like this:

private Lazy<Data> _lazyData = new Lazy<Data>(GetDataInternal);

public virtual Data GetData()
{
    var lazyData = _lazyData;

    try
    {
        return lazyData.Value;
    }
    finally
    {
        //one of the threads waiting for result will replace the _lazyData with new lazy instance as soon as the Data is calculated or exception is thrown
        Interlocked.CompareExchange(ref _lazyData, new Lazy<Data>(GetDataInternal), lazyData);
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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