Need to know if my threading lock does what it is supposed to in .Net?

Question

I have an application that, before is creates a thread it calls the database to pull X amount of records. When the records are retrieved from the database a locked flag is set so those records are not pulled again.

Once a thread has completed it will pull some more records form that database. When I call the database from a thread should I set a lock on that section of code so it is called only by that thread at that time? Here is an exmaple of my code (I commented in the area where I have the lock):

private void CreateThreads() 
{ 
       for(var i = 1; i <= _threadCount; i++) 
    { 
        var adapter = new Dystopia.DataAdapter(); 
        var records = adapter.FindAllWithLocking(_recordsPerThread,_validationId,_validationDateTime); 
        if(records != null && records.Count > 0) 
        { 
            var paramss = new ArrayList { i, records }; 
            ThreadPool.QueueUserWorkItem(ThreadWorker, paramss);
        } 

        this.Update(); 
    } 


} 

private void ThreadWorker(object paramList) 
{ 
    try 
    { 
        var parms = (ArrayList) paramList; 
        var stopThread = false; 
        var threadCount = (int) parms[0]; 
        var records = (List<Candidates>) parms[1]; 
        var runOnce = false; 
        var adapter = new Dystopia.DataAdapter(); 
        var lastCount = records.Count; 
        var runningCount = 0; 
        while (_stopThreads == false) 
        { 


           if (records.Count > 0) 
            { 
                foreach (var record in records) 
                { 
                    var proc = new ProcRecords(); 
                    proc.Validate(ref rec); 


                    adapter.Update(rec);

                    if (_stopThreads) 
                    { 
                        break; 
                    } 
                } 
                //This is where I think I may need to sync the threads.
                //Is this correct?
                lock(this){
                records = adapter.FindAllWithLocking;  
                }             
            } 
        } 
    } 
    catch (Exception ex) 
    { 
        MessageBox.Show(ex.Message); 
    } 

SQL to Pull records:

WITH cte AS ( 
  SELECT TOP (@topCount) *
  FROM Candidates WITH (READPAST) 
  WHERE 
  isLocked = 0 and
  isTested = 0 and  
  validated = 0 
  ) 
UPDATE cte 
  SET 
  isLocked = 1,
  validationID = @validationId,
  validationDateTime = @validationDateTime
  OUTPUT INSERTED.*;

Answer 1

You shouldn't need to lock your threads as the database should be doing this on the request for you.

Answer 2

You shouldn't lock(this) since its really easy for you to create deadlocks you should create a separate lock object. if you search for "lock(this)" you can find numerous articles on why.

Here's an SO question on lock(this)

Answer 3

I see a few issues.

First, you are testing _stopThreads == false, but you have not revealed whether this a volatile read. Read the second of half this answer for a good description of what I am talking about.

Second, the lock is pointless because adapter is a local reference to a non-shared object and records is a local reference which just being replaced. I am assuming that the adapter makes a separate connection to the database, but if it shares an existing connection then some type of synchronization may need to take place since ADO.NET connection objects are not typically thread-safe.

Now, you probably will need locking somewhere to publish the results from the work item. I do not see where the results are being published to the main thread so I cannot offer any guidance here.

By the way, I would avoid showing a message box from a ThreadPool thread. The reason being that this will hang that thread until the message box closes.