6
\$\begingroup\$

I need some advice about my code. The following code will create a new object the first time, and also force other threads to wait for the creation. When the object is created, we return the cached value, and when the value is about to expire, we renew it without blocking other threads, the reference is replaced by the new value.

I'm not sure about the SemaphoreSlim & ManualResetEvet parts, may be somebody can simplify this code.

Thanks.

public class UpdatableLazy<T>
{
    public class Container<T>
    {
        public readonly T Value;

        internal Container(T value)
        {
            Value = value;
        }
    }

    private readonly Func<Task<T>> _updateFunc;
    private readonly Func<T, bool> _isRenewNeeded;
    private volatile Container<T> _container;
    private volatile bool _isUpdating;
    private readonly SemaphoreSlim _ss = new SemaphoreSlim(1, 1);

    public UpdatableLazy(Func<Task<T>> updateFunc, Func<T, bool> isRenewNeeded)
    {
        _updateFunc = updateFunc;
        _isRenewNeeded = isRenewNeeded;
    }

    private readonly ManualResetEventSlim _mre = new ManualResetEventSlim();

    public async Task<T> GetValueAsync()
    {
        //The value doesn't exist yet
        if (_container == null)
        {
            //if a thread is already creating the value, we will wait for the value
            if (!await _ss.WaitAsync(0).ConfigureAwait(false))
            {
                _mre.Wait();
            }

            //After the release of the ManualResetEvent, the value should be available
            if (_container != null)
            {
                return _container.Value;
            }

            try
            {
                //Let's create the value
                _container = new Container<T>(await _updateFunc().ConfigureAwait(false));
                return _container.Value;
            }
            finally
            {
                //We tell awaiting threads that the value is available
                _mre.Set();
                _ss.Release();
            }
        }

        //If the value is not updating and we need to update, we replace the old value by a new one
        if (!_isUpdating && _isRenewNeeded(_container.Value))
        {
            _isUpdating = true;

            try
            {
                _container = new Container<T>(await _updateFunc().ConfigureAwait(false));
            }
            finally
            {
                _isUpdating = false;
            }
        }

        return _container.Value;
    }
}

How I tested it :

class SomeObject
{
    public string Value = "Hello";
    public DateTime Expire = DateTime.UtcNow.AddSeconds(5);
}

class Program
{
    private static int _count;
    static void Main(string[] args)
    {
        var iteration = 10000;
        var cachedObj = new UpdatableLazy<SomeObject>(Create, o => DateTime.UtcNow > o.Expire);
        Parallel.For(0, iteration, new ParallelOptions {MaxDegreeOfParallelism = 2048}, async i =>
        {
            await cachedObj.GetValueAsync().ConfigureAwait(false);
            Interlocked.Increment(ref _count);
        });

        if (iteration != _count)
            throw new Exception("_count and iteration should be equal");

        Console.WriteLine("Done");
        Console.ReadKey();
    }

    static async Task<SomeObject> Create()
    {
        await Task.Delay(1000).ConfigureAwait(false);
        return new SomeObject();
    }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

The initial creation looks thread-safe at first glance. I don't know why you await your semaphore instead of just waiting synchronously with _ss.Wait(0) but I don't think it matters much.

The updating part however is not thread-safe. Any number of threads can come in between:

if (!_isUpdating && _isRenewNeeded(_container.Value))

and

_isUpdating = true;

This will result in every thread getting their own value, which is probably not what you want.

You should also call Dispose on disposable objects when you no longer need them.

\$\endgroup\$
3
  • \$\begingroup\$ I don't want to lock the _isUpdating thing, because I don't want to slow down others queries. With my test sample I didn't manage to update twice at the same time, maybe my test is not stressful enough \$\endgroup\$
    – Bastiflew
    May 4, 2017 at 13:07
  • \$\begingroup\$ @Baptiste, taking a lock is very fast operation. If no other thread contests a lock - there will be no performance hit. You just have to check _isUpdating twice: first without lock, then from inside the lock . If the lock is contested however, then that means that multiple threads came to _isUpdating = true; part of your code, and that is exactly the problem the lock solves. You can also useInterlocked.CompareExchange for synchronization instead of a lock. \$\endgroup\$
    – Nikita B
    May 5, 2017 at 7:53
  • \$\begingroup\$ @Baptiste, I guess my point is, that in my experience if something can happen it will happen sooner or later when it comes to threading. And that's what you should assume when you make a decision on whether or not performance benefits are more important than proper synchronization. Maybe in your case it does not even matter if the instance is the same. Then you should just make sure that _updateFuncis thread-safe, and you are good to go. \$\endgroup\$
    – Nikita B
    May 5, 2017 at 7:59

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.