C# Quartz Race Condition

Question

I am automating some tasks on my website, but I'm currently stuck.

    public void Execute(JobExecutionContext context)
    {
            var linqFindAccount = from Account in MainAccounts
                                  where Account.Done == false
                                  select Account;

            foreach (var acc in linqFindAccount)
            {
                acc.Done = true;
                // stuff
            }
     }

The issue is that when I start multiple threads the first threads get assigned to the same first account because they set the Done value to true at the same time. How am I supposed to avoid this?

EDIT:

    private  object locker = new object();

    public void Execute(JobExecutionContext context)
    {
        lock (locker)
        {
            var linqFindAccount = from Account in MainAccounts
                                  where Account.Done == false
                                  select Account;

            foreach (var acc in linqFindAccount)
            {
                Console.WriteLine(context.JobDetail.Name + " assigned to " + acc.Mail);
                acc.Done = true;
                // stuff
            }

         }
     }


 Instance [ 2 ] assigned to firstmail@hotmail.com
 Instance [ 1 ] assigned to firstmail@hotmail.com

First two threads got assigned to the first account, even though the list contains 30 accounts.

Thanks.

Could you give more details of what Execute() method shoydl exactly acheieve? Just update Done property of found accounts? Why you need multiple threads for this? — sll, Aug 13 '11 at 12:22
you are going to nee some sort of lock object. See (probably better links but this is the first one I found) http://stackoverflow.com/questions/779533/thread-safety-object-static-or-not — Eddy, Aug 13 '11 at 12:24
@sllev I update the done property of the finished accounts, the issue is that I start up to ~20 threads at the same time, and the first ~2-3 threads get assigned to the same first account, because they get started at the same time and the done value is set to false at the time, and I need multiple threads because it's faster. — mtnoob, Aug 13 '11 at 12:27
Multiple threads to enumerate a list and set property are faster? What exactly faster? I would suggest stick with single thread — sll, Aug 13 '11 at 12:48
@sllev Obviously you have missed the // stuff comment. I do much more, thus multiple threads are faster because multiple accounts are done at the same time. — mtnoob, Aug 13 '11 at 12:56
Only one way is to wrap body of the loop in lock(0 but in this way only one thread able to procees an account with updates — sll, Aug 13 '11 at 12:58
@sllev I have tried that, but didn't seem to work either. Check my edit out, thanks! — mtnoob, Aug 13 '11 at 13:07
You should probably reconsider this design. Are the accounts stored in the db? You have to realize that the foreach loop is only one hit to the db. Thus as soon as one loop has started, it doesn't help that another thread has changed it to done as other threads won't be notified. You should look into `Queue` instead — Oskar Kjellin, Aug 13 '11 at 14:41
@Oskar Kjellin Not in a db, just in a simple text file. I load them on startup and then start the threads. — mtnoob, Aug 13 '11 at 15:25
Okay, but the rest of my post still applies. The other threads won't know that you've set an account as done. Look into Queue — Oskar Kjellin, Aug 13 '11 at 15:35
@Oskar Kjellin It seems that I've got it working using a Queue. Thank you very much. — mtnoob, Aug 13 '11 at 15:53
@mtnoob great :) Posted it as an answer as well so we don't have answered questions laying around as unanswered — Oskar Kjellin, Aug 13 '11 at 20:50

score 2 · Answer 1 · answered Aug 13 '11 at 13:36

2

Use

private static readonly object locker = new object();

instead of

private  object locker = new object();

answered Aug 13 '11 at 13:36

Peter PAD

2,252
1
17
20

This link might be useful http://stackoverflow.com/questions/5053172/why-does-the-lock-object-has-to-be-static – Oskar Kjellin Aug 13 '11 at 13:38
We don't need multitheading in this situation – Peter PAD Aug 23 '11 at 05:20

score 0 · Answer 2 · answered Oct 15 '13 at 06:04

 foreach (var acc in linqFindAccount)
        {
            string mailComponent = acc.Mail;
            Console.WriteLine(context.JobDetail.Name + " assigned to " + mailComponent);
            acc.Done = true;
            // stuff
        }

Try above.

score 0 · Answer 3 · answered Aug 13 '11 at 20:49

Your problem is that the deferred execution happens when you start the foreach loop. So the result is cached and not reevaluated every loop. So every thread will work with it's own list of the items. So when an Account is set to done, the other list still remain with the object in it.

A Queue is more suitable in this case. Just put the items in a shared Queue and let the loops take items of the queue and let them finish when the Queue is empty.

Dmitry · Answer 4 · 2011-08-13T22:23:45.847

Few problems with your code:

1) Assuming you use stateless Quartz jobs, your lock does not do any good. Quartz creates new job instance every time it fires a trigger. That is why you see the same account processed twice. It would only work if you use stateful job (IStatefulJob). Or make lock static, but read on.

2) Even if 1) is fixed, it would defeat the purpose of having multiple threads because they will all wait for each other on the same lock. You might as well have one thread doing this.

I don't know enough about requirements especially what's going on in // stuff. It maybe that you don't need this code to run on multiple threads and sequential execution will do just fine. I assume this is not the case and you want to run it multiple threads. The easiest way is to have only one Quartz job. In this job, load Accounts in chunks, say 100 jobs in every chunk. This will give you 5 chunks if you have 500 accounts. Offload every chunk processing to thread pool. It will take care of using optimal number of threads. This would be a poor man's Producer Consumer Queue.

public void Execute(JobExecutionContext context) {

    var linqFindAccount = from Account in MainAccounts
                            where Account.Done == false
                            select Account;
    IList<IList<Account>> chunks = linqFindAccount.SplitIntoChunks(/* TODO */);

    foreach (IList<Account> chunk in chunks) {
        ThreadPool.QueueUserWorkItem(DoStuff, chunk);
    }
}

private static void DoStuff(Object parameter) {
    IList<Account> chunk = (IList<Account>) parameter;

    foreach (Account account in chunk) {
        // stuff
    }
}

As usual, with multiple threads you have to be very careful with accessing mutable shared state. You will have to make sure that everything you do in 'DoStuff' method will not cause undesired side effects. You may find this and this useful.

C# Quartz Race Condition

4 Answers4