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;
 }
}
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Dec 7, 2015 at 12:16
\$\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);
 }
}
answered Dec 7, 2015 at 16:32
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.