5
\$\begingroup\$

I would appreciate critique and comments in regards of thread-safety on a basic type I'm trying to create.

In C#, the Lazy<T> type is initialized only once, and only when needed the first time.

My type, called Snoozy<T>, is similar, but differs in one important aspect:

The value can be reinitialized when necessary (but only when read).

The purpose is to not perform the value factorization unless someone needs the new value. This could prevent e.g. unnecessary heavy calculations.

Example: A bank account stores a long list of events describing withdrawals and deposits. To get the current balance, all transactions need to be summed up. This could potentially be a very CPU-intensive operation, but the adding of new transactions is rare whereas reading the balance is frequent. When adding a new transaction, there is really no need to recalculate the balance until someone explicitly wants to read it.

This is the code I've come up with so far:

public class Snoozy<T>
{
    private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
    private readonly Func<T> _factory;
    private T _value;
    private bool _isStale;

    public Snoozy(Func<T> valueFactory)
    {
        _factory = valueFactory;
        _isStale = true;
    }

    public T Value
    {
        get
        {
            _lock.EnterUpgradeableReadLock();

            if (_isStale)
            {
                _lock.EnterWriteLock();

                _value = _factory();
                _isStale = false;

                _lock.ExitWriteLock();
            }

            _lock.ExitUpgradeableReadLock();

            return _value;
        }
    }

    // This is how the consumer signals that the underlaying value has changed.
    public void Poke()
    {
        _lock.EnterWriteLock();
        _isStale = true;
        _lock.ExitWriteLock();
    }

    public static implicit operator Snoozy<T>(T value)
    {
        return new Snoozy<T>(() => value);
    }

    public static implicit operator T(Snoozy<T> snoozy)
    {
        return snoozy.Value;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Well, it looks a lot better than this \$\endgroup\$ – Ben Aaronson Aug 25 '16 at 11:10
  • \$\begingroup\$ What is the purpose of implicit operator Snoozy<T>(T value)? Shouldn't it take a Func<T>? \$\endgroup\$ – Stefano d'Antonio Aug 25 '16 at 12:21
  • 1
    \$\begingroup\$ @Uno: You're absolutely right! It should. (: \$\endgroup\$ – Reyhn Aug 29 '16 at 9:06
1
\$\begingroup\$

Why don't you just wrap Lazy<T> and recreate it if necessary?

public sealed class Snoozy<T>
{
    private readonly Func<T> _factory;
    private Lazy<T> _lazy;

    public Snoozy(Func<T> factory)
    {
        _factory = factory;
        _lazy = new Lazy<T>(factory);
    }

    public T Value => _lazy.Value; 

    public void Poke()
    {
        if (_lazy.IsValueCreated)
        {
            _lazy = new Lazy<T>(_factory);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review; good first answer, enjoy your stay! \$\endgroup\$ – ferada Aug 27 '16 at 21:31
  • \$\begingroup\$ Honestly, it didn't occur to me. (: The only downside with this pretty solution is that _lazy is recreated on every Poke, which probably is unnecessary. Also, what happens when thread 1 is writing the volatile _lazy at the same time as thread 2 is constructing _lazy.Value? \$\endgroup\$ – Reyhn Aug 29 '16 at 9:13
  • 1
    \$\begingroup\$ I modified my answer. volatile was not necessary. And I included a check for _lazy to be initialized in Poke method to avoid extra allocation. In the situation you described thread 2 will use the previous value. Yes, it's possible that _factory delegate will be called twice in a short period of time. To avoid it you probably need an asynchronous initialization with cancellation \$\endgroup\$ – just.ru Aug 29 '16 at 16:44
1
\$\begingroup\$

I have some bad news for you... According to MSDN only one thread can enter an upgradeable read lock:

Only one thread can enter upgradeable mode at any given time. If a thread is in upgradeable mode, and there are no threads waiting to enter write mode, any number of other threads can enter read mode, even if there are threads waiting to enter upgradeable mode.

That means your current implementation is basically just exclusive locking.


You should validate your parameters:

public Snoozy(Func<T> valueFactory)
{
    if (valueFactory == null)
    {
        throw new ArgumentNullException(nameof(valueFactory));
    }

and likewise in your conversions.


On the whole, this feels like a cached value to me - there are probably existing constructs which would suit your needs better depending on what framework/type of app you're building. Rolling your own cache is hard. Remember, there are only 2 hard problems in programming: naming, cache invalidation and off by one errors.

\$\endgroup\$
  • \$\begingroup\$ Regarding the bad news; I'm not sure I understand you (or the MSDN doc) correctly. I interpret the quoted text as any thread can still read, until EnterWriteLock is executed, which is the intention. \$\endgroup\$ – Reyhn Aug 29 '16 at 9:05
  • \$\begingroup\$ @Reyhn - yes they can enter a Read lock but not an UpgradeableRead lock. \$\endgroup\$ – RobH Aug 29 '16 at 12:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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