2

I spin off a new thread that reads data from a database. Every X records a flag is signaled where the main thread will then process the records that have been retrieved keeping some and discarding others. When the flag is signaled I use a lock to allow the reader thread to wait until the processor thread to finish processing the records. However, it appears the lock is not doing this because as I iterate through the records, more keep getting added (indicating the reader thread is still reading). This causes the collection to be modified resulting in an InvalidOperationExecption.

Maybe I have misunderstood what the purpose of "lock" is or I am not using it correctly. Here is pseudo of what I have:

readonly object locker = new object(); Dictionary screened = new Dictionary; Search(){ Thread reader = new Thread( () => Read("search terms") ); reader.Start(); while( found < desiredAmount ){ if(SIGNAL){ lock(locker){ ProcessRecords(); } } } } Read(){ Connect to DB while(reader.Read()){ screened.add(record); } } ProcessRecords(){ foreach(var x in screened){ //process record } }

I hope the pseudo was good enough, from my understanding Read() should not execute while in the lock block. Please help me understand lock a little better.

PS Yes I have read MSDNs articles on locks and still do quite grasp how to use lock in more complex situations.

Improve this question

6 Answers 6

Reset to default
4

You'll need to put a lock around the while loop as well. The lock would work if you have two or more threads contending for the same lock, in your above sample, you have no contention, because no other thread requests a lock apart from the first.

Read(){ Connect to DB while(reader.Read()){ lock(locker) screened.add(record); } }

A better way would be to put the lock inside ProcessRecords() as well.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

'better' is a bit normative there. I like my lock control finegrained to avoid bottlenecks; also some synchronisation objects don't support 'recursive locking' (reentrancy). I believe in .NET only the new ReaderWriterLock has that property
I read this right after I posted the comment on sehes answer. This seems to be working now.
2

screened.Add() is not protected, AFAICT

Try adding the lock like so:

while(reader.Read()){ lock(locker) { screened.add(record); } }
Improve this answer

2 Comments

Ahhh, I understand it now. I added the lock as you said, but also had to add a lock in the ProcessRecords() method right before the loop. Seems to be working now.
You shouldn't be needing that second lock unless ProcessRecords is called from other locations than where you show it in the sample.
1

I am not sure I understand your code... but IF you use a Dictionary to store + read the data then I highly recommend using ConcurrentDictionary - it is thread-safe and really fast (since most operations are implemented lock-free)...

For information see:

Improve this answer

1 Comment

Won't TryAdd() end up not adding records while the ProcessRecords() method is iterating through it?
1

You need to lock on the same object in both threads if you want them to be mutually exclusive.

So, at a high level, you want to repeat the following steps until all records are processed:

  • acquire lock in your reader thread
  • read some records
  • acquire lock in your main thread
  • process the records
Improve this answer

3 Comments

So should I be locking the screened object in the Read method, then when the flag is signaled, "unlock" in that method, then lock in the Process() method? The articles on msdn didn't mention anything like that. Also, I don't understand how I would lock something, leave that block of code to lock somewhere else and then come back to it.
You don't need to explicitly "unlock". That happens automatically when the logic inside your lock construct has been completed. Just make sure the critical sections in each thread are surrounded by the same lock object.
I'm not sure exactly what you want to accomplish, but you may want your reader thread to loop indefinitely, and inside the loop the logic would be to acquire the lock and then read n records from the db (then implicitly release the lock).
1

As it stands with one collection in use, if you keep any accepted entries in it, the processing will re-process them repeatedly.

A better approach would be a producer/consumer queue, possibly using framework 4.0's BlockingCollection<T>.

The general idea of producer/consumer queue is that the database reader would put items into the queue and the processing would remove them from the queue, and if kept, put them in a different collection.

Improve this answer

Comments

0

Normally, a lock is used to protect data from being accessed by multiple threads at the same time.

Each part of the code that needs to access the data must first acquire the lock.

A better way to do what you are trying to do would be to pass messages between your threads.

Let's say both of your threads (reader and searcher) each have acces to the same System.Collections.Concurrent.ConcurentQueue. Once your reader has the required number of rows, it puts a collection of objects or whatever on the queue.

Your searcher thread tries to get items out of the queue using the TryDeque function. When it can't get an item, it sleeps, when it can get an item, it processes it.

Putting a larger group of rows on the queue will probably yield better performances because your threads won't spend a lot of time trying to acquire the lock.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.