Should I double check before and after locking a list?

Question

I have an in-application service which allows me to feed it messages from various sources, which will be put into a simple list. The service, running in its own thread, will, periodically, process all messages in the list into various files; one file for each source, which are then managed for size.

My question is about the proper way to check for messages and performing a lock around the code which accesses the list. There are only two places which access the list; one is where a message is added to the list and the other is where the messages are dumped from the list into a processing list.

Adding a message to the list:

Public Sub WriteMessage(ByVal messageProvider As IEventLogMessageProvider, ByVal logLevel As EventLogLevel, ByVal message As String)
    SyncLock _SyncLockObject
        _LogMessages.Add(New EventLogMessage(messageProvider, logLevel, Now, message))
    End SyncLock
End Sub

Processing the list:

Dim localList As New List(Of EventLogMessage)
SyncLock _SyncLockObject
    If (_LogMessages.Count > 0) Then
        localList.AddRange(_LogMessages)
        _LogMessages.Clear()
    End If
End SyncLock

' process list into files...

My questions are: should I do a double check when I am processing the list, see below? And why? Or why not? And are there any dangers in accessing the list’s count property outside of the lock? Are either of the methods better or more efficient? And why? Or why not?

Dim localList As New List(Of EventLogMessage)
If (_LogMessages.Count > 0) Then
    SyncLock _SyncLockObject
        If (_LogMessages.Count > 0) Then
            localList.AddRange(_LogMessages)
            _LogMessages.Clear()
        End If
    End SyncLock
End If

' process list into files...

I understand that in this particular case, it may not matter if I do a double check given the fact that, outside of the processing function, the list can only grow. But this is my working example and I’m trying to learn about the finer details of threading.

Thank you in advance for any insights…

---

After some further research, thank you 'the coon', and some experimental programming, I have some further thoughts.

Concerning the ReaderWriterLockSlim, I have the following example which seems to work fine. It allows me to read the number of messages in the list without interfering with other code which may be trying to read the number of messages in the list, or the messages themselves. And when I desire to process the list, I can upgrade my lock to write mode, dump the messages into a processing list and process them outside of any read/write locks, thus not blocking any other threads which may want to add, or read, more messages.

Please note, that this example uses a simpler construct for the message, a String, as opposed to the previous example which used a Type along with some other metadata.

Private _ReadWriteLock As New Threading.ReaderWriterLockSlim()

Private Sub Process()
    ' create local processing list
    Dim processList As New List(Of String)
    Try
        ' enter read lock mode
        _ReadWriteLock.EnterUpgradeableReadLock()
        ' if there are any messages in the 'global' list
        ' then dump them into the local processing list
        If (_Messages.Count > 0) Then
            Try
                ' upgrade to a write lock to prevent others from writing to
                ' the 'global' list while this reads and clears the 'global' list
                _ReadWriteLock.EnterWriteLock()
                processList.AddRange(_Messages)
                _Messages.Clear()
            Finally
                ' alway release the write lock
                _ReadWriteLock.ExitWriteLock()
            End Try
        End If
    Finally
        ' always release the read lock
        _ReadWriteLock.ExitUpgradeableReadLock()
    End Try
    ' if any messages were dumped into the local processing list, process them
    If (processList.Count > 0) Then
        ProcessMessages(processList)
    End If
End Sub

Private Sub AddMessage(ByVal message As String)
    Try
        ' enter write lock mode
        _ReadWriteLock.EnterWriteLock()
        _Messages.Add(message)
    Finally
        ' always release the write lock
        _ReadWriteLock.ExitWriteLock()
    End Try
End Sub

The only problem I see with this technique is that the developer must be diligent about acquiring and releasing the locks. Otherwise, deadlocks will occur.

As to whether this is more efficient than using a SyncLock, I really could not say. For this particular example and its usage, I believe either would suffice. I would not do the double check for the very reasons ‘the coon’ gave about reading the count while someone else is changing it. Given this example, the SyncLock would provide the same functionality. However, in a slightly more complex system, one where multiple sources might read and write to the list, the ReaderWriterLockSlim would be ideal.

---

Concerning the BlockingCollection list, the following example works like the one above.

Private _Messages As New System.Collections.Concurrent.BlockingCollection(Of String)

Private Sub Process()
    ' process each message in the list
    For Each item In _Messages
        ProcessMessage(_Messages.Take())
    Next
End Sub

Private Sub AddMessage(ByVal message As String)
    ' add a message to the 'global' list
    _Messages.Add(message)
End Sub

Simplicity itself…

Answer 1

Theory:

Once a thread acquires the _SyncLockObject lock all other threads reentering that method will have to wait for the lock to be released.

So the check before and after the lock is irrelevant. In other words, it will have no effect. It is also not safe, because you're not using a concurrent list.

If one thread happens to check the Count in the first test while another is clearing or adding to the collection, then you'll get an exception with Collection was modified; enumeration operation may not execute.. Also, the second check can only be executed by one thread at a time (since it's synced).

This applies for your Add method as well. While the lock is owned by one thread (meaning the execution flow has reached that line), no other threads will be able to process or add to the list.

You should be careful to also lock if you are just reading from the list in some other places in your application. For more complex read/write scenarios (such as a custom concurrent collection), I recommend using ReaderWriterLockSlim.

Practice:

Use a BlockingCollection, since it is thread safe (i.e. it handles concurrency internally).