is the current implementation 'safe'?

Absolutely not. The fact that you had to ask this question indicates that you do not understand enough about threading to build your own mechanisms like this. You need to have a deep and thorough understanding of the memory model to build these mechanisms. That is why you should always rely on the mechanisms provided for you in the framework, that were written by experts..

Why is it unsafe? Consider the following scenario. We have two threads, A and B. _Value is null and _Loaded is false.

• We're on thread A.


• The memory location of _Value is loaded into the processor cache for the CPU that thread A is affinitized to. It is null.


• We switch to thread B.


• Thread B reads _Loaded as false, takes the lock, checks _Loaded again, calls create, assigns _Value and _Loaded and leaves the lock.


• We switch back to thread A.


• 
  _Loaded is now true, so thread A returns _Value from the processor cache, which is null.

Thread A is not required to invalidate the cache because thread A never takes a lock.!

Now, I made an argument here from processor caches. This is the wrong argument to make in general. Rather, what you must do when trying to build a new threading mechanism like this is to not reason about any specific processor architecture, but rather to reason about the abstract memory model of the C# language. C# permits reads and writes to move forwards and backwards in time in multithreaded programs. Any time travel that is not explicitly forbidden by the C# specification must be considered to be possible. Your task is to then write code that is correct for any possible combination of movements of reads and writes in time regardless of whether they are really possible on a specific processor or not.

Note that in particular the C# specification does not require that all threads observe a consistent set of write and read re-orderings. It is perfectly legal and possible for two threads to disagree on how a read was re-ordered with respect to a write.

If writing correct programs in a world where all reads and writes can be moved around in time sounds hard, that's because it is. I am not competent to do this work, and I do not attempt to. I leave it to experts.

are there any important performance considerations vs the original one?

Only you can answer that question. Answer performance questions by gathering real-world empirical data.

However, I can say a few things about this problem in general.

The first is: double-checked locking is intended to avoid the cost of the lock. Let's examine the assumptions underlying that intention. The assumption is that the cost of taking the lock is too high on the uncontended path. Is that assumption warranted? What is the cost of taking an uncontended lock? Did you measure it? Did you compare it against the cost of the lock-avoiding check? (Since the lock-avoiding check code is wrong, testing it for performance is not actually meaningful since we can always write faster code if we don't care about correctness, but still, we need to know whether this intervention is an improvement.) And most importantly, is the cost of taking an uncontended lock relevant to the consumer of this code? Because they are the stakeholder whose opinions are relevant; what do they say about the cost of an uncontended lock?

Let's suppose that the cost of an uncontended lock is relevant. Then surely the cost of a contended lock is enormously relevant. You've built a mechanism that potentially contends a lot of threads! What are the alternatives that you considered here? For example, you could avoid the lock altogether by deciding that it is OK for the create function to be called on multiple threads -- perhaps we know that it is cheap and idempotent. Those threads can then race to their heart's content to initialize the field, and we can use an interlocked exchange to ensure that we get a consistent value. That avoids the cost of the lock altogether, but it creates a different kind of cost, and puts a requirement on the caller to pass an idempotent creator.

Let's consider other aspects of your solution with respect to performance. You allocate the lock object regardless of whether you ever take the lock, and you keep it forever. What's the burden on the garbage collector? What is the impact on collection pressure? These things are all deeply relevant to performance. Again, remember, the assumption here is that we are so worried about the couple of nanoseconds it takes to enter and leave an uncontended lock that we're willing to write a double checked lock. If those nanoseconds are relevant then surely the milliseconds it takes to do an extra collection are incredibly relevant!

is there anything else I'm missing that I should be concerned about in a high traffic application?

I don't know how to answer that question.

is there anything else I'm missing that I should be concerned about in a high traffic application?

Yes, your Lazy<T>.Value isn't generic anymore but an object and if Func<T> returns a value type then a lot of un/boxing will take place. This might hurt performance.

I think a LazyFactory.GetOrCreate<T>(...) would do a better job.

Meaning that I have to put half of my logic concerning the lazy property in the constructor, and having more boilerplate code.

This is a little speculative, but I think you have an XY problem. You're trying to reduce boilerplate, but there are probably better ways to do that than what you've suggested.

If I understand correctly, your problem is that your classes look something like this:

public class MyClass

{

    private Lazy<string> _MyStringValue;

    // ...



    public MyClass()

    {

        this._MyStringValue = new Lazy<string>(() => {

            var builder = new StringBuilder();

            builder.Append("a");

            // 50 more lines of expensive construction

            return builder.ToString();

        });



        // 100 more lines constructing OTHER lazy stuff

    }

}

Gloss over the details of building up the value; it's just an example. The important point is that you have all this logic here deep in your constructor.

I think there are two things you can do to alleviate this problem:

1. 
   

   Parameterize

   
   
   

   Why put all this logic in the constructor? You're losing a lot of reusablity by doing that anyway. So make these things parameters and construct them elsewhere:

   
   
   

   public class MyClass
   
   {
   
       private Lazy<string> _MyStringValue;
   
       // ...
   
   
   
       public MyClass(Lazy<string> myStringValue)
   
       {
   
           this._MyStringValue = myStringValue;
   
       }
   
   }
   
   

   
   


2. 
   

   You can embed this construction logic in a method, and then pass the method to the Lazy constructor:

   
   
   

   class MyStringValueMaker
   
   {
   
       // Could be an instance method if that's more appropriate.
   
       // This is just for example
   
       public static string MakeValue()
   
       {
   
           var builder = new StringBuilder();
   
           builder.Append("a");
   
           // 50 more lines of expensive construction
   
           return builder.ToString();
   
       }
   
   }
   
   

   
   
   

   And then elsewhere:

   
   
   

   var myClass = new MyClass(new Lazy<string>(MyStringValueMaker.MakeValue));
   
   

   
   

Now suddenly everything is much better organized, more reusable, and simpler to understand.

If that's not what your class originally looked like, well, then I think you'd be better off posting a new question asking for a review on the original class to get ideas about how to improve.

I like the idea, but you should carefully explain how this works in comments.

Try this:

  MyLazy myLazy = new MyLazy();



  int value1 = myLazy.Get(() => 42);

  Console.WriteLine(value1);



  int value2 = myLazy.Get(() => 65);

  Console.WriteLine(value2);

It correctly prints out:

42

42

But even that we know the answer to everything is 42, it isn't that intuitive. The problem is obviously that you have to - or can - provide a creator function per call to Get<T>(Func<T> creator) and that it is arbitrary, but only the first actually has any effect.

Why not wrap a Lazy<T> and then lazy load the Lazy<T> in your Get

public class MyLazy {

    private object lazy;

    private object _Lock = new object();



    public T Get<T>(Func<T> factory) {

        if (lazy == null) {

            lock (_Lock) {

                if (lazy == null) {

                    lazy = new Lazy<T>(factory);

                }

            }

        }

        return ((Lazy<T>)lazy).Value;

    }

}

Taking advantage of existing features that have been tried and tested instead of trying to roll your own.

is there anything else I'm missing that I should be concerned about in a high traffic application?

By passing the delegate in the Get method, you're instantiating a delegate object each time you call the property. System.Lazy<T> creates the delegate instance only once.

is the current implementation 'safe'?

No it isn't, because:

1. You did not implement Double-checked locking correctly - you have two fields (_Value and _Loaded) instead of only one.


2. You have added new feature - Invalidate - that invalides the correctness of double-checked locking even if you fix previous problem (by e.g. boxing the value).

Lessons to learn

1. Always prefer well-known implementations (e.g. System.Lazy<T> or System.Threading.LazyInitializer) over your own - thread/process synchronization and cryptography are two heaviest topics to master, do not expect that you will be able to design these things yourself in a day, it may take years to master!


2. Benchmark/profile before you optimize - lock is often good enough and you can try e.g. System.Threading.SemaphoreSlim or ReaderWriterLockSlim to speed it up a bit, but beware that it could get even worse - so again: test and measure first, be clever if you need to, be lazy if you can.


3. If you still wish to write your own version then at least remember:
   
   
   
   
   1. Reads and writes can be reordered in any way (unless they depend on each other like e.g. in assignment a = b + c - the order of fetching b and c is not guaranteed, but write to a has to be done after the computation). Be extra cautious when synchronization involves more than one variable! You will likely think that it works because you do things in some order, but that is wrong! The order is not guaranteed across threads!
   
   
   2. 
      volatile only guarantess the order of writes, not that other threads would see them immediately. EDIT: As Voo pointed out, the documentation there (from Microsoft!) apears to be incomplete. Language specification (10.5.3) states that volatile reads have acquire semantics and volatile writes have release semantics. Personal note: how is one supposed to get this right, if you cannot even trust the documentation?! But I found Volatile.Read which gives very strong guarantees (in documentation): Returns: The value that was read. This value is the latest written by any processor in the computer, regardless of the number of processors or the state of processor cache. System.Threading.Interlocked have some good methods too.
   
   
   3. I am not an expert ;)
   
   
   
   

EDIT: Seeing your third attempt I will try to give you my version, but I repeat: I am not an expert ;)

public class MyLazy<T>

{

    private class Box

    {

        public T Value { get; }

        public Box(T value) => Value = value;

    }



    private Box box;

    private Func<T> factory;

    private readonly object guard = new object();



    public MyLazy(Func<T> factory) => this.factory = factory;



    public T Value

    {

        get

        {

            var box = Volatile.Read(ref this.box);

            return box != null ? box.Value : GetOrCreate();

        }

    }



    private T GetOrCreate()

    {

        lock (guard)

        {

            var box = Volatile.Read(ref this.box);

            if (box != null) return box.Value;

            box = new Box(factory());

            Volatile.Write(ref this.box, box);

            return box.Value;

        }

    }



    public void Invalidate() => Volatile.Write(ref this.box, null);

    public void Invalidate(Func<T> factory) // bonus

    {

        lock (guard)

        {

            this.factory = factory;

            Volatile.Write(ref this.box, null);

        }

    }

}

With the feedback that (my brain) could understand I've came to this for now.

• (I think) I literally copied the locking structure of Lazy, thread-safe Singleton
  


• Included adding the volatile keyword for the _Loaded check


• Moved the generic definition to the class type. Adding a bit more boilerplate code on the advantage of more type safety and no-boxing


• Added a warning to remind myself there might be issues

As for the advice "Leave it to the smarter people". That's something I can't work with. I like to learn, I like other people to learn and I prefer a society where people are motivated to fail (against calculated cost) to learn for themselves.

I appreciate that everyone has a different opinion about that, that's okay.

I still not 100% sure if this solves at least the thread-safety problems of the first version, because the conversation went a bit off-topic imo. If anyone that is knowledgable can comment on that I would appreciate it. For the rest; I'm going to try to use this code and see what it does in production and if it causes (practical) problems for my caching of properties.

/// <summary>

/// Warning: might not be as performant (and safe?) as the Lazy<T>, see: 

/// https://codereview.stackexchange.com/questions/207708/own-implementation-of-lazyt-object

/// </summary>

public class MyLazy<T>

{

    private T               _Value;

    private volatile bool   _Loaded;

    private object          _Lock = new object();





    public T Get(Func<T> create)

    {

        if ( !_Loaded )

        {

            lock (_Lock)

            {

                if ( !_Loaded ) // double checked lock

                {

                    _Value   = create();

                    _Loaded = true;

                }

            }

        }



        return _Value;

    } 





    public void Invalidate()

    {

        lock ( _Lock )

            _Loaded = false;

    }

}

I thought when I asked this question it was going to be about the passing of the delegate, but it is the double checked lock that seems to quite difficult to grasp.

Not stopping my curiousity I took the .NET source of Lazy and stripped out the following that do not suit my needs anyway in this case:

• Exception caching


• all the other modes then ExecutionAndPublication


• comments


• Contract.Assert's


• any other things like Debugger visualization, ToString() etc


• Inlined some functions that stopped having any logic because of stripping the other things

This is the result until now of the stripped Lazy. Be aware that I tried to do my best in not changing anything that might change behaviour, and hopefully didn't. If anyone else has updates that would be more then welcome.

Note that I did this for study and learning and not to put it in production.

Some questions that I still have to further simplify:

• 
  can in this case the try/finally and Monitor.Enter/Exit and the lockTaken be simplified to a lock {} statement? If I follow Eric's answer here that would be a yes, but maybe I'm overlooking something: https://stackoverflow.com/a/2837224/647845 Probably not, because the if statement.


• can the sentinel be replaced by a boolean and a null assignment for the factory?


• when you assume the logic is good, can the whole CreateValue() be replaced by an inline new Boxed(factory())?

-

#if !FEATURE_CORECLR

[HostProtection(Synchronization = true, ExternalThreading = true)]

#endif

public class Lazy2<T>

{

    class Boxed

    {

        internal Boxed(T value)

        {

            m_value = value;

        }

        internal T m_value;

    }





    static readonly Func<T> ALREADY_INVOKED_SENTINEL = delegate 

    {

        return default(T);

    };



    private Boxed m_boxed;

    private Func<T> m_valueFactory;

    private object m_threadSafeObj;





    public Lazy2(Func<T> valueFactory)

    {

        m_threadSafeObj = new object();

        m_valueFactory = valueFactory;

    }





    public T Value

    {

        get

        {

            Boxed boxed = null;

            if (m_boxed != null )

            {

                boxed = m_boxed;

                if (boxed != null)

                {

                    return boxed.m_value;

                }

            }



            return LazyInitValue();

        }

    }



    private T LazyInitValue()

    {

        Boxed boxed = null;



        object threadSafeObj = Volatile.Read(ref m_threadSafeObj);

        bool lockTaken = false;



        try

        {

            if (threadSafeObj != (object)ALREADY_INVOKED_SENTINEL)

                Monitor.Enter(threadSafeObj, ref lockTaken);



            if (m_boxed == null)

            {

                boxed = CreateValue();

                m_boxed = boxed;

                Volatile.Write(ref m_threadSafeObj, ALREADY_INVOKED_SENTINEL);

            }

            boxed = m_boxed;

        }

        finally

        {

            if (lockTaken)

                Monitor.Exit(threadSafeObj);

        }



        return boxed.m_value;

    }



    private Boxed CreateValue()

    {

        Boxed boxed = null;



        Func<T> factory = m_valueFactory;

        m_valueFactory = ALREADY_INVOKED_SENTINEL;



        boxed = new Boxed(factory());



        return boxed;

    }

}

I've done the same analysis for .NET Core version of Lazy and stripped out the following that do not suit my needs anyway in this case (the new Lazy(Func<T>) constructor)

• Exception caching


• all the other modes then ExecutionAndPublication


• comments


• Contract.Assert's


• any other things like Debugger visualization, ToString() etc


• Inlined some functions that stopped having any logic because of stripping the other things


• I've reversed the order of methods to make it read more chronological

Questions/notes that I have:

• 
  would anything change if _state will be changed from an object to a boolean in this stripped version? (in the original version it is used for alternative paths, like different publication modes or a cached exception) never mind - the state object is also being used as a lock. I expect that we can change it for object though? saving the seperate class.


• is it allowed to inline some other methods, like ViaFactory?


• it feels to me a bit strange that Value being called recursively the first time but that might have to do with other code that I stripped - i.e. the Exception caching.

..

internal class LazyHelper

{

    internal LazyHelper()

    {

    }

}





public class Lazy<T>

{

    private volatile LazyHelper _state;

    private Func<T> _factory;

    private T _value;





    public Lazy(Func<T> valueFactory)

    {

        _factory = valueFactory;

        _state = new LazyHelper();

    }



    public T Value => _state == null ? _value : CreateValue();





    private T CreateValue()

    {

        var state = _state;

        if (state != null) 

        {

            ExecutionAndPublication(state);

        }

        return Value;

    }





    private void ExecutionAndPublication(LazyHelper executionAndPublication)

    {

        lock (executionAndPublication)

        {

            ViaFactory();

        }

    }





    private void ViaFactory()

    {

        _value = _factory();

        _state = null; // volatile write, must occur after setting _value

    }

}

Not sure if it is allowed, but if I even inline more you really get a 'normal' double checked lock with a volatile. The main difference is that the locker object is also being used as a _loaded flag.

public class Lazy<T>

{

    private volatile object _state;

    private Func<T> _factory;

    private T _value;





    public Lazy(Func<T> valueFactory)

    {

        _factory = valueFactory;

        _state = new object();

    }



    public T Value => _state == null ? _value : CreateValue();





    private T CreateValue()

    {

        var state = _state;

        if (state != null) 

        {

            lock (state)

            {

                if (ReferenceEquals(_state, state)) // seems to act as the second check

                {

                    _value = _factory();

                    _state = null;

                }

            }

        }

        return Value;

    }

}