Is this simple VB.Net class thread safe? If not, how can I improve it?

Question

Option Strict On

Public Class UtilityClass
  Private Shared _MyVar As String

  Public Shared ReadOnly Property MyVar() As String
    Get
      If String.IsNullOrEmpty(_MyVar) Then
        _MyVar = System.Guid.NewGuid.ToString()
      End If
      Return _MyVar
    End Get
  End Property

  Public Shared Sub SaveValue(ByVal newValue As String)
    _MyVar = newValue
  End Sub

End Class

Answer 1

While locking is a good general approach to adding thread safety, in many scenarios involving write-once quasi-immutability, where a field should become immutable as soon as a non-null value is written to it, Threading.Interlocked.CompareExchange may be better. Essentially, that method reads a field and--before anyone else can touch it--writes a new value if and only if the field matches the supplied "compare" value; it returns the value that was read in any case. If two threads simultaneously attempt a CompareExchange, with both threads specifying the field's present value as the "compare" value, one of the operations will update the value and the other will not, and each operation will "know" whether it succeeded.

There are two main usage patterns for CompareExchange. The first is most useful for generating mutable singleton objects, where it's important that everyone see the same instance.

If _thing is Nothing then
    Dim NewThing as New Thingie() ' Or construct it somehow
    Threading.Interlocked.CompareExchange(_thing, NewThing, Nothing)
End If

This pattern is probably what you're after. Note that if a thread enters the above code between the time another thread has done so and the time it has performed the CompareExchange, both threads may end up creating a new Thingie. If that occurs, whichever thread reaches the CompareExchange first will have its new instance stored in _thing, and the other thread will abandon its instance. In this scenario, the threads don't care whether they win or lose; _thing will have a new instance in it, and all threads will see the same instance there. Note also that because there's no memory barrier before the first read, it is theoretically possible that a thread which has examined the value of _thing sometime in the past might continue seeing it as Nothing until something causes it to update its cache, but if that happens the only consequence will be the creation of a useless new instance of Thingie which will then get discarded when the Interlocked.CompareExchange finds that _thing has already been written.

The other main usage pattern is useful for updating references to immutable objects, or--with slight adaptations--updating certain value types like Integer or Long.

Dim NewThing, WasThing As Thingie
Do
    WasThing = _thing
    NewThing = WasThing.WithSomeChange();
Loop While Threading.Interlocked.CompareExchange(_thing, NewThing, WasThing) IsNot WasThing

In this scenario, assuming there is some means by which, given a reference to Thingie, one may cheaply produce a new instance that differs in some desired way, it's possible to perform any such operation on _thing in a thread-safe manner. For example, given a String, one may easily produce a new String which has some characters appended. If one wished to append some text to a string in a thread-safe manner (such that if one thread attempts to add Fred and the other tries to add Joe, the net result would be to either append FredJoe or JoeFred, and not something like FrJoeed), the above code would have each thread read _thing, generate a version with its text appended and, try to update _thing. If some other thread updated _thing in the mean-time, abandon the last string that was constructed, make a new string based upon the updated _thing, and try again.

Note that while this approach isn't necessarily faster than the locking approach, it does offer an advantage: if a thread which acquires a lock gets stuck in an endless loop or otherwise waylaid, all threads will be forever blocked from accessing the locked resource. By contrast, if the WithSomeChanges() method above gets stuck in an endless loop, other users of _thing won't be affected.

Answer 2

With multithreaded code, the relevant question is: Can state be modified from several threads? If so, the code isn’t thread safe.

In your code, that’s the case: there are several places which mutate _MyVar and the code is therefore not thread safe. The best way to make code thread safe is almost always to make it immutable: immutable state is simply thread safe by default. Furthermore, code that doesn’t modify state across threads is simpler and usually more efficient than mutating multi-threaded code.

Unfortunately, it’s impossible to see without context whether (or how) your code could be made immutable from several threads. So we need to resort to locks which is slow, error-prone (see the other answer for how easy it is to get it wrong) and gives a false sense of security.

The following is my attempt to make the code correct with using locks. It should work (but keep in mind the false sense of security):

Public Class UtilityClass
  Private Shared _MyVar As String
  Private Shared ReadOnly _LockObj As New Object()

  Public Shared ReadOnly Property MyVar() As String
    Get
      SyncLock _LockObj
        If String.IsNullOrEmpty(_MyVar) Then
          _MyVar = System.Guid.NewGuid.ToString()
        End If
        Return _MyVar
      End SyncLock
    End Get
  End Property

  Public Shared Sub SaveValue(ByVal newValue As String)
    SyncLock _lockObj
      _MyVar = newValue
    End SyncLock
  End Sub

End Class

A few comments:

• We cannot lock on _MyVar since we change the reference of _MyVar, thus losing our lock. We need a separate dedicated locking object.
• We need to lock each access to the variable, or at the very least every mutating access. Otherwise all the locking is for naught since it can be undone by changing the variable in another place.
• Theoretically we do not need to lock if we only read the value – however, that would require double-checked locking which introduces the opportunity for more errors, so I’ve not done it here.
• Although we don’t necessarily need to lock read accesses (see previous two points), we might still have to introduce a memory barrier somewhere to prevent reordering of read-write access to this property. I do not know when this becomes relevant because the rules are quite complex, and this is another reason I dislike locks.

All in all, it’s much easier to change the code design so that no more than one thread at a time has write access to any given variable, and to restrict all necessary communication between threads to well-defined communication channels via synchronised data structures.