Thread-safe initialization of static variables

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.

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: Is the C# static constructor thread safe?

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)
{
    /*...*/
}

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.

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.

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

Due to string interning, you should not lock on a string literal. If you lock on a string literal and that string literal is used by multiple classes then you may be sharing that lock. This can potentially cause unexpected behavior.