Again double-checked locking and C#

Question

Recently I have been refactoring some of my C# code and I found a few double-checked locking practices taking place. I didn't know it was a bad practice back then and I really want to get rid of it.

The problem is that I have a class that should be lazily initialized and frequently accessed by lots of threads. I also do not want to move the initialization to a static initializer, because I am planning to use a weak reference to keep the initialized object from staying too long in the memory. However, if needed, I want to 'revive' the object ensuring this happens in a thread-safe manner.

I was wondering if using a ReaderWriterLockSlim in C# and enter an UpgradeableReadLock before the first check, and then if necessary enter a write lock for the initialization would be an acceptable solution. Here is what I'm having in mind:

public class LazyInitialized
{
    private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();

    private volatile WeakReference _valueReference = new WeakReference(null);
    public MyType Value
    {
        get
        {
            MyType value = _valueReference.Target as MyType;
            _lock.EnterUpgradeableReadLock();
            try
            {
                if (!_valueReference.IsAlive) // needs initializing
                {
                    _lock.EnterWriteLock();
                    try
                    {
                        if (!_valueReference.IsAlive) // check again
                        {
                            // prevent reading the old weak reference
                            Thread.MemoryBarrier(); 
                            _valueReference = new WeakReference(value = InitializeMyType());
                        }
                    }
                    finally
                    {
                        _lock.ExitWriteLock();
                    }
                }
            }
            finally
            {
                _lock.ExitUpgradeableReadLock();
            }
            return value;
        }       
    }

    private MyType InitializeMyType()
    {
        // code not shown    
    }
}

My point is that no other thread should try to initialize the item once again, while many threads should read simultaneously once the value is initialized. The upgradeable read lock should block all readers if the write lock is acquired, therefore while the object is being initialized, the behavior will be similar to having a lock statement where the upgradeable read lock begins. After the initialization the Upgradeable read lock will permit multiple threads therefore the performance hit of waiting each thread will not be present.

I also read an article here saying that volatile causes memory barriers to be automatically inserted before read and after write, so I assume only one manually defined barrier between the read and the write will be enough to ensure that the _valueReference object is correctly read. I will gladly appreciate your advices and criticism for using this approach.

Care to comment why double check locking is a bad practice? I know the pattern in other languages (C++, etc.) is a bad idea...but Microsoft specifically states that the CLR does not have the same problem and double check locking should work fine. — Justin Niessner, Jun 13 '11 at 18:46
@Justin Niessner: sounds like the OP may have read that double-checked locking is borked in Java, but I don't think that's *generally* the case for .NET. I will say, while the code above looks ok to me, I've found double-checked locking doesn't work very well with collections or dictionaries. See http://stackoverflow.com/questions/2624301/how-to-show-that-the-double-checked-lock-pattern-with-dictionarys-trygetvalue-is — Juliet, Jun 13 '11 at 19:20
@Justin: The problem with DCLocking is not that it is broken in C#; rather, it's that (1) people use things that resemble the DCL pattern that *are* broken without realizing it and (2) it is almost never actually justified by real performance numbers; if you're having a lock-contention problem it is probably better to solve the problem by eliminating the contention rather than going with a low-lock solution. — Eric Lippert, Jun 13 '11 at 20:20

Coeffect · Answer 1 · 2011-06-13T19:23:49.117

2

A warning: only a single thread can enter UpgradeableReadLock mode at a time. Check out ReaderWriterLockSlim. So if threads pile up while the first thread enters write mode and creates the object, you'll have a bottle neck until the backup is (hopefully) resolved. I would seriously suggest using a static initializer, it will make your life easier.

EDIT: Depending on how often the object needs to be recreated, I would actually suggest using the Monitor class and its Wait and Pulse methods. If the value needs to be recreated, have the threads Wait on an object and Pulse another object to let a worker thread know that it needs to wake up and create a new object. Once the object has been created, PulseAll will allow all the reader threads to wake up and grab the new value. (in theory)

edited Jun 13 '11 at 19:23

answered Jun 13 '11 at 18:54

Coeffect

8,772
2
27
42

I have read about the ReaderWriterLockSlim UpgradeableReadLock method and I got the impression that unless WriteLock is obtained from within the readlock, it is accessible to more than one thread simultaneously. Actually it makes sense to me, since if the conditions of entering a write lock are not met (or not usually met), no performance hit will be present as compared to a normal lock. – Ivaylo Slavov Jun 13 '11 at 19:02
@Ivalyo: No, it would permit multiple ReaderLocks but your code has none. – H H Jun 13 '11 at 19:03
@Ivaylo: To quote the article, "Because there can be only one thread in upgradeable mode at a time, upgrading to write mode cannot deadlock when recursion is not allowed, which is the default policy." – Coeffect Jun 13 '11 at 19:05
It seems that I have got a wrong impression of how UpgradeableReadLock works. I owe you one. – Ivaylo Slavov Jun 13 '11 at 19:16

H H · Accepted Answer · 2011-06-13T19:30:30.907

2

To emphasize the point @Mannimarco makes: if this is the only access point to the Value, and it looks that way, then your whole ReaderWriterLockSlim setup is no better than a simple Monitor.Enter / Monitor.Leave approach. It is a lot more complicated though.

So I believe the following code is equivalent in function and efficiency:

private WeakReference _valueReference = new WeakReference(null);
private object _locker = new object();

public MyType Value
{    
  get
  {    
    lock(_locker)  // also provides the barriers
    {
        value = _valueReference.Target;

        if (!_valueReference.IsAlive)
        {
            _valueReference = new WeakReference(value = InitializeMyType());
        }
        return value; 
    }
  }    
}

edited Jun 13 '11 at 19:30

answered Jun 13 '11 at 19:02

H H

263,252
30
330
514

Yes, that's right. I happened to misunderstand the UpgradeableReadLock mode as it would allow entering it from multiple threds if no writer lock is acquired. I was wrong and the behavior is exacltly as using a normal lock. If, however i wrap the first check into a read lock and the second check in an upgradeable read lock, as well as a write lock for the initialization, I think I am getting the desired effect. However, looking at the code does not make me proud of it. Thank you and @Mannimarco for helping me resolve my misunderstanding for an important concept. – Ivaylo Slavov Jun 13 '11 at 19:49

Again double-checked locking and C#

2 Answers2

Linked