What should I SyncLock in this code, and where?

I have a class that has two method in it, one calls a class which creates and executes a number of threads, the other is an event handler that handles an event raised when those threads complete (and then calls the first method again).

I understand that the method that handles the event runs in the thread that raised the event. So as such, I SyncLock a member v开发者_如何转开发ariable that says how many threads are running and subtract one from it:

SyncLock Me ' GetType(me)
    _availableThreads -= 1
End SyncLock

So I have a few questions:

Main Question: Should I be SyncLock'ing _availableThreads everywhere in the class - i.e in the method that creates the threads (which adds 1 when a thread is created)

Side Questions related to this question:

1. I'd usually SyncLock the current instance, but I've seen code that SyncLocks the type instead, so what is the difference between sync locking Me (Current Instance) and GetType(Me)?

2. Would there be a performance difference between the two? and is there anything smaller I'd be able to lock for the above that doesn't affect anything else - perhaps a separate 'padlock' object created for the sole purpose of locking things within a class?

Note: The sole purpose of _availableThreads is to control how many threads can run at any given time and the threads process jobs that can take hours to run.

Code:

Public Class QManager
    Private _maxThreadCount, _availableThreads As Integer

    Public Sub New(ByVal maxThreadCount As Integer)
        Me.MaximumThreadCount = maxThreadCount
    End Sub

    Public Sub WorkThroughQueue()

        //get jobs from queue (priorities change, so call this every time)
        Dim jobQ As Queue(Of QdJobInfo) = QueueDAO.GetJobList


        //loop job queue while there are jobs and we have threads available
        While jobQ.Count > 0 And _availableThreads <= _maxThreadCount

            //create threads for each queued job
            Dim queuedJob As New QdJob(jobQ.Dequeue)
            AddHandler queuedJob.ThreadComplete, AddressOf QueuedJob_ThreadCompleted

            _availableThreads += 1 //use a thread up (do we need a sync lock here?)***************************
            queuedJob.Process() //go process the job

        End While

        //when we get here, don't do anything else - when a job completes it will call this method again
    End Sub

    Private Sub QueuedJob_ThreadCompleted(ByVal sender As QdJobInfo, ByVal args As EventArgs)

        SyncLock Me //GetType(me)
            _availableThreads -= 1
        End SyncLock

        //regardless of how the job ended, we want to carry on going through the rest of the jobs
        WorkThroughQueue()

    End Sub



#Region "Properties"


    Public Property MaximumThreadCount() As Integer
        Get
            Return _maxThreadCount
        End Get
        Set(ByVal value As Integer)
            If value > Environment.ProcessorCount * 2 Then
                _maxThreadCount = value
            Else
                value = Environment.ProcessorCount
            End If
            LogFacade.LogInfo(_logger, "Maximum Thread Count set to " & _maxThreadCount)

        End Set
    End Property

#End Region

End Class

You shouldn't SyncLock the instance or the type. You always want to SyncLock on a variable that is fully within the control of the class, and neither of those are. You should declare a private New Object and use that for your SyncLock.

Private lockObject as New Object()

...

SyncLock lockObject
   ...
End SyncLock

Unfortunately, you need to do a few things differently here.

First off, I'd recommend avoiding SyncLock, and using Interlocked.Increment and Interlocked.Decrement to handle changing _availableThreads. This will provide thread safety for that variable without a lock.

That being said, you still will need a SyncLock around every access to your Queue - if it's being used from multiple threads. An alternative, if you're using .NET 4, would be to change over to using the new ConcurrentQueue(Of T) class instead of Queue. If you use SyncLock, you should create a private object only accessible by your class, and use it for all synchronization.

You should be using the Interlocked class here, the Decrement() method to decrease the count. Yes, everywhere the variable is accessed.

Using SyncLock Me is as bad as SyncLock GetType(Me). You should always use a private object to lock on so nobody can accidentally cause a deadlock. The golden rule is that you cannot lock data, you can only block code from accessing data. Since the code is your private implementation detail, the object that holds the lock state must also be a private detail. Neither your object (Me) nor the Type of that object is private. Allowing other code to lock it by accident.

You can substitute the thread counter with Semaphore. If you use Semaphore you do not need to exit from while loop and neither it is necessary to call WorkThroughQueue() from ThreadCompleted event handler. Semaphore is thread safe so you can use it without locking.

http://www.albahari.com/threading/part2.aspx#_Semaphore