Can I lock a collection in a multi thread application

I've inherited a multi threaded application. My multi threading knowledge is pretty woeful I'm afraid.

In a nutshell, the user can start and abort a fairly lengthy process. They can then restart that process, possibly abort it again and so on. The first thing that happens when they start is that a collection of items to process is populated based on selection the user has made. Let's call this thread the processing thread and the collection the toDoList.

There is a separate thread that is responsible for updating the UI with the status of the items in the toDoList. This is triggered by a timer tick and it iterates over toDoList using a For loop. Let's call this thread the display thread

The problem is that it's possible for a user to abort and restart the process while the display thread is part way through iterating over the toDoList. When this happens the processing thread clears and repopulates the toDoList which, in turn, causes the display thread to crash because it's iterating over a collection that's being amended (Collection was modified; enumeration operation may not execute.)

The obvious solution that jump to mind is that I want to somehow lock the collection before it's used in either thread. By way of a metaphor, the UI thread would want a shared (read) lock and the processing thread would want an exclusive (write) lock.

I've been googling and can find all sorts of ways of locking a section of code so that only one thread can enter it at a time but that's no good to me because the code paths are completely separate and unrelated, they just both access the same resource.

Any suggestions?

<sigh> isn't that always the way? I spent a couple of hours going up the wrong paths yesterday before I posted here and this morning my first google search after posting turned up what looks to be the right answer. The locks will always be exclusive but I don't think that'll be a problem.

I haven't implemented and tested it yet but will pop back and resolve the thread if it all works.


Edit>Yep, it all seems to be working. It's hard to say for certain because it's hard to test a timing collision but several runs haven't thrown up the error so I think I'm good. The syntax is slightly different from my link (which was C#) and in VB it's just SynchLock.

I do have one concern that I'd like peoples thoughts on. I originally wanted to read the collection in a shared lock so that multiple reads wouldn't "queue up" and create a snowball effect. I.e. the timer tick that updated the UI is currently set to 4 secs. If the toDoList was really big and it tool more than 4 secs to iterate it, is there a risk that this will create a bottleneck as multiple reads queue up behind each other?

I think that if you describe correctly your problem in the forum or anywhere else or to anyone that's make you take a step back and help you to find more easily the solution.

Maybe have a look at BlockingCollection class?

I would be inclined to make a copy of the list for use in the display thread, but that assumes that iterating over that thread takes some noticeable amount of time. If that condition was met, then the list could be wrapped in a class that allowed controlled access to the list. For example, the class could have methods for Clear, GetCopy, and GetItem. The first two would try to lock on the list (or some other object, but the list should be a fine object to lock on), and would only perform any actions if it got the lock (synclock). The GetCopy would return a deep copy of the list, especially since it sounds like just a List(of String). That would be a quick method, at which point the display thread can go ahead and do what it wants because it is no longer working with the actual list. The Clear method would also need to lock on the object such that it isn't called when the GetCopy is called. GetItem probably doesn't need to be locked for a variety of reasons.

However, that might be what you are already doing.

Thread-Safe Collections

Quote:

---

I would be inclined to make a copy of the list for use in the display thread

---

I thought about that but I wasn't sure whether the overhead of taking the deep copy would be much less than using it directly. The operation the UI actually carries out on it are pretty minimal and testing in my limited development environment wasn't showing any significant slow down. For now I've submitted it for testing with an instruction to performance test it. If blocking looks like being an issue I'm going to try the copy as you suggest.

Quote:

---

Thread-Safe Collections

---

Just having a read now. Thanks for the link.

The issue I have with thread safe collections, and the like, is that I'm not sure that it applies. Thread safety should be about adding, deleting, and so on, from the collection. That's not really the issue here, as I understand it. It seems like the issue is that you do this (or some variation on this):

Code:

---

'If using a thread safe list:
For x = 0 to list.Count 'The count is evaluated ONLY here. That would be thread safe.
 list(x) 'Get one item from the list, which is also thread safe.
Next 'This has nothing to do with the list, and can't be thread safe.

---

The idea is that VB will evaluate the number of iterations right off, which will be N. Accessing elements from the list by the display thread can be done in a totally thread safe manner, but N is still N. If the other thread clears the list, that clearing will be done in a totally thread safe manner, and N will STILL be N, but N needs to be 0 (and the loop exit), because the list.count has now changed. In other words, the list itself is thread safe, but the loop that accesses the list doesn't know anything about that, it just keeps on going until N is reached, even though N is no longer the loop.count.

If that's the case, the obvious way to test would be to greatly exaggerate the loop. Add something like a Sleep(1000) before accessing the list element. That would give the thread a good long time for the list to be cleared before accessing an element that is no longer valid, which should make failures far more likely.

Quote:

---

Originally Posted by Shaggy Hiker

The issue I have with thread safe collections, and the like, is that I'm not sure that it applies. Thread safety should be about adding, deleting, and so on, from the collection. That's not really the issue here, as I understand it. It seems like the issue is that you do this (or some variation on this):

Code:

---

'If using a thread safe list:
For x = 0 to list.Count 'The count is evaluated ONLY here. That would be thread safe.
 list(x) 'Get one item from the list, which is also thread safe.
Next 'This has nothing to do with the list, and can't be thread safe.

---

The idea is that VB will evaluate the number of iterations right off, which will be N. Accessing elements from the list by the display thread can be done in a totally thread safe manner, but N is still N. If the other thread clears the list, that clearing will be done in a totally thread safe manner, and N will STILL be N, but N needs to be 0 (and the loop exit), because the list.count has now changed. In other words, the list itself is thread safe, but the loop that accesses the list doesn't know anything about that, it just keeps on going until N is reached, even though N is no longer the loop.count.

If that's the case, the obvious way to test would be to greatly exaggerate the loop. Add something like a Sleep(1000) before accessing the list element. That would give the thread a good long time for the list to be cleared before accessing an element that is no longer valid, which should make failures far more likely.

---

Also part of the problem is this, "The problem is that it's possible for a user to abort and restart the process while the display thread is part way through iterating over the toDoList. When this happens the processing thread clears and repopulates the toDoList ..."

I don't think that thread safe collections will help with that.

Hmmmmmm.....

This little experiment shows that List fails, but surprisingly BlockingCollection doesn't(didn't during my limited test runs).

Code:

---

Public Class Form1

    Private procTask As task
    Private dispTask As task

    Private Sub Form1_Shown(sender As Object, e As EventArgs) Handles Me.Shown
        LoadToDo()
        procTask = Task.Run(Sub() Proc())
        dispTask = Task.Run(Sub() Disp())
    End Sub

    Private Sub Form1_FormClosing(sender As Object, e As FormClosingEventArgs) Handles Me.FormClosing
        _disp.Set()
        dispTask.Wait()
        _proc.Set()
        procTask.Wait()
    End Sub

#Const Test = False 'True for List, False for BlockingCollection

#If Test Then
    Private ToDo As New List(Of String)
#Else
    Private ToDo As New Concurrent.BlockingCollection(Of String)
#End If

    Private Sub LoadToDo()
        'clear ToDo
#If Test Then
        todo.Clear()
#Else
        If ToDo.Any Then
            For Each s As String In ToDo.GetConsumingEnumerable
                If Not ToDo.Any Then Exit For
            Next
        End If
#End If
        For x As Integer = 1 To prng.Next(9, 21)
            ToDo.Add(x.ToString)
        Next
    End Sub

    Private Shared prng As New Random
    Private _proc As New Threading.ManualResetEvent(False)
    Private Sub Proc()
        Do
            For Each s As String In ToDo
                If prng.Next(1, 31) = 10 Then
                    LoadToDo()
                    Exit For
                End If
            Next
        Loop While Not _proc.WaitOne(100)
    End Sub

    Private _disp As New Threading.ManualResetEvent(False)
    Private Sub Disp()
        Do
            Me.Invoke(Sub()
                          RichTextBox1.Clear()
                          RichTextBox1.Refresh()
                          For Each s As String In ToDo
                              Threading.Thread.Sleep(10)
                              RichTextBox1.AppendText(s)
                              RichTextBox1.AppendText(ControlChars.Cr)
                              If _disp.WaitOne(0) Then Exit For
                          Next
                      End Sub)
        Loop While Not _disp.WaitOne(500)
    End Sub
End Class

---

Yeah, list won't work for the reasons identified. It might make a single addition safe but I need the entire collection to be safe. BlockingCollection is interesting though. Thanks for the suggestion.

These thread-safe collections are what .NET Framework provides. There are other external libraries (some can be found on NuGet) which may provide some features that are closer to your requirements. But anything external is just another dependency to think of in the future: is it supported and bugfixed; will it work with .NET Core/.NET 5+; what about backward compatibility of latest versions (e.g. to use with .NET 4.5 on some old machines)...

You can do it by using language features like locks: SyncLock in VB.NET and lock in C#

There are many features and libraries that help multi-threading apps so developers can use what is most suitable for the specific problem.

Yeah. I've gone with a synchlock for now. I'm little concerned over potential performance implication but it seems to be ok so far. I've told the test department to do some big runs on it to make sure.

I was curious about blocking collections so I expanded my test. While I was at it I added some alternative code to SyncLock. Don't recall why I don't use SyncLock anymore, could have even been a mistake on my part.

The results using JUST using blocking collections is enlightening.

.02¢ My real concern is what are the rules for your Producer-Consumer pattern. I can't recall having that pattern where the producer yanked the rug out from under the consumer, but that is what it sounds like you have.

Code:

---

Public Class Form1

    Private procTask As task
    Private dispTask As task

#Const Test = False 'True to test AutoResetEvent
#If Test Then
    ' instead of SyncLock
    Private lock As New Threading.AutoResetEvent(True)
#End If

    Private ToDo As New Concurrent.BlockingCollection(Of Sample)

    Private Sub LoadToDo()
        'note: not protected
        If ToDo.Any Then
            'clear ToDo
            For Each s As Sample In ToDo.GetConsumingEnumerable
                If Not ToDo.Any Then Exit For
            Next
        End If
        'start and end
        Dim si As Integer
        Dim ei As Integer
        If prng.Next(2) = 0 Then
            si = prng.Next(100, 201)
            ei = si + prng.Next(30, 91)
        Else
            si = prng.Next(99000, 99201)
            ei = si + prng.Next(30, 91)
        End If
        For x As Integer = si To ei
            ToDo.Add(New Sample(x))
        Next
    End Sub

    Const specVal As Integer = -1234567890I
    Private Shared prng As New Random
    Private _proc As New Threading.ManualResetEvent(False)
    Private Sub Proc()
        Do
#If Test Then
            lock.WaitOne() 'get the lock
#End If
            For Each s As Sample In ToDo
#If Test Then
                s.IncVal()
#Else
                If prng.Next(1, 31) = 19 Then
                    s.SetVal(specVal)
                    LoadToDo()
                    Exit For
                Else
                    s.IncVal()
                End If
#End If
            Next
#If Test Then
            LoadToDo()
            lock.Set() 'release the lock
#End If
        Loop While Not _proc.WaitOne(10)
    End Sub

    Private _disp As New Threading.ManualResetEvent(False)
    Private Sub Disp()
        Dim sb As New System.Text.StringBuilder
        Dim cterr As Boolean = False
        Do
            cterr = False
#If Test Then
            lock.WaitOne() 'get lock
#End If
            Dim sc As Integer = ToDo.Count
            Dim lv As Integer = 0
            For Each s As Sample In ToDo
                Dim v As Integer = s.GetVal
                If v = specVal Then
                    sb.Append(ControlChars.Tab)
                    sb.Append("  >>>  ")
                End If
                If lv = 0 Then
                    lv = v - 1
                End If
                sb.Append(v.ToString)
                If v <> lv + 1 Then
                    sb.Append("  <<<<<<<<<<<<<<<<<<")
                End If
                sb.Append(ControlChars.Cr)
                lv = v
            Next
            If sc <> ToDo.Count Then
                sb.Append(">>>>>>>>>>>>>>> COUNT <<<<<<<<<<<<<<<")
                sb.Append(ControlChars.Cr)
                cterr = True
            End If
#If Test Then
            lock.Set() 'release lock
#End If

            Dim t As Task
            t = Task.Run(Sub()
                            Dim ar As IAsyncResult
                            ar = Me.BeginInvoke(Sub()
                                                    RichTextBox1.Clear()
                                                    RichTextBox1.AppendText(sb.ToString)
                                                    sb.Length = 0
                                                    RichTextBox1.Refresh()
                                                End Sub)
                            While Not ar.IsCompleted
                                _disp.WaitOne(2)
                            End While
                        End Sub)
            t.Wait()
            If cterr Then _disp.WaitOne(500)
        Loop While Not _disp.WaitOne(200)
    End Sub

    Private Class Sample
        Private _val As Integer = 0
        Public Sub New(val As Integer)
            Me._val = val
        End Sub

        Public Function GetVal() As Integer
            Return Me._val
        End Function

        Public Sub SetVal(val As Integer)
            Me._val = val
        End Sub

        Public Sub IncVal()
            Me._val += 1
        End Sub
    End Class

    Private Sub Form1_Shown(sender As Object, e As EventArgs) Handles Me.Shown
        LoadToDo()
        procTask = Task.Run(Sub() Proc())
        dispTask = Task.Run(Sub() Disp())
    End Sub

    Private closeTask As Task
    Private Sub Form1_FormClosing(sender As Object,
                                  e As FormClosingEventArgs) Handles Me.FormClosing

        If closeTask Is Nothing Then
            e.Cancel = True
            closeTask = Task.Run(Sub()
                                    _proc.Set()
                                    _disp.Set()
                                    procTask.Wait()
                                    dispTask.Wait()
                                    Me.Invoke(Sub() Me.Close())
                                End Sub)
        End If
    End Sub

End Class

---