Static Property and lock Usage

Question

Is this code correct or is there any possibility of some random threading deadlocks etc?

Is it a good idea to use static properties and locking together? Or is static property thread-safe anyway?

Private Shared _CompiledRegExes As List(Of Regex)
Private Shared Regexes() As String = {"test1.Regex", "test2.Regex"}
Private Shared RegExSetupLock As New Object

Private Shared ReadOnly Property CompiledRegExes() As List(Of Regex)
    Get
        If _CompiledRegExes Is Nothing Then
        SyncLock RegExSetupLock

            If _CompiledRegExes Is Nothing Then
                _CompiledRegExes = New List(Of Regex)(Regexes.Length - 1)

                For Each exp As String In Parser.Regexes
                    _CompiledRegExes.Add(New Regex(exp, RegexOptions.Compiled Or RegexOptions.CultureInvariant Or RegexOptions.IgnoreCase))
                Next

            End If

        End SyncLock

    End If

    Return _CompiledRegExes

End Get
End Property

If it's not obvious code is compiling regexes and storing in a List(Of Regex) so they can run faster. And it's shared so every instance of class can get benefit out of it.

Answer 1

Your code isn't thread-safe because you're using double-checked locking without making the field volatile. Unfortunately VB.NET doesn't have a volatile modifier, so you can't apply the normal fix. Just acquire the lock every time instead, or use static initialization to initialize _CompiledRegExes when the type is initialized.

See my singleton page for a general discussion of this with regards to singletons - I know this isn't quite a singleton, but it's close. The page gives C# code, but it should be fairly easy to understand.

Additionally, I generally recommend making lock variables read-only. You really don't want to be changing the value :)

In general terms:

• No, static properties aren't thread-safe automatically
• Yes, it's okay to lock on a static variable (but initialize it explicitly, like you are doing)
• Don't try to write lock-free or low-lock code unless you really know what you're doing. I regard myself as reasonably knowledgeable about threads, and I still don't try using double-checked locking etc.
• Type initialization is thread-safe (with a few caveats if you have complex initializers which end up referencing each other) so that's a good time to do initialization like this - then you really don't need a lock.

EDIT: You don't need to make the type a singleton. Just write function to initialize the list and return it, then use that function in the initializer for the variable:

' This has to be declared *before* _CompiledRegExes '
' as the initializer will execute in textual order '
' Alternatively, just create the array inside BuildRegExes '
' and don't have it as a field at all. Unless you need the array '
' elsewhere, that would be a better idea. '
Private Shared ReadOnly Regexes() As String = {"test1.Regex", "test2.Regex"}

Private Shared ReadOnly _CompiledRegExes As List(Of Regex) = BuildRegExes()

Private Shared ReadOnly Property CompiledRegExes() As List(Of Regex)
    Get
        Return _CompiledRegExes
    End Get
End Property

Private Shared Function BuildRegExes() As List(Of Regex)
    Dim list = New List(Of Regex)(Regexes.Length - 1)

    For Each exp As String In Regexes
        _CompiledRegExes.Add(New Regex(exp, RegexOptions.Compiled Or RegexOptions.CultureInvariant Or RegexOptions.IgnoreCase))
    Next
    Return list
End Function