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.

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.

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.

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 ;)
   
   
   
   

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;

    }

}