PHP User Warning: fetch_template() calls should be replaced by the vB_Template class. Template name: bbcode_highlight in ..../includes/functions.php on line 4197

PHP User Warning: fetch_template() calls should be replaced by the vB_Template class. Template name: bbcode_highlight in ..../includes/functions.php on line 4197

PHP User Warning: fetch_template() calls should be replaced by the vB_Template class. Template name: bbcode_highlight in ..../includes/functions.php on line 4197

PHP User Warning: fetch_template() calls should be replaced by the vB_Template class. Template name: bbcode_highlight in ..../includes/functions.php on line 4197
VS 2010 [RESOLVED] SyncLock protecting thread counter-VBForums
Results 1 to 13 of 13

Thread: [RESOLVED] SyncLock protecting thread counter

  1. #1

    Thread Starter
    Pro Grammar chris128's Avatar
    Join Date
    Jun 2007
    Location
    England
    Posts
    7,604

    Resolved [RESOLVED] SyncLock protecting thread counter

    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).


    vb.net Code:
    1. 'At class level:
    2. Private _ThreadLimitLock As New Object
    3. Private _ThreadLimitSignal As New System.Threading.AutoResetEvent(False)
    4. Private _CurrentThreadCount As Integer
    5. Private _RemainingCountLock As New Object
    6. Private _RemainingComputers As Integer
    7. Private _AllFinishedSignal As New System.Threading.ManualResetEventSlim(False)
    8.  
    9.  
    10. 'In sub that performs the tasks:
    11. For Each Computer In ComputerList
    12.                     System.Threading.Interlocked.Increment(_CurrentThreadCount)
    13.                     If _CurrentThreadCount > ThreadLimit Then
    14.                         _ThreadLimitSignal.WaitOne()
    15.                     End If
    16.                    
    17.                     Dim BgThread As New System.Threading.Thread(AddressOf GetServicesFromSingleComputer)
    18.                     BgThread.IsBackground = True
    19.                     BgThread.Start(Computer)
    20. Next
    21. _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:

    vb.net Code:
    1. SyncLock _ThreadLimitLock
    2.            _CurrentThreadCount -= 1
    3.             If _CurrentThreadCount <= ThreadLimit Then
    4.                 _ThreadLimitSignal.Set()
    5.             End If
    6. End SyncLock
    7.  
    8. SyncLock _RemainingCountLock
    9.             _RemainingComputers -= 1
    10.             If _RemainingComputers < 1 Then
    11.                 _AllFinishedSignal.Set()
    12.             End If
    13. 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
    Last edited by chris128; Feb 3rd, 2015 at 01:07 PM.
    My free .NET Windows API library (Version 2.2 Released 12/06/2011)

    Blog: cjwdev.wordpress.com
    Web: www.cjwdev.co.uk


  2. #2
    Bad man! ident's Avatar
    Join Date
    Mar 2009
    Location
    Cambridge
    Posts
    5,313

    Re: SyncLock protecting thread counter

    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.

  3. #3

    Thread Starter
    Pro Grammar chris128's Avatar
    Join Date
    Jun 2007
    Location
    England
    Posts
    7,604

    Re: SyncLock protecting thread counter

    Quote Originally Posted by ident View Post
    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?
    My free .NET Windows API library (Version 2.2 Released 12/06/2011)

    Blog: cjwdev.wordpress.com
    Web: www.cjwdev.co.uk


  4. #4

    Thread Starter
    Pro Grammar chris128's Avatar
    Join Date
    Jun 2007
    Location
    England
    Posts
    7,604

    Re: SyncLock protecting thread counter

    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:

    vb.net Code:
    1. System.Threading.Interlocked.Increment(_CurrentThreadCount)
    2. If _CurrentThreadCount > ThreadLimit Then
    3.                         _ThreadLimitSignal.WaitOne()
    4. End If

    To this:

    vb.net Code:
    1. If System.Threading.Interlocked.Increment(_CurrentThreadCount) > ThreadLimit Then
    2.                         _ThreadLimitSignal.WaitOne()
    3. 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?
    My free .NET Windows API library (Version 2.2 Released 12/06/2011)

    Blog: cjwdev.wordpress.com
    Web: www.cjwdev.co.uk


  5. #5
    PowerPoster SJWhiteley's Avatar
    Join Date
    Feb 2009
    Location
    South of the Mason-Dixon Line
    Posts
    2,256

    Re: SyncLock protecting thread counter

    Quote Originally Posted by chris128 View Post

    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.
    "Ok, my response to that is pending a Google search" - Bucky Katt.
    "There are two types of people in the world: Those who can extrapolate from incomplete data sets." - Unk.
    "Before you can 'think outside the box' you need to understand where the box is."

  6. #6
    Bad man! ident's Avatar
    Join Date
    Mar 2009
    Location
    Cambridge
    Posts
    5,313

    Re: SyncLock protecting thread counter

    Quote Originally Posted by chris128 View Post
    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.

  7. #7
    Super Moderator Shaggy Hiker's Avatar
    Join Date
    Aug 2002
    Location
    Idaho
    Posts
    33,929

    Re: SyncLock protecting thread counter

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

    In these two lines:

    Code:
    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:

    Code:
     If System.Threading.Interlocked.Increment(_CurrentThreadCount)> ThreadLimit Then
    My usual boring signature: Nothing

  8. #8

    Thread Starter
    Pro Grammar chris128's Avatar
    Join Date
    Jun 2007
    Location
    England
    Posts
    7,604

    Re: SyncLock protecting thread counter

    Quote Originally Posted by Shaggy Hiker View Post
    You do have an issue in that code, though I'm not sure that it is THE problem.

    In these two lines:

    Code:
    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:

    Code:
     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.
    My free .NET Windows API library (Version 2.2 Released 12/06/2011)

    Blog: cjwdev.wordpress.com
    Web: www.cjwdev.co.uk


  9. #9

    Thread Starter
    Pro Grammar chris128's Avatar
    Join Date
    Jun 2007
    Location
    England
    Posts
    7,604

    Re: SyncLock protecting thread counter

    Quote Originally Posted by SJWhiteley View Post
    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).
    My free .NET Windows API library (Version 2.2 Released 12/06/2011)

    Blog: cjwdev.wordpress.com
    Web: www.cjwdev.co.uk


  10. #10
    Powered By Medtronic dbasnett's Avatar
    Join Date
    Dec 2007
    Location
    Pointless Forest 38.517,-92.023
    Posts
    9,213

    Re: SyncLock protecting thread counter

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

    Code:
    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
    My First Computer -- Documentation Link (RT?M) -- Using the Debugger -- Prime Number Sieve
    Counting Bits -- Subnet Calculator -- UI Guidelines -- >> SerialPort Answer <<

    "Those who use Application.DoEvents have no idea what it does and those who know what it does never use it." John Wein

  11. #11

    Thread Starter
    Pro Grammar chris128's Avatar
    Join Date
    Jun 2007
    Location
    England
    Posts
    7,604

    Re: SyncLock protecting thread counter

    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!
    My free .NET Windows API library (Version 2.2 Released 12/06/2011)

    Blog: cjwdev.wordpress.com
    Web: www.cjwdev.co.uk


  12. #12
    Frenzied Member
    Join Date
    May 2014
    Location
    Central Europe
    Posts
    1,277

    Re: [RESOLVED] SyncLock protecting thread counter

    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...

  13. #13
    Super Moderator Shaggy Hiker's Avatar
    Join Date
    Aug 2002
    Location
    Idaho
    Posts
    33,929

    Re: [RESOLVED] SyncLock protecting thread counter

    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.
    My usual boring signature: Nothing

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  



Featured


Click Here to Expand Forum to Full Width