WaitOne() waits forever even though all events fired

Question

Threaded is suppossed to create 4 seperate threads and wait for each of them till they finish. Each thread sleeps for a while and terminates only when the shared Mutex opbject is not occupied by another thread and then signal trough a event that it finished(This is a simplified version of my code but fails at the same spot)

But what happens is that most of the time the Main thread will wait forever at one of the WaitOne() seemingly at random.

Also I had to comment-out some parts of my code out because it led to even more unexpected behaviour (I.e somehow after each thread finished the main thread would jump back into the for clause and cause a IndexOutOfBounds)

class Threading
{
    static Mutex CM;
    static List<Manga> SharedList;
    static ManualResetEvent CEvent = new ManualResetEvent(false);
    static ManualResetEvent Event1 = new ManualResetEvent(false);
    static ManualResetEvent Event2 = new ManualResetEvent(false);
    static ManualResetEvent Event3 = new ManualResetEvent(false);
    static ManualResetEvent Event4 = new ManualResetEvent(false);

   public List<Manga> ThreadedMangaIndexCrawl(int MaxThreads)
   {
       CM = new Mutex(false);
       SharedList = new List<Manga>();

       ManualResetEvent[] evs = new ManualResetEvent[4];
       evs[0] = Event1;    // Event for t1
       evs[1] = Event2;    // Event for t2
       evs[2] = Event3;    // Event for t3
       evs[3] = Event4;    // Event for t4

       /*for (int i = 0; i < MaxThreads + 1; i++)
       {
           if (i > MaxThreads)
           { break; }
           Thread t = new Thread(() => this.StartIndexCrawling(1,i,i+1,evs[i]));
           t.Start();
       }*/
       int i = 0;
       Thread t1 = new Thread(() => this.StartIndexCrawling(1, i, i + 1, evs[i]));
       t1.Name = "Thread" + i;
       t1.Start();
       i++;
       Thread t2 = new Thread(() => this.StartIndexCrawling(1, i, i + 1, evs[i]));
       t2.Name = "Thread" + i;
       t2.Start();
       i++;
       Thread t3 = new Thread(() => this.StartIndexCrawling(1, i, i + 1, evs[i]));
       t3.Name = "Thread" + i;
       t3.Start();
       i++;
       Thread t4 = new Thread(() => this.StartIndexCrawling(1, i, i + 1, evs[i]));
       t4.Name = "Thread" + i;
       t4.Start();


      /* foreach (var e in evs)
       { 
           e.WaitOne(); 

       }*/

       evs[0].WaitOne();
       evs[1].WaitOne();
       evs[2].WaitOne();
       evs[3].WaitOne(); 

       return SharedList;
   }

   void StartIndexCrawling(int Target, int Start, int End, ManualResetEvent E)
   {
       Thread.Sleep(1000);
       CM.WaitOne();
       CM.ReleaseMutex();
       E.Set();
   }
}

Any help would be great

Wisner

Are you creating more than one of these objects? The static members seem rather dangerous for a non singleton object. – Marylandmarylee

Consider using Tasks to clean up this logic. – Fenland

@roken: I think Tasks will not cleanup execution flow-mess which is there, this is only an other construct to do async work with more enat helpers/extensions – Rooted

Do you mean more than one "Threading" ? No, I'm just making one instance – Wisner

It seems you never switch a CM mutex into the signaled state so this is a race – Rooted

@Rooted The problem are not the threads. They all finish and every single one of them does its E.Set(). The mutex works as intended, well as far as I can tell – Wisner

Answer 1

Most likely, all four threads will execute:

this.StartIndexCrawling(1, 3, 3 + 1, evs[4]);

This has to do with your use of closures. All four threads will be bound to the variable i and use whatever value it has once the code is executed (and not the value when the Thread object is created).

Your code is unlikely to work if all four threads use the same value.

Bizet

Also check out this post #271940 – Rooted

I'll be damned. That was the last thing I considered to be the cause. But why do the threads execute themselves with the current i and not the one they got trough the parameters? //edit just saw the link after posting – Wisner

Within StartIndexCrawling you have a local copy of the values. But the top-most code of the thread is the anonymous function defined by the closure () => this.StartIndexCrawling(1, i, i + 1, evs[i]). And this code refers to shared variable i. That's how closures work. – Bizet

Answer 2

See Codo's answer.
Here is what you should do to solve it:

   int i = 0;
   Thread t1 = new Thread(() => this.StartIndexCrawling(1, 0, 1, Event1));
   t1.Name = "Thread" + i;
   t1.Start();
   i++;
   Thread t2 = new Thread(() => this.StartIndexCrawling(1, 1, 2, Event2));
   t2.Name = "Thread" + i;
   t2.Start();
   i++;
   Thread t3 = new Thread(() => this.StartIndexCrawling(1, 2, 3, Event3));
   t3.Name = "Thread" + i;
   t3.Start();
   i++;
   Thread t4 = new Thread(() => this.StartIndexCrawling(1, 3, 4, Event4));
   t4.Name = "Thread" + i;
   t4.Start();

Giulio