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

Deadlocks don't happen when you only have a single resource.

Regarding static properties, no they are not thread safe automatically. It's a common approach to lock on a private static field.

Answer 2

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

Answer 3

(EDIT by jonskeet) This text is just to get the formatting right for the start of the class. No idea why it fails if there's no text before the code.)

Class Better
    Private Shared Regexes() As String = {"test1.Regex", "test2.Regex"}
    Private Shared _CompiledRegExes As List(Of Regex) = BuildList(Regexes)

    Private Shared Function BuildList(ByVal source() As String) As List(Of Regex)
        Dim result = New List(Of Regex)(Regexes.Length - 1)
        For Each exp As String In source
            result.Add(New Regex(exp, RegexOptions.Compiled Or RegexOptions.CultureInvariant Or RegexOptions.IgnoreCase))
        Next
        Return result
    End Function
    Public Shared ReadOnly Property _CompiledRegExes1() As List(Of Regex)
        Get
            Return _CompiledRegExes
        End Get
    End Property

End Class

Class MoreBetter
    Private Shared ReadOnly Regexes() As String
    Private Shared ReadOnly _CompiledRegExes As List(Of Regex)

    Shared Sub New()
        Regexes = New String() {"test1.Regex", "test2.Regex"}
        _CompiledRegExes = 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
    End Sub

    Public Shared ReadOnly Property _CompiledRegExes1() As List(Of Regex)
        Get
            Return _CompiledRegExes
        End Get
    End Property

End Class

EDIT: The reason I like the second one is that it is less error prone. The first one will not work if "Private Shared _CompiledRegExes" is accidentally moved before "Private Shared Regexes()"

Answer 4

VB.Net has a Static keyword you can use to declare variables in members or properties. Variables declared this way retain their state across calls to the method and are guaranteed to be threadsafe (implemented with the Monitor class behind the scenes).

Answer 5

Though I rarely support using this method, it is legitmate.

Class UsingStatic
Private Shared Regexes() As String = {"test1.Regex", "test2.Regex"}
Public Shared ReadOnly Property CompiledRegExes() As List(Of Regex)
    Get
        Static _CompiledRegExes As List(Of Regex) = BuildList(Regexes) <--executes once
        Return _CompiledRegExes <-- executes every time it is called
    End Get
End Property

Private Shared Function BuildList(ByVal source() As String) As List(Of Regex)
    Dim result = New List(Of Regex)(Regexes.Length - 1)
    For Each exp As String In source
        result.Add(New Regex(exp, RegexOptions.Compiled Or RegexOptions.CultureInvariant Or RegexOptions.IgnoreCase))
    Next
    Return result
End Function

End Class

Note that you MUST put the constructor for _CompiledRegExes on the same line. This tiny change will cause you to get a new object each time.

Public Shared ReadOnly Property CompiledRegExes() As List(Of Regex)
    Get
        Static _CompiledRegExes As List(Of Regex) <--executes once
        _CompiledRegExes = BuildList(Regexes) <-- executes every time it is called
        Return _CompiledRegExes
    End Get
End Property

Answer 6

from msdn "The lockobject expression should always evaluate to an object that belongs exclusively to your class. You should declare a Private object variable to protect data belonging to the current instance, or a Private Shared object variable to protect data common to all instances."