1

Trying to understand when it's considered best-practice to lock a static variable or not. Is the static Instance setter thread-safe? If not, should it be and why (what's the consequence of not making it thread-safe)?

class MyClass
{
    private static MyClass _instance;

    private static readonly object _padlock = new object();

    public static MyClass Instance
    {
        get
        {
            if(_instance == null)
            {
                lock(_padlock)
                {
                    if(_instance == null)
                    {
                        _instance = new MyClass();
                    }
                }
            }
            return _instance;
        }
        set => _instance = value;
    }

}
Chris Gessler
  • 22,727
  • 7
  • 57
  • 83
  • 1
    Why do you have a setter? Is it so that you can replace `MyClass` with a fresh one at some point? – ProgrammingLlama Mar 27 '19 at 02:33
  • 1
    Correct. It's going to be used as part of the ambient context pattern where I'm implementing a factory class. I can't see a reason to lock it, but it seems like it's not thread safe either. – Chris Gessler Mar 27 '19 at 02:36
  • 1
    I recommend hiding the setter behind a separate appropriately named method to discourage random assignments, as what may happen.. also no, it’s *not* thread-safe. – user2864740 Mar 27 '19 at 03:02
  • I agree it's not thread-safe, but the assignment being atomic briefly confused me. Thanks for everyone's help. – Chris Gessler Mar 27 '19 at 03:32

2 Answers2

3

This is called Double-checked locking.

However, Double-checked locking requires that the underlying field is volatile1.

In short, the assignment is atomic, yet it will need to be synchronized (full fence, via a lock) across the different cores/CPUs. The reason why is another core concurrently reading the value might get an outdated value cached1.

There are several ways to make the code thread-safe:

  • Avoid double-checked locking, and simply perform everything within the lock statement.
  • Make the field volatile using the volatile keyword.
  • Use the Lazy class, which is guaranteed to be thread-safe.

Note : The completely unguarded setter further adds a complication3..

However, in your case, using double-checked locking will probably work fine with a single check and lock with the volatile field, yet i think your best bet is to just full lock everything and be safe

public static MyClass Instance
{
    get
    {
         lock(_padlock)
         {
             if(_instance == null)
                 _instance = new MyClass();
             return _instance;
         }

    }
    set 
    {
         lock(_padlock)
         {
             _instance = value;
         }
    } 
}

Note : Yes it will incur a performance penalty

Reference

Additional Resources

TheGeneral
  • 79,002
  • 9
  • 103
  • 141
  • Thanks. I'm aware of the double-checked locking and prefer using Lazy instead, but this is a rare case where I need a setter. In the past I've used the volatile keyword, but with int (for example). So you DO recommend locking the setter, making it thread-safe? – Chris Gessler Mar 27 '19 at 03:06
  • @ChrisGessler yes you will need to lock the setter as well. – TheGeneral Mar 27 '19 at 03:09
  • @MichaelRandall - Can you explain the negatives of not locking it? For example, could I end up with corrupt memory? Will it crash the exe or would I simply receive a cached (old) copy of the object? I tried crashing the app with thousands of threads and it couldn't. – Chris Gessler Mar 27 '19 at 03:16
  • @ChrisGessler No the assignments are atomic, worst case is you end up with the wrong reference, you think you are getting the default, but its a different instantiation of default, or you think you have set it, but you end up with the default, or vice versa – TheGeneral Mar 27 '19 at 03:18
  • @MichaelRandall - I agree. Safety first! Thanks for you help! – Chris Gessler Mar 27 '19 at 03:28
  • Nitpicking: `However, Double-checked locking requires that the underlying field is volatile. In short, the assignment is atomic, yet it will need to be synchronized (full fence) across the different cores/CPUs` -> volatile in .net does not emit a full fence. `lock` does though. – Kevin Gosse Mar 27 '19 at 08:04
  • @KevinGosse yeah it was a little loose when i read it back. thanks – TheGeneral Mar 27 '19 at 08:35
1

It seems to me that locks or no locks (on the setter), you will always have an issue of timing. Imagine these scenarios:

  1. You have a lock on the setter but a call to the getter comes in just before the lock is engaged. The caller gets the old instance.
  2. You have a lock on the setter but a call to the getter comes in just after the lock is engaged. The caller waits for the lock to be free, and then gets the new instance.
  3. You don't have a lock on the setter, and the call comes in just before you replace the instance. The caller gets the old instance.
  4. You don't have a lock on the setter, and the call comes in just after you replace the instance. The caller gets the new instance.

With locks and without locks, it's a matter of timing which instance the caller receives.

The only issue I can see is if you want to be able to set Instance to null. If that's the case, your current code will not work because _instance could be changed between the if statement and returning it. You can resolve this by taking a copy of the reference:

public static MyClass Instance
{
    get
    {
        var instanceSafeRef = _instance;
        if(instanceSafeRef == null)
        {
            lock(_padlock)
            {
                if(_instance == null)
                {
                    _instance = new MyClass();
                }
                instanceSafeRef = _instance;
            }
        }
        return instanceSafeRef;
    }
    set => _instance = value;
}
ProgrammingLlama
  • 36,677
  • 7
  • 67
  • 86