Overzealous null checking of backing field in my singleton?

Question

The code below represents a singleton that I use in my application. Lets assume that _MyObject = New Object represents a very expensive database call that I do not want to make more than once under any circumstance. To ensure that this doesn't happen, I first check if the _MyObject backing field is null. If it is, I break into a SyncLock to ensure that only one thread can get in here at a time. However, in the event that two threads get past the first null check before the singleton is instantiated, thread B would end up sitting at the SyncLock while thread A creates the instance. After thread A exits the lock, thread B would enter the lock and recreate the instance which would result in that expensive database call being made. To prevent this, I added an additional null check of the backing field which occurs within the lock. This way, if thread B manages to end up waiting at the lock, it will get through and do one more null check to ensure that it doesn't recreate the instance.

So is it really necessary to do two null checks? Would getting rid of the outer null check and just starting out with the Synclock be just the same? In other words, is thread-locking a variable just as fast as letting multiple threads access a backing field simultaneously? If so, the outer null check is superfluous.

Private Shared synclocker As New Object
Private Shared _MyObject As Object = Nothing
Public Shared ReadOnly Property MyObject As Object
    Get
        If _MyObject Is Nothing Then 'superfluous null check?
            SyncLock synclocker
                If _MyObject Is Nothing Then _MyObject = New Object
            End SyncLock
        End If

        Return _MyObject
    End Get
End Property

Answer 1

You're absolutely right to have added the inner If statement (you would still have a race condition without it, as you correctly noted).

You are also correct that, from a purely-logical point of view, the outer check is superfluous. However, the outer null check avoids the relatively-expensive SyncLock operation.

Consider: if you've already created your singleton, and you happen to hit your property from 10 threads at once, the outer If is what prevents those 10 threads from queueing up to essentially do nothing. Synchronising threads isn't cheap, and so the added If is for performance rather than for functionality.

Answer 2

This will probably be better as an answer rather than a comment.

So, using Lazy to implement "do expensive operation only once, than return reference to the created instance":

Private Shared _MyObject As Lazy(Of Object) = New Lazy(Of Object)(AddressOf InitYourObject)

Private Shared Function InitYourObject() As Object
    Return New Object()
End Function

Public Shared ReadOnly Property MyObject As Object
    Get
        Return _MyObject.Value
    End Get
End Property

This is a very simple and thread-safe way of doing on-demand one time initialization. The InitYourObject method handles whatever initialization you need to do and returns an instance of the created class. On first request, the initialization method is called when you call _MyObject.Value, the subsequent requests will return the same instance.