[RESOLVED] SyncLock protecting thread counter

[chris128]

I've got an option in one of my programs to limit the number of threads that get used when the program loops through a list of computers to perform queries against them - I don't want to just use ThreadPool.QueueUserWorkItem because that could spawn too many threads and hammer the CPU (people often use the program to query several hundred remote PCs so I wouldn't want it trying to query all of them at once on separate threads). So I let the user specify how many threads should be allowed to run at once, but there seems to be a race condition that can sometimes cause the task to never finish.

I think this is due to the way I'm limiting the threads but I can't think of a way of fixing this. I'll demonstrate how it works now (I've taken out a lot of irrelevant code for this question, so don't worry if the overall purpose of it doesn't make sense, just focus on the thread counting and locking).

'At class level:
Private _ThreadLimitLock As New Object
Private _ThreadLimitSignal As New System.Threading.AutoResetEvent(False)
Private _CurrentThreadCount As Integer
Private _RemainingCountLock As New Object
Private _RemainingComputers As Integer
Private _AllFinishedSignal As New System.Threading.ManualResetEventSlim(False)
 
 
'In sub that performs the tasks:
For Each Computer In ComputerList
                    System.Threading.Interlocked.Increment(_CurrentThreadCount)
                    If _CurrentThreadCount > ThreadLimit Then
                        _ThreadLimitSignal.WaitOne()
                    End If
                   
                    Dim BgThread As New System.Threading.Thread(AddressOf GetServicesFromSingleComputer)
                    BgThread.IsBackground = True
                    BgThread.Start(Computer)
Next
_AllFinishedSignal.Wait()

So it is looping through each computer and starting a thread for each of them, unless the current thread count is over the user specified limit - in which case it waits on the AutoResetEvent to be signalled. This gets signalled from the method that each background thread is running but the problem is I need to use a lock around the part that signals it... like so:

SyncLock _ThreadLimitLock
           _CurrentThreadCount -= 1
            If _CurrentThreadCount <= ThreadLimit Then
                _ThreadLimitSignal.Set()
            End If
End SyncLock
 
SyncLock _RemainingCountLock
            _RemainingComputers -= 1
            If _RemainingComputers < 1 Then
                _AllFinishedSignal.Set()
            End If
End SyncLock

I need this to all be done under a lock because multiple threads will be running this at the same time so I need them to decrement the count one at a time as they finish their work. Even if I changed this to use Interlocked.Decrement to decrement the counter without a lock, I'd still need to lock on the part that actually checks to see if the current thread count is now below the thread limit and signals the other thread to continue wouldn't I?

The problem as far as I can see, is that in the loop through the computer list we are incrementing the thread counter and checking its value then waiting on the AutoResetEvent but none of this is done under the lock that prevents multiple threads from modifying the thread counter at once. I can't make this be done under that lock though because then it is just going to deadlock and never proceed because one thread will be waiting for the lock so it can signal the AutoResetEvent but it will never get that lock because the thread that has the lock is sat waiting on the WaitOne method in the computer loop.

Any suggestions for how I can sort this out or alternatives to the way I'm doing it? All I want is to be able to control the number of threads I'm creating at any one time and have a way of knowing when they are all complete.

Thanks
Chris

[ident]

I might be thinking stupid but have you thought about using a ConcurrentQueue maybe for the ComputerList. Another idea is reference a check agsint the ThreadPool.GetMaxThreads Method.

[chris128]

I might be thinking stupid but have you thought about using a ConcurrentQueue maybe for the ComputerList. Another idea is reference a check agsint the ThreadPool.GetMaxThreads Method.

If I check the value of GetMaxThreads on my PC it gives me 1023, which is way more threads than I want to be allowed. I don't really want to call ThreadPool.SetMaxThreads and set that to whatever the user specifies, because they might specify something quite low and I believe that the threadpool threads get used internally by the CLR just to run your app and not only when you call QueueUserWorkItem (I know if I use task manager to view the number of threads in just an empty WPF program it shows at least 13 or 14 threads).

I'm not sure how the ConcurrentQueue would help? There's no synchronization issue with actually getting the computer names. If I used a ConcurrentQueue, how would that help with limiting the number of threads that can be running at any one time?

[chris128]

I just realised that Interlocked.Increment actually returns the updated value... every example I've ever seen of it just uses it on its own to update the integer you pass in to it by reference, rather than using its return value. So I could actually change this:

System.Threading.Interlocked.Increment(_CurrentThreadCount)
If _CurrentThreadCount > ThreadLimit Then
                        _ThreadLimitSignal.WaitOne()
End If

To this:

If System.Threading.Interlocked.Increment(_CurrentThreadCount) > ThreadLimit Then
                        _ThreadLimitSignal.WaitOne()
End If

By doing it like this, is there still a chance of another thread changing the value of _CurrentThreadCount in between it being incremented and checking its value against the thread limit?

[SJWhiteley]

By doing it like this, is there still a chance of another thread changing the value of _CurrentThreadCount in between it being incremented and checking its value against the thread limit?

The check will work, but, of course, another check can be called before the WaitOne. The ONLY purpose is to make the increment an atomic operation. Using the result does to become part of the atomic operation.

Why not, at the completion of each thread, flag an event that the thread has completed? Then, in that routine (synclocked), check your thread counter and spawn another thread as appropriate: you'd take 'computers' out of a queue as appropriate.

[ident]

If I check the value of GetMaxThreads on my PC it gives me 1023, which is way more threads than I want to be allowed. I don't really want to call ThreadPool.SetMaxThreads and set that to whatever the user specifies, because they might specify something quite low and I believe that the threadpool threads get used internally by the CLR just to run your app and not only when you call QueueUserWorkItem (I know if I use task manager to view the number of threads in just an empty WPF program it shows at least 13 or 14 threads).

I'm not sure how the ConcurrentQueue would help? There's no synchronization issue with actually getting the computer names. If I used a ConcurrentQueue, how would that help with limiting the number of threads that can be running at any one time?

It wouldn't thinking about it. It was post curry half asleep thinking.

[Shaggy Hiker]

You do have an issue in that code, though I'm not sure that it is THE problem.

In these two lines:

System.Threading.Interlocked.Increment(_CurrentThreadCount)
If _CurrentThreadCount > ThreadLimit Then

you first perform an interlocked increment, then you compare the new value to a different value. Because you wrote that as two statements, you largely lost the advantage of Interlocked.Increment. The method performs something like x +=1, but in a thread-safe manner. However, somebody else could get in there before you compare the result in the If statement. That doesn't seem like it would matter, though. At worst, the current count would be higher than it had been immediately after the interlocked.increment call, which means that the If statement will be false somewhat more often than it really should be. That seems like it should just result in more threads waiting when they probably could have been running, or threads running more out of order than normal.

I got called away to eat soup at that point, so this may now be totally out of order with other posts. That's how multithreading works, though, in a non-preemptive world.

My point was that Interlocked.Increment returns the new value, so you could write:

 If System.Threading.Interlocked.Increment(_CurrentThreadCount)> ThreadLimit Then

[chris128]

You do have an issue in that code, though I'm not sure that it is THE problem.

In these two lines:

System.Threading.Interlocked.Increment(_CurrentThreadCount)
If _CurrentThreadCount > ThreadLimit Then

you first perform an interlocked increment, then you compare the new value to a different value. Because you wrote that as two statements, you largely lost the advantage of Interlocked.Increment. The method performs something like x +=1, but in a thread-safe manner. However, somebody else could get in there before you compare the result in the If statement. That doesn't seem like it would matter, though. At worst, the current count would be higher than it had been immediately after the interlocked.increment call, which means that the If statement will be false somewhat more often than it really should be. That seems like it should just result in more threads waiting when they probably could have been running, or threads running more out of order than normal.

I got called away to eat soup at that point, so this may now be totally out of order with other posts. That's how multithreading works, though, in a non-preemptive world.

My point was that Interlocked.Increment returns the new value, so you could write:

 If System.Threading.Interlocked.Increment(_CurrentThreadCount)> ThreadLimit Then

Thanks and yeah I did actually realise that just before you posted, and changed my code to suit. However, this doesn't seem to have fixed the bug and I also tried changing WaitOne to use the overloaded version that specifies a timeout - I set the timeout to 2 minutes, but apparently the program still hangs on "1 remaining" forever sometimes. What is annoying is that I can't reproduce the issue here, so I'm having to make changes and add extra debug logging etc then send new versions to the customer that is experiencing this issue occasionally and see if they encounter it again.

[chris128]

The check will work, but, of course, another check can be called before the WaitOne. The ONLY purpose is to make the increment an atomic operation. Using the result does to become part of the atomic operation.

Why not, at the completion of each thread, flag an event that the thread has completed? Then, in that routine (synclocked), check your thread counter and spawn another thread as appropriate: you'd take 'computers' out of a queue as appropriate.

I guess that could work, and use the ConcurrentQueue that someone mentioned earlier so there are no issues. That would require something different for the first 10 computers though wouldn't it (assuming 10 threads is our limit), as otherwise there would never be 10 running in the first place if the only time a new thread got started was at the end of an existing thread's work.

So I'd have to loop through all of the computers, ignore the first 10 and add the rest to a concurrent queue, then start threads for each of those first 10. Then when each of them finishes, they decrement the thread counter and then raise an event to say they have finished. In the event handler, if the count is below the user specified limit then it gets the next computer from the queue and starts a new thread to process that one.

I'll give it a go and see if it helps, although after the customer reported that they still have the same issue even when I added a timeout to the WaitOne call, I'm thinking perhaps this is not actually the cause of the hang after all... but I can't see what else could be causing it (the part that decrements the remaining computer count and decides when the process has finished definitely doesn't have any concurrency issues as it is the only thing that uses that counter and that lock, and it is in the Finally block of a Try/Catch/Finally that wraps the entire method that each thread runs).

[dbasnett]

Here is a simulation of what I think you want to do.

Public Class Form1

    Private Sub GetServicesFromSingleComputer(compNM As Object)
        Debug.WriteLine("S " & compNM.ToString)
        For x As Integer = 1 To 10
            Threading.Thread.Sleep(10) 'simulated work
        Next
        thrdCompl.Set() 'THREAD COMPLETE
        Debug.WriteLine("E " & compNM.ToString)
    End Sub

    Const maxThrds As Integer = 5

    Dim thrds As New Concurrent.ConcurrentQueue(Of Threading.Thread)
    Dim thrdStart As New Threading.AutoResetEvent(False)
    Dim thrdCompl As New Threading.AutoResetEvent(False)
    Dim isRun As New Threading.AutoResetEvent(False)

    Dim qmonThrd As Threading.Thread
    Dim qmonL As New Object

    Private Sub threadStarter(butnm As Object)
        Debug.WriteLine("")
        Debug.WriteLine("S " & butnm.ToString)

        Threading.Monitor.Enter(qmonL)
        If qmonThrd Is Nothing Then
            qmonThrd = New Threading.Thread(AddressOf qMonitor)
            qmonThrd.IsBackground = True
            qmonThrd.Start()
        End If
        Threading.Monitor.Exit(qmonL)

        For Each compNm As String In ComputerList

            Dim t As New Threading.Thread(AddressOf GetServicesFromSingleComputer)
            t.IsBackground = True

            thrdStart.WaitOne()

            thrds.Enqueue(t)
            t.Start(butnm.ToString & " " & compNm)
        Next

        Debug.WriteLine("E " & butnm.ToString & "  " & thrds.Count.ToString)
    End Sub

    Private Sub qMonitor()
        Do
            Do While thrds.Count > 0
                'try to remove threads that are completed
                For x As Integer = 0 To thrds.Count - 1
                    Dim t As Threading.Thread
                    If thrds.TryDequeue(t) Then
                        If Not (t.ThreadState = Threading.ThreadState.Stopped OrElse t.ThreadState = Threading.ThreadState.Aborted) Then
                            thrds.Enqueue(t) 'not done put back in Q
                        End If
                    End If
                Next
                If thrds.Count < maxThrds Then
                    thrdStart.Set() 'signal if less than max
                    Threading.Thread.Sleep(10) 'optional to limit how fast threads are started
                Else
                    thrdCompl.WaitOne(250) 'wait for any thread to complete
                End If
            Loop
            thrdStart.Set()
            Threading.Thread.Sleep(100)
        Loop While Not isRun.WaitOne(10)
    End Sub

    Dim ComputerList() As String = New String() {"FIRST", "Lorem", "ipsum", "dolor", "sit", _
                                                 "sunt", "culpa", "qui", "officia", "deserunt", _
                                                 "mollit", "anim", "est", "laborum", "LAST"}

    Private Sub Form1_FormClosing(sender As Object, e As FormClosingEventArgs) Handles Me.FormClosing
        isRun.Set()
        qmonThrd.Join()
    End Sub

    Dim clickCT As Integer = 0
    Private Sub Button1_Click(sender As Object, e As EventArgs) Handles Button1.Click
        clickCT += 1
        Button2.Select()
        Dim t As New Threading.Thread(AddressOf threadStarter)
        t.IsBackground = True
        t.Start(Button1.Name & " > " & clickCT.ToString)
    End Sub

    Private Sub Button2_Click(sender As Object, e As EventArgs) Handles Button2.Click
        clickCT += 1
        Button1.Select()
        Dim t As New Threading.Thread(AddressOf threadStarter)
        t.IsBackground = True
        t.Start(Button2.Name & " > " & clickCT.ToString)
    End Sub
End Class

[chris128]

Quick update on this - it turned out there is no bug at all! I got incorrect information from the end user, and after adding a lot of additional debug logging to the program and getting the logs back from them I was finally able to see that the program genuinely was still waiting for one of the computer queries to complete (turns out there was just a problem on this one remote computer that meant remote queries to it were hanging). They could have discovered this themselves if they had just looked at the information the program was giving them in the GUI, but no they just saw it sat on "1 computer remaining" for more than a couple of minutes and assumed it had hung and didn't actually look at the list of computers to see if any were still showing as processing.

Anyway, I will still look at changing my threading routine to make it a bit more resilient to race conditions, but so far it seems like no one has actually encountered a legitimate bug with the way it works now. I will also see if I can kick off a timer when each thread is started and if the thread hasn't finished after 2 minutes then just kill that thread and report a timeout error back to the user (as the Windows API function the program calls to query the remote computers does not seem to timeout in some cases, which is what was causing this problem).

Thanks for all of your suggestions anyway!

[digitalShaman]

it seems like no one has actually encountered a legitimate bug with the way it works now

there is a minor 'bug' in your code i realized earlier but as it would not manifest in the problem you had seen i did not mention it until now.

When the first of a series of threads finishes and assuming at this point of time the number of running threads has not yet exceeded ThreadLimit because the main loop is still adding new threads, this first ending thread will signal _ThreadLimitSignal. now when later the ThreadLimit is exceeded, _ThreadLimitSignal.WaitOne() will immediatly return and you will have one thread more running than you intended. Nothing too serious i guess...

[Shaggy Hiker]

I think it all evens out. As I noted, there is a slim chance of more threads waiting than would be strictly necessary. So, it's kind of +1 or -1, but either way it shouldn't really matter.