Is synclock required? If so, How?

[PickyBiker]

Trying to learn about threads and synclock.

The example code below has 3 BackgroundWorker threads. All 3 threads access the same Num property. Since the property's get/set handles all access to the _n variable, is a synclock required to prevent a race condition or does the property take care of that? If a synclock is required, how/where would that be implemented?

Imports System.ComponentModel
Public Class Form1
    Private _n As Long
    Private Property Num As Long
        Get
            Return _n
        End Get
        Set(ByVal value As Long)
            _n = value
        End Set
    End Property

    Private Sub Form1_Load(sender As System.Object, e As System.EventArgs) Handles MyBase.Load
        Label1.Text = ""
        Label3.Text = ""
        Label5.Text = ""
        Button2.Enabled = False
        Button4.Enabled = False
        Button6.Enabled = False
    End Sub

    Private Sub BW1_DoWork(sender As Object, e As DoWorkEventArgs) Handles BW1.DoWork
        While e.Cancel = False
            Num += 1
            BW1.ReportProgress(Num, " Apples")
            Threading.Thread.Sleep(1000)
            If BW1.CancellationPending Then
                e.Cancel = True
            End If
        End While
    End Sub

    Private Sub BW2_DoWork(sender As Object, e As DoWorkEventArgs) Handles BW2.DoWork
        While e.Cancel = False
            Num += 1
            BW2.ReportProgress(Num, " Oranges")
            Threading.Thread.Sleep(1000)
            If BW2.CancellationPending Then
                e.Cancel = True
            End If
        End While
    End Sub

    Private Sub BW3_DoWork(sender As Object, e As DoWorkEventArgs) Handles BW3.DoWork
        While e.Cancel = False
            Num += 1
            BW3.ReportProgress(Num, " Bananas")
            Threading.Thread.Sleep(1000)
            If BW3.CancellationPending Then
                e.Cancel = True
            End If
        End While
    End Sub

    Private Sub BW1_ProgressChanged(sender As Object, e As ProgressChangedEventArgs) Handles BW1.ProgressChanged
        Label1.Text = e.ProgressPercentage.ToString + e.UserState
    End Sub

    Private Sub BW2_ProgressChanged(sender As Object, e As ProgressChangedEventArgs) Handles BW2.ProgressChanged
        Label3.Text = e.ProgressPercentage.ToString + e.UserState
    End Sub

    Private Sub BW3_ProgressChanged(sender As Object, e As ProgressChangedEventArgs) Handles BW3.ProgressChanged
        Label5.Text = e.ProgressPercentage.ToString + e.UserState
    End Sub

    Private Sub BW1_RunWorkerCompleted(sender As Object, e As RunWorkerCompletedEventArgs) Handles BW1.RunWorkerCompleted
        Label1.Text = "BW 1  Canceled"
        Button1.Enabled = True
        Button2.Enabled = False
    End Sub

    Private Sub BW2_RunWorkerCompleted(sender As Object, e As RunWorkerCompletedEventArgs) Handles BW2.RunWorkerCompleted
        Label3.Text = "BW 2  Canceled"
        Button3.Enabled = True
        Button4.Enabled = False
    End Sub

    Private Sub BW3_RunWorkerCompleted(sender As Object, e As RunWorkerCompletedEventArgs) Handles BW3.RunWorkerCompleted
        Label5.Text = "BW 3  Canceled"
        Button7.Enabled = True
        Button6.Enabled = False
    End Sub

    '
    '
    '
    '
    '
    '
    '
    '
    '
    '
    '
    '
    '
    '  BUTTONS Handled below

    Private Sub Button1_Click(sender As System.Object, e As System.EventArgs) Handles Button1.Click
        Button1.Enabled = False
        Button2.Enabled = True
        Num = 0
        BW1.WorkerSupportsCancellation = True
        BW1.WorkerReportsProgress = True
        BW1.RunWorkerAsync()
    End Sub

    Private Sub Button2_Click(sender As System.Object, e As System.EventArgs) Handles Button2.Click
        Button2.Enabled = False
        Button1.Enabled = True
        BW1.CancelAsync()
    End Sub

    Private Sub Button3_Click(sender As System.Object, e As System.EventArgs) Handles Button3.Click
        Button3.Enabled = False
        Button4.Enabled = True
        Num = 0
        BW2.WorkerReportsProgress = True
        BW2.WorkerSupportsCancellation = True
        BW2.RunWorkerAsync()
    End Sub

    Private Sub Button4_Click(sender As Object, e As EventArgs) Handles Button4.Click
        Button4.Enabled = False
        Button3.Enabled = True
        BW2.CancelAsync()
    End Sub

    Private Sub Button5_Click(sender As Object, e As EventArgs) Handles Button5.Click
        Button5.Enabled = False
        Button6.Enabled = True
        Num = 0
        BW3.WorkerReportsProgress = True
        BW3.WorkerSupportsCancellation = True
        BW3.RunWorkerAsync()
    End Sub

    Private Sub Button6_Click(sender As Object, e As EventArgs) Handles Button6.Click
        Button6.Enabled = False
        Button5.Enabled = True
        BW3.CancelAsync()
    End Sub

    Private Sub Button7_Click(sender As Object, e As EventArgs) Handles Button7.Click
        Try
            BW1.CancelAsync()
        Catch
        End Try
        Try
            BW2.CancelAsync()
        Catch
        End Try
        Try
            BW2.CancelAsync()
        Catch
        End Try
        Application.Exit()
    End Sub
End Class

[Shaggy Hiker]

Reads do not require locking, but writes do. Therefore, there would be locking around the Num+=1 statements. It would be a harder decision if you didn't have those sleep statements in there, because without those, there would be no way that the locking could be handled efficiently.

What you can do is create an Object at form scope, and lock on that object. The point is that one thread will lock the object. The other two threads will try to lock the object, see that it is already locked, then wait until the lock is released. So, you can just replace the Num += 1 with:

SyncLock (yourObjectHere)
 Num+=1
End SyncLock

If you didn't have the sleep in there, then the contention for that object would be pretty high. Each thread would be trying to grab the lock as often as it could, since there wouldn't be any measurable delay between releasing the lock and trying to grab it again in the next iteration. That would probably KILL the performance. Since you have the sleep in there, then each thread is spending most of its time sleeping rather than spending most of its time contending for the lock. That does make it a better example.

Without the lock, you could get some interesting results around the Num +=1. That isn't an atomic statement. Num is brought local, then 1 is added to it, then it is written back. So, thread A could read Num, then thread B could read Num, then thread C could read Num, then A would increment its version and write it back, then B could increment its version and write it back, then C could increment its version and write it back, in which case Num would go up by 1. For example, Num could be 5. A would see 5, B would see 5, and C would see 5. A would increase Num to 6 and write it back, but B also read 5, so it would increment it to 6 and write it back, then C would also have seen 5 and increased it to 6 and write it back, so all three threads would have run, yet 5 only went to 6, even though each thread incremented it.

You could also have Num be 7 or 8, and if the contention was great enough, there would also be other alternatives that would be weirder. The numbers could run up to dozens, then drop back down, so you could see the same number three times in a row.

By the way there is also an Interlocked.Increment method that would take care of this particular situation, as it will do the increment as an atomic operation, which means that you can't switch threads in the middle of the increment. The read, add, write would all be one step, and you couldn't switch threads in the middle of it.

[PickyBiker]

Okay I think I've got this now. Thanks Shaggy!

There is not enough going on in the worker threads to operate if the sleep is zero on any one of them. The app locks up tight without some kind of delay, so I set the sleep to 1, 2 & 3 respectively on the worker threads. That should raise the chances for contention considerably.

I then created a global lock object:
Private Lock As New Object

Then for each write access of Num, it now says:

SyncLock Lock
 Num+=1
End SyncLock

If I understood what you said correctly that would seem to cover it.

[Shaggy Hiker]

I wouldn't expect the app to lock up if there is no sleep. It may just be appearing to lock up, when it is actually just waiting to crash. Without sleep, all three threads would be spamming the UI thread with progress reports. That would likely have some impact, and I guess locking things up doesn't seem all that unreasonable as an impact, now that I think about it.

The lock object is kind of like a baton. Only one thread can hold the baton, and it can only enter the block inside the SyncLock if it holds the baton. You can use pretty nearly any object to lock on, so if you have one already, you can use that, though it may be most obvious the way you've done it. I've heard that it can be a bad idea to use the form itself as a lock object, though I don't remember hearing why that would be bad.

[PickyBiker]

Tried it again with sleep(0) in the first worker thread and it locks up as soon as I start that thread. I can't quit the app, can't drag the window, and it never even displays a single digit on label1. If I replace the sleep with a for loop of 60k it works normally. If I lower that to 50k, it freezes again. So my guess is without some kind of work being done or a delay of some type, it is trying to display the label value so fast that it can't complete the first display before it tries again and again and again so it just gets hung up.

I'm not going to treat that as a problem because in a real program there will be some type of activity to consume cycles.

    ' Threading.Thread.Sleep(0)            
    For i = 0 To 60000
    Next

[Shaggy Hiker]

Yeah, one would hope so. Threading doesn't solve all problems. There's a cost to switching contexts...though now that we have multiple cores, that may or may not matter. The context switching meant that the process that was running has to store all of its state, then the other thread loads its state, and starts running. On one core, that has a bigger cost. With multiple cores, each thread might have its own core, so the context switching might happen less frequently, or not at all. However, it's still a cost, even if a smaller one, which means that the advantage of running multiple threads has to be greater than the cost of multiple threads, or else your threaded code will run slower than the non-threaded code. So, threading isn't good in all cases. It's good when the background stuff takes noticeable time, and REALLY good if the background stuff will be pausing for some length of time (waiting for something, such as a remote service call).

[PickyBiker]

I just want to be certain you know I do appreciate the insights you have provided on this. It is like reading something complicated and while you are trying to digest it's meaning, someone comes along as says, "oh, this is what that means and then explains why".

Best regards,

Picky

[jmcilhinney]

Without having read all of this thread and therefore possibly having missed something important, I would recommend that you check out the Interlocked class.

[PickyBiker]

If that is where the Interlocked.Increment is, it's already on my list to check out.

Really appreciate you guys.

[Shaggy Hiker]

Yeah, that's there, and a few others.