c# while loop runs extra time

Question

Trying to make program run on list of urls and open limited number of threads to process them. Problem is that the while loop will execute when listPosition is 18, even that the siteUrls.Count is 18. But it will never show any of those test messageboxes i added. Thread code itself has a locker that does currentThreads-- in the end of thread's work.

Why does it start the while loop when the condition becomes (18<18)? Why does it try to launch thread with the 18 but it wont show a message box? Problem exists only when i put more threads than list items count. Its strange to me because i think it should not run the loop when listPosition equals _siteUrls.Count. I tried adding various "waiting" for threads to start and what not, but it still doesn't work.

p.s. when i add 5ms wait before increasing listPosition it becomes more stable. What exactly does it need to wait for so i can program it in more nicely?

p.p.s Thread doesn't operate any of the counters except doing lock (ThreadsMinus) { currentThreads--;}

  listPosition = 0;

    while (listPosition < siteUrls.Count)
    {
        if (listPosition == 18)
        {
            MessageBox.Show("18");
        }
        currentThreads++;
        var t = new Thread(() => Register(siteUrls[listPosition], listPosition));
        t.Start();

        if (listPosition == 18)
        {
            MessageBox.Show("18");
        }
        while (currentThreads >= maxThreads)
        {
            Thread.Sleep(50);
        }
         listPosition++;
    }

Where are you decrementing currentThreads? From the code you have posted it looks like you could get into an infinite loop as soon currentThreads == maxThreads - which could explain why you never see your message boxes. — Mike Parkhill, Dec 11 '12 at 02:55
I wrote there that the code of the thread itself has the currentThreads-- in the end of its work. Breaking my head for a week over this piece of code — Dim NY, Dec 11 '12 at 02:57
Too bad, .Net 4 has the Parallel.ForEach() method which would help this a lot. http://msdn.microsoft.com/en-us/library/dd460720.aspx — Mike Parkhill, Dec 11 '12 at 03:00
Need that to be compatible with winxp. But its not about the threading, its something wrong with code design. Or .net bug. — Dim NY, Dec 11 '12 at 03:01

score 2 · Accepted Answer · answered Dec 11 '12 at 03:11

2

Your code manipulates counters (listPosition and currentThreads) in unsafe way. Please use Interlocked.Increment and Interlocked.Decrement or appropriate lock around access to each counter.

Also it is unclear from the sample where currentThreads is decreased (as pointed out by Mike Parkhill), which is the most likely reason of infinite loop.

answered Dec 11 '12 at 03:11

Alexei Levenkov

98,904
14
127
179

But why? I suspected that.This while loop is currently executed from button click event so it should be one thread. And i'm decreasing threads count inside locker. Thread itself doesnt operate listPosition at all – Dim NY Dec 11 '12 at 03:12
There is no code shown that decreases either counter, so assumption is that both are decreased in `Register` call that runs on other thread. And no locking code shown either, so it is unclear what you are locking on (also clearly it is wrong as correct locking would block new thread from modifying either of the counters since the only place in current sample to do `lock` is outside whole `while` loop) – Alexei Levenkov Dec 11 '12 at 03:17
I do not decrease listPosition at all, and for the second counter i'm using locker object and locking listPosition to decrease it right before thread ends. So when it comes to 18 it shouldnt start the while loop again. And i will be glad if you point me out why do i need to lock in the while loop itself. lock (ThreadsMinus) { _currentThreads--; } is what i have in the thread. And i dont see reason to lock them in the while loop itself. – Dim NY Dec 11 '12 at 03:21
You need to lock inside the while loop because it also a thread that is sharing the currentThreads variable with the other threads. Anywhere that references it should lock it. I believe my answer will solve your stability issues without having to use Sleeps to block the main thread. – Mike Parkhill Dec 11 '12 at 03:24
Oh, i see. I thought that if it is locked somewhere - it can't be accessed from anywhere. Do i need to create separate lockers for each counter and also for increaing\decreasing of each? – Dim NY Dec 11 '12 at 03:26
@DimaNych, You need to lock around every access to piece of data that can be changed by multiple threads. For some reason you have code where only decrement has lock, but increment and read don't. Interlocked + volatile will probably solve your issue, but I'd recommend starting with `lock` in all places where you access the counter. Make sure to use the same object to lock for the same piece of data (i.e. for the counter should be exactly one locking object). – Alexei Levenkov Dec 11 '12 at 03:28
I see, and do i need to create new objects for each of those cases or just use 1 for each counter? Thanks a lot guys! – Dim NY Dec 11 '12 at 03:33
@DimaNych, Number of object to lock on (search term is "lock granularity") depends on how you need to protect data. In your case one would be enough, but in general you need to look at your data - how it is changing/shared and determine how many lock objects you need. – Alexei Levenkov Dec 11 '12 at 03:42
I probably misunderstand how lock works, will try to find examples with multiple locks tomorrow and reread about locks in general. Thanks a lot for your help Alexei. – Dim NY Dec 11 '12 at 03:46

Mike Parkhill · Answer 2 · 2012-12-11T03:16:04.323

1

Try the following (note you need to lock currentTheads in here too):

while (listPosition < siteUrls.Count)
{


    lock(ThreadsMinus){
      // only start a new thread if we haven't hit the maximum yet, otherwise keep spinning
      if (currentThreads < maxThreads){
         currentThreads++;
         var t = new Thread(() => Register(siteUrls[listPosition], listPosition));
          t.Start();
          listPosition++;
      } 
    }

}

It will only spark off a new thread if you're under the maxThreads, but because not every iteration will start a thread you could iterate multiple times for any given listPosition until a thread becomes available. So even though you only have 18 urls in your list you could iterate over that while loop a thousand times before they've all been serviced depending on how long your Register method takes.

edited Dec 11 '12 at 03:16

answered Dec 11 '12 at 03:09

Mike Parkhill

5,511
1
28
38

You can't lock on value types (possible approach - http://stackoverflow.com/questions/420825/how-to-properly-lock-a-value-type)... – Alexei Levenkov Dec 11 '12 at 03:12
But i do have private static readonly object ThreadsMinus = new object(); lock (ThreadsMinus) { _currentThreads--; } – Dim NY Dec 11 '12 at 03:14
@Alexie, you're absolutely right. Meant to use a lock object with a related named - going on an assumption of the name of the lock object that OP is using in their other (unposted) method. – Mike Parkhill Dec 11 '12 at 03:15
+1. @DimaNych - see my comment to other answer... You are not doing locking correctly (most likely only on `--`, but not on `++`). – Alexei Levenkov Dec 11 '12 at 03:21

c# while loop runs extra time

2 Answers2