Thread safe lazy initializer; is swapping Func<>s a good idea?

Question

The class at the bottom is an implementation of fast, thread safe, "lock free", lazy initializer.

I say "lock free", although it isn't completely; It uses a lock until it is initialized, and then replaces the call to get the data with one that doesn't use the lock. (Similar to a double checked lock with a volatile member; and from performance tests I get v. similar results.)

My question is; is this a good/safe thing to do?

(After I had written this I noticed that the .net 4.0 has a LazyInit<T> which performs the same operations and more, but in a very highly contrived example that I created my implementation was slightly faster :-) )

NB Class has been modified to include Value member, and volatile on the _get

class ThreadSafeInitializer<TOutput>
    where TOutput : class
{
    readonly object _sync = new object();
    TOutput _output;
    private volatile Func<TOutput> _get;

    public TOutput Value { get { return _get(); } }

    public ThreadSafeInitializer(Func<TOutput> create)
    {
        _get = () =>
        {
            lock (_sync)
            {
                if (_output == null)
                {
                    _output = create();
                    _get = () => _output; // replace the Get method, so no longer need to lock
                }
                return _output;
            }
        };
    }
}

Answer 1

Make the Get member volatile, so that you are sure to call the current version. With that modification it's safe as far as I can tell.

Answer 2

As Guffa said, you want to add volatile on Get to make this safe

However I found the implementation a bit tricky to follow. I think its trying to be a bit too fancy for its own good.

So something along these lines is easier to follow:

class Lazy<T> {

    readonly object sync = new object();
    Func<T> init;
    T result;
    volatile bool initialized;

    public Lazy(Func<T> func) {
        this.init = func;
    }

    public T Value {
        get {

            if(initialized) return result;

            lock (sync) {
                if (!initialized) {
                    result = this.init();
                    initialized = true;
                }
            }
            return result;
        }
    }
}

Minor advantages:

• Less fancy
• No performance hit of invoking the Func
• Lock only acquired on initialization.
• Accessing Value via obj.Value is a bit more intuitive than obj.Get()

Answer 3

I write this code a lot:

private SomeThing _myThing = null;
public SomeThing MyThing 
{ get { return _myThing != null ? _myThing : _myThing = GetSomeThing(); } }

Lazy loads just fine and is very readable. My point? the lack of lock(object) is not accidental. Lazy loading properties with locks would be asking for trouble IMO.

Use of the lock statement should be tightly controlled and both Paul and Sam stand hereby accused of calling code they didn't write while holding a lock. Maybe that's no big deal, maybe it deadlocks your program. And your doing this from inside an innocent-looking property get so that you might save a few milliseconds?

My guess is that if you can safely load it twice without catastrophic results that would be better. Then in the rare event that two threads access the same property at the same time then two instances are loaded... Depends on what your loading if that is really a bad thing, I bet most times it doesn't matter that each thread gets a different instance. If it does matter I would recommend performing the needed locks at construction and don't bother with lazy loading. Likely since the constructor can only happen on one thread you won't need a lock at all.