Thread-safe initialization of static variables

Question

I've been using this pattern to initialize static data in my classes. It looks thread safe to me, but I know how subtle threading problems can be. Here's the code:

public class MyClass // bad code, do not use
{
    static string _myResource = "";
    static volatile bool _init = false;
    public MyClass()
    {
        if (_init == true) return;
        lock (_myResource)
        {
            if (_init == true) return;
            Thread.Sleep(3000); // some operation that takes a long time 
            _myResource = "Hello World";
            _init = true;
        }
    }
    public string MyResource { get { return _myResource; } }
}

Are there any holes here? Maybe there is a simpler way to do this.

UPDATE: Consensus seems to be that a static constructor is the way to go. I came up with the following version using a static constructor.

public class MyClass
{
    static MyClass() // a static constructor
    {
        Thread.Sleep(3000); // some operation that takes a long time 
        _myResource = "Hello World";
    }

    static string _myResource = null;

    public MyClass() { LocalString = "Act locally"; } // an instance constructor

    // use but don't modify
    public bool MyResourceReady { get { return _myResource != null; } }
    public string LocalString { get; set; }
}

I hope this is better.

Answer 1

You can use static constructors to intialize your static variables, which C# guarantees will only be called once within each AppDomain. Not sure if you considered them.

So you can read this: http://msdn.microsoft.com/en-us/library/aa645612(VS.71).aspx (Static Constructors)

And this: http://stackoverflow.com/questions/7095/is-the-c-static-constructor-thread-safe

Answer 2

Depending on your use case (i.e. if threads don't need to pass information to each other using this variable), marking the member variable as [ThreadStatic] may be a solution.
See here.

Answer 3

Performing a lock() on _myResource and changing it inside lock() statement seems like a bad idea. Consider following workflow:

1. thread 1 calls MyClass().
2. execution stops before line _init = true; right after assigning _myResource.
3. processor switches to thread 2.
4. thread 2 calls MyClass(). Since _init is still false and refrence _myResource changed, it succesfully enters lock() statement block.
5. _init is still false, so thread 2 reassigns _myResource.

Workaround: create a static object and lock on this object instead of initialized resource:

private static readonly object _resourceLock = new object();

/*...*/

lock(_resourceLock)
{
    /*...*/
}

Answer 4

Your class is not safe:

1. You change the object you're locking on after you've locked on it.
2. You have a property that gets the resource without locking it.
3. You lock on a primitive type, which is generally not a good practice.

This should do it for you:

public class MyClass
{
    static readonly object _sync = new object();
    static string _myResource = "";
    static volatile bool _init = false;

    public MyClass()
    {
        if (_init == true) return;
        lock (_sync)
        {
            if (_init == true) return;
            Thread.Sleep(3000); // some operation that takes a long time 
            _myResource = "Hello World";
            _init = true;
        }
    }

    public string MyResource 
    { 
        get 
        { 
            MyClass ret; // Correct
            lock(_sync)
            {
                ret = _myResource;
            }
            return ret;
        } 
    }
}

Update:
Correct, the static resource should not be returned directly... I've corrected my example accordingly.

Answer 5

static string _myResource = "";
...
public MyClass()
{
    ...
    lock (_myResource)
    {
    }
}

Due to string interning, you should not lock on a string literal.

UPDATE

Yes, @RaoulRubin, so if you lock on a string literal and that string literal is used by multiple classes then you will be sharing that lock. This can potentially cause unexpected behavior.