Are C# static class private fields thread safe?

user_7947341 0
Viewed 2627

I have a C# static class accessed from multiple threads. Two questions:

  1. Are my private static fields thread safe when the field is initialized on declaration?
  2. Should I lock when creating private static fields from within static constructor?

Usage of static class from different threads:

class Program
    {
        static void Main(string[] args)
        {
            for (int i = 0; i < 100; i++)
            {
                Task.Run(() =>
                {
                    string name = MyStaticClass.GetValue(9555);
                    //...
                });
            }
        }
}

Option 1 of static class:

public static class MyStaticClass
    {
        private static MyClass _myClass = new MyClass();

        public static string GetValue(int key)
        {
            return _myClass.GetValue(key);
        }
    }

Option 2 of static class:

public static class MyStaticClass
    {
        private static MyClass _myClass;
        private static object _lockObj = new object();

        static MyStaticClass()
        {
            InitMyClass();
        }

        private static void InitMyClass()
        {
            if (_myClass == null)
            {
                lock(_lockObj)
                {
                    if (_myClass == null)
                    {
                        _myClass = new MyClass();
                    }
                }
            }
        }

        public static string GetValue(int key)
        {
            return _myClass.GetValue(key);
        }
    }

Instance class created from the static class:

public class MyClass
    {
        private Dictionary<int, Guid> _valuesDict = new Dictionary<int, Guid>();

        public MyClass()
        {
            for (int i = 0; i < 10000; i++)
            {
                _valuesDict.Add(i, Guid.NewGuid());
            }
        }

        public string GetValue(int key)
        {
            if (_valuesDict.TryGetValue(key, out Guid value))
            {
                return value.ToString();
            }

            return string.Empty;
        }
    }
private static multithreading class c#
4 Answers

Should I lock when initializing private static fields from within static constructor?

Let's not bury the lede here:

Never lock in a static constructor. Static constructors are already locked by the framework so that they run on one thread exactly once.

This is a special case of a more general bit of good advice: never do anything fancy with threads in a static constructor. The fact that static constructors are effectively locked, and that lock can be contested by any code that accesses your type, means that you can very quickly get into deadlocks that you did not expect and are hard to see. I give an example here: https://ericlippert.com/2013/01/31/the-no-lock-deadlock/

If you want lazy initialization, use the Lazy<T> construct; it was written by experts who know how to make it safe.

Are my private static fields thread safe when the field is initialized on declaration?

Thread safety is the preservation of program invariants when program elements are called from multiple threads. You haven't said what your invariants are, so it is impossible to say if your program is "safe".

If the invariant you are worried about is that the static constructor is observed to run before the first static method is executed, or the first instance is created, of a type, C# guarantees that. Of course, if you write crazy code in your static constructor, then crazy things can happen, so again, try to keep your static constructors very simple.

user_886560

fields of static class are not thread safe by default and should avoid unless it is just for read purpose. Here down side is "lock" as well, it will create serialized processing in multi threaded environment.

public static class MyStaticClass
{
    private static MyClass _myClass;
    private static object _lockObj;

    static MyStaticClass()
    {
           _myClass = new MyClass();
           _lockObj = new object();
    }

    public static string GetValue(int key)
    {
        return _myClass.GetValue(key);
    }
    public static void SetValue(int key)
    {
        lock(_lockObj)
        {           
             _myClass.SetValue(key);
        }
    }
}
user_35769710

Your second version is preferable. You can lock it down a little bit more by making your field readonly:

public static class MyStaticClass
{
    private static readonly MyClass _myClass = new MyClass();

    public static string GetValue(int key)
    {
        return _myClass.GetValue(key);
    }
}

Your intent appears to be that _myClass is initially set to an instance of MyClass and never set to another. readonly accomplishes that by specifying that it can only be set once, either in a static constructor or by initializing it as above. Not only can another thread not set it, but any attempt to change it will result in a compiler error.

You could omit readonly and just never set _myClass again, but readonly both communicates and enforces your intent.

Here's where it gets trickier: Your reference to an instance of MyClass is thread safe. You don't have to worry about whether various threads will replace it with a different instance (or set it to null), and it will be instantiated before any threads attempt to interact with it.

What this does not do is make MyClass thread safe. Without knowing what it does or how you interact with it, there's no way for me to say what the needs or concerns are.

If that is a concern, one approach is to use a lock to prevent concurrent access that shouldn't occur, exactly as @Mahi1722 demonstrated. I'm including the code from that answer (not to plagiarize, but if anything happens to that answer then this one will refer to an answer that doesn't exist.)

public static class MyStaticClass
{
    private static MyClass _myClass = new MyClass();
    private static object _lockObj = new object();

    public static string GetValue(int key)
    {
        return _myClass.GetValue(key);
    }

    public static void SetValue(int key)
    {
        lock(_lockObj)
        {           
             _myClass.SetValue(key);
        }
    }
}

Both methods that interact with _myClass lock using _lockObject which means that any execution of either will block while another thread is executing either.

That's a valid approach. Another is to actually make MyClass thread safe, either by using concurrent collections or implementing such locks within that class. That way you don't have to use lock statements in every class that uses an instance of MyClass. You can just use it knowing that it manages that internally.

user_51010460

Both are correct, but there is no need to lock inside static constructor. So, i will choose the first option, it is shorter and clearer

user_48889710
Related