Solved

Multithread Synchronization

Posted on 2006-11-21
8
665 Views
Last Modified: 2008-03-10
Hi Experts,

Been a while since I've built a multithreaded application - put together an app that spawns off 300 threads and ofcourse I'm now runing into tons of fun synch issues. I've condensed my code as much possible. WaitForMultipleObjects also does not appear to work - the app just ends without waiting for all threads to finish.  Any help is greatly appreciated.

#include "fifo.h"

#define NUM_THREADS 300

typedef struct{

      int enabled;
      int data;
      int rate;
      FIFO         fifo;      
}Thread_Info_Type;


Thread_Info_Type thread_info[NUM_THREADS] = { 0 };
HANDLE                   thread_event_hdls[NUM_THREADS] = { 0 };


int terminate_flag      = 0;


CRITICAL_SECTION GlobalCriticalSection;


void Worker_Thread(int *i)
{
      int j = *i;
      int data = 0;

      Fifo_Init(&thread_info[j].fifo);
            
      while((!terminate_flag) || (!Is_Fifo_Empty(&thread_info[j].fifo)))
      {
            
            data = Fifo_Pop(&thread_info[j].fifo);      
            
            if(thread_info[j].enabled)
            {
                  printf("Thread %d\n",j);
            }

            Sleep(thread_info[j].rate);
      }

      Fifo_Terminate(&thread_info[j].fifo);
            
      SetEvent(thread_event_hdls[j]);
}


void Initialize_Threads()
{
      HANDLE ThreadHandle;      
      int      i = 0;

      InitializeCriticalSection(&GlobalCriticalSection);
            
      terminate_flag   = 0;

      for( i = 0; i < NUM_THREADS; i++)
      {            
            thread_event_hdls[i] = CreateEvent ( NULL, FALSE, FALSE, NULL);
            thread_info[i].rate  = 20;
                               thread_info[j].enabled = 1;

            ThreadHandle = CreateThread(NULL,
                                          16000,
                                          (LPTHREAD_START_ROUTINE)Worker_Thread,
                                          &i,
                                          0,
                                          0);  
            
            if (ThreadHandle == NULL){      
                  MessageBox(0,"Error Creating Thread!", "Initialize Threads Error!", MB_OK);       
            }            
      
            SetThreadPriority(ThreadHandle,THREAD_PRIORITY_NORMAL);
      }
}

void Terminate_Threads()
{
      int i = 0;

      EnterCriticalSection(&GlobalCriticalSection);
      terminate_flag = 1;
      LeaveCriticalSection(&GlobalCriticalSection);

      WaitForMultipleObjects(NUM_THREADS,thread_event_hdls,TRUE, INFINITE);

      MessageBox(0,"DONE With ALL Threads!", "Terminate Done!", MB_OK);

      DeleteCriticalSection(&GlobalCriticalSection);
}


void main( )
{
      int i = 0;

      Initialize_Threads();
      
      

      for(i = 0; i < NUM_THREADS*5; i++){
      Fifo_Push(&thread_info[i%NUM_THREADS].fifo,i);
      }

      Sleep(1000);
      
      Terminate_Threads();
      
            
}


///////////////////////////////////
fifo
//////////////////////////////////
int Fifo_Init(FIFO *fifo)
{
      ELEMENT *e;
      
      InitializeCriticalSection(&fifo->critical_section);

      EnterCriticalSection(&fifo->critical_section);

      e = malloc(sizeof(ELEMENT));
      
      if( e == NULL ){
            DeleteCriticalSection(&fifo->critical_section);
            MessageBox(NULL,"Malloc Failed!","Fifo_Init Error!",MB_OK);
            return -1;
      }
      e->next = NULL;
      e->data = 0;
      fifo->first = e;
      fifo->last = e;

      LeaveCriticalSection(&fifo->critical_section);

      return 1;
}

void Fifo_Terminate(FIFO *fifo) {
      
      ELEMENT *e, *e_temp;

      EnterCriticalSection(&fifo->critical_section);
      
      e = fifo->first;

      while ( e != NULL ) {
      
            e_temp = e->next;
            free(e);
            e = e_temp;            
      }

      fifo->first = NULL;
      fifo->last  = NULL;

      LeaveCriticalSection(&fifo->critical_section);

      DeleteCriticalSection(&fifo->critical_section);

}
int Fifo_Push(FIFO *fifo,CEI_UINT32 data) {
      
      ELEMENT *e;

      EnterCriticalSection(&fifo->critical_section);

      if( fifo->last == NULL ) {            
            MessageBox(NULL,"Fifo Not Initialized!","Fifo_Push Error!",MB_OK);
            return -1;
      }

      e = malloc(sizeof(ELEMENT));
      
      if( e == NULL ){
            MessageBox(NULL,"Malloc Failed!","Fifo_Push Error!",MB_OK);
            return -1;
      }

      e->data = 0;
      e->next = NULL;
      fifo->last->data = data;
      fifo->last->next = e;
      fifo->last = e;

      LeaveCriticalSection(&fifo->critical_section);

      return 1;
}
CEI_UINT32 Fifo_Pop(FIFO *fifo) {
      
      ELEMENT    *e;
      CEI_UINT32 data;

      EnterCriticalSection(&fifo->critical_section);

      if( fifo->last == fifo->first )
            return fifo->first->data;
      
      e = fifo->first;
      fifo->first = fifo->first->next;
      data = e->data;
      free(e);

      LeaveCriticalSection(&fifo->critical_section);

      return data;
}
int Is_Fifo_Empty(FIFO *fifo) {

      int ret_val;

      EnterCriticalSection(&fifo->critical_section);

      ret_val = fifo->last == fifo->first ? 1 : 0;

      LeaveCriticalSection(&fifo->critical_section);

      return ret_val;

}
0
Comment
Question by:DizZzM
  • 3
  • 3
  • 2
8 Comments
 
LVL 48

Accepted Solution

by:
AlexFM earned 500 total points
Comment Utility
Your way to pass &i to thread is not safe. Pass i by value:

ThreadHandle = CreateThread(NULL,
                                   16000,
                                   (LPTHREAD_START_ROUTINE)Worker_Thread,
                                   (void*)i,   // <<
                                   0,
                                   0);  

void Worker_Thread(void* p)
{
     int j = (int) p;
     ...
}

To wait for threads, you can use thread handles and not thread_event_hdls array. Thread handle is signaled when thread exits, so you can wait directly for array of thread handles returned by CreateThread.
0
 

Author Comment

by:DizZzM
Comment Utility
Thanks for the prompt response Alex -  your suggestion actually fixed a few things. Stopped getting assertion errors and the thread signalling now appears to be working. Appears the indexing was inconsistent the way I had it coded - good catch. I thought *i would copy the value to j and that value would stick until the thread exited but I guess this isn't the case? Mind explaining?

Still having synch issues though - the issue appears to be with my fifo. If I just let the worker thread run and pop items off the fifo all threads exit nicely. If I  put in the following code in main though:

     for(i = 0; i < NUM_THREADS*5; i++){
     Fifo_Push(&thread_info[i%NUM_THREADS].fifo,i);
     }

 then I appear to get into some sort of deadlock. The call from Fifo_Push never returns. I know I need a critical section in there though.

Any ideas?
0
 
LVL 48

Expert Comment

by:AlexFM
Comment Utility
About passing thread number: thread doesn't start immediately when CreateThread is called. When thread starts, it gets pointer (in your version) which points to variable i in Initialize_Threads function. At this time i value can change, go out of array range or even go out of scope. This is why I suggested to pass i by value.

About deadlock: there are many places in your functions when function returns without releasing critical section. For example:

CEI_UINT32 Fifo_Pop(FIFO *fifo) {
     
     ELEMENT    *e;
     CEI_UINT32 data;

     EnterCriticalSection(&fifo->critical_section);

     if( fifo->last == fifo->first )
          return fifo->first->data;    // ??? this is only one place, there are others
     
     e = fifo->first;
     fifo->first = fifo->first->next;
     data = e->data;
     free(e);

     LeaveCriticalSection(&fifo->critical_section);

     return data;
}

You need to leave critical section always. In C++ it is possible to write wrapper class which leaves  critical section in destructor. In C you need to leave critical section manually before every return statement.
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
In Fifo_Push there are some returns where you didn't leave the critical section before. That definitively leads to a dead-lock with the next call to EnterCriticalSection.

You easiliy can solve that problem either by adding a LeaveCriticalSection prior to any return or by using that little helper class:

class Locker
{
      CRITICAL_SECTION* m_pcs;
public:
      Locker(CRITICAL_SECTION* pcs) : m_pcs(NULL) { Lock(pcs); }
      ~Locker()  { Unlock();  }
      void Lock(CRITICAL_SECTION* pcs)
       {  if (m_pcs != NULL && m_pcs != pcs)
                Unlock();
           if (m_pcs == NULL)  EnterCriticalSection(pcs);
           m_pcs = pcs;
       }
      void Unlock()
       {  if (m_pcs != NULL) LeaveCriticalSection(m_pcs); m_pcs = NULL; }
};

You can use it like that:

void f()
{
     Locker lock(&GlobalSection);    // the constructor enters

     if  (...)
         return;   // the destructor will leave the critical section

}


Note, critical sections cannot be nested. If you are already locked regarding one critical section you can't lock again beside you would use a second (different) critical section resource.

void f()
{
   EnterCriticalSection(&c1);

    if (...)
    {
         EnterCriticalSection(&c2);     // using c1 would lead to a dead-lock

Same happens by calling another function that locks using a shared critical section object.

class Locker is a little bit more robust but it cannot really solve the problem that critical sections cannot be nested.

Regards, Alex (itsmeandnobodyelse)

0
IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

 

Author Comment

by:DizZzM
Comment Utility
Amazing the little bugs you over look when coding all day long.... Thanks guys.

Sorry itsmeandnobodyelse but AlexFM really answered this question - the main issue I was having and probably wouldn't of caught solo for a while was the passing by reference to Create_Thread.

Do have one last question though if you guys don't mind:

Kept getting access violation errors once I fixed the FIFO. Looking at my main program, noticed the only issue could be

Thread_Info_Type thread_info[NUM_THREADS];

is a shared resource. However, looking at the code the only shared portion that is currently being (manipulated) used is the Fifo which has its own critical section embedded within.  I wrapped all access to thread_info [] around my global critical section and it fixed the problem. Still not 100% on why it fixed it though - can someone briefly explain? I was under the impression that each item within the array would get it's own memory space and hence be considered it's own single resource. Am I wrong? Should the entire struct - and furthermore entire array of structs be considered a 'single resrouce'?

Additionally - if I do need to wrap every access to the array in a critical section, there's probably no need for me to have a critical section embedded within my fifo right? (Not that it hurts anything if it's synched properly)

Thanks again guys!
0
 
LVL 39

Expert Comment

by:itsmeandnobodyelse
Comment Utility
>>>> Sorry itsmeandnobodyelse but AlexFM really answered this question

No problem, you decide who gets the points (though it is possible to split points if more than one answer helped).

But you need to insert LeaveCriticalSection prior to each return ... or its definitively a dead-lock.

Regards, Alex
0
 
LVL 48

Expert Comment

by:AlexFM
Comment Utility
If you access some array (list, etc.) from different threads, with add/remove/edit operations, you need one critical section for all array.
If you create array (list) before starting threads, and then only edit array members in threads, without add/delete operations, you can keep critical section in every array member.
This is general rule. About your access violation problem - I didn't understand exactly what happens. As usual, it is necessary to see code and exact exception information.
0
 

Author Comment

by:DizZzM
Comment Utility
AlexFM - I opened up a new question to be fair to others who might want to help. The points are also grabs for yourself if you can help relieve me of this headache ;)

http://www.experts-exchange.com/Programming/Programming_Languages/C/Q_22070013.html

0

Featured Post

Enabling OSINT in Activity Based Intelligence

Activity based intelligence (ABI) requires access to all available sources of data. Recorded Future allows analysts to observe structured data on the open, deep, and dark web.

Join & Write a Comment

An Outlet in Cocoa is a persistent reference to a GUI control; it connects a property (a variable) to a control.  For example, it is common to create an Outlet for the text field GUI control and change the text that appears in this field via that Ou…
This is a short and sweet, but (hopefully) to the point article. There seems to be some fundamental misunderstanding about the function prototype for the "main" function in C and C++, more specifically what type this function should return. I see so…
The goal of this video is to provide viewers with basic examples to understand and use structures in the C programming language.
Video by: Grant
The goal of this video is to provide viewers with basic examples to understand and use for-loops in the C programming language.

743 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

17 Experts available now in Live!

Get 1:1 Help Now