Critical section usage in multithreading?

Question

I found this tutorial on web regarding multithreading and download code to try on visual studio 2010. http://www.codeproject.com/Articles/14746/Multithreading-Tutorial

One of the program related to 'Thread Local Storage' which i copied below for your reference. It looks very simple as 2 threads, both increasing class data member 'm1', 'm2', 'm3'. In this example 'm2' is a static class variable that can be accessed by both threads.

Notice in the code, critical section is enabled with '#define WITH_SYNCHRONIZATION' at the beginning of file. My understanding is that since the 'for loop' in 'TMain()' is protected by critical section, whichever thread that gets to 'TMain()' first would finish the 50,000 increment as a whole with no interleaving from the other thread, print out '50,020 for m2', and the other thread would continue the rest and print out '100,020 for m2' later.

But no, the print out of m2 looks like there's no critical section at all. The 'm2' value is scrambled with values like:

Thread t2: ... m2=50376 ...
Thread t1: ... m2=63964 ...

I must have missed something basic. What is it?

Below code is from the webpage with minor value change, can compile in VStudio readily.

#include <stdio.h>
#include <windows.h>
#include <process.h>
#include <string>

using namespace std;

#define WITH_SYNCHRONIZATION

class ThreadX
{
private:
  int        m1;
  static int m2;                       // shared variable
  static  __declspec(thread)  int m3;  // thread local variable


#ifdef WITH_SYNCHRONIZATION
  CRITICAL_SECTION m_CriticalSection; 
#endif

public:
  string threadName;

  ThreadX()
  {
      m1 = 10;
#ifdef WITH_SYNCHRONIZATION
        InitializeCriticalSection(&m_CriticalSection);
#endif
  }

  virtual ~ThreadX()
  {
#ifdef WITH_SYNCHRONIZATION
       // Release resources used by the critical section object.
       DeleteCriticalSection(&m_CriticalSection);
#endif
  }

  void TMain(void) 
  {
#ifdef WITH_SYNCHRONIZATION
    EnterCriticalSection( &m_CriticalSection );
#endif

    for ( int i = 1; i <= 50000; i++ )
    {
        ++m1;  // init value 10
        ++m2;  // init value 20
        ++m3;  // init value 30
    }

    printf( "Thread %s: m1 = %d, m2 = %d, m3 = %d\n", threadName.c_str(), m1, m2, m3 );

#ifdef WITH_SYNCHRONIZATION
    LeaveCriticalSection( &m_CriticalSection );
#endif

  } 

  static unsigned __stdcall ThreadStaticTMain(void * pThis)
  {
      ThreadX * pthX = (ThreadX*)pThis;
      pthX->TMain();

      return 1;
  }

};

int ThreadX::m2 = 20;
int ThreadX::m3 = 30;

int main()
{
    // In this program we create 2 threads and request that their
    // entry-point-function be the TMain() function of the ThreadX
    // class.  Because _beginthreadex() cannot accept a class member
    // function we must employ a 2 step process involving a tricky
    // cast to accomplish this.

    ThreadX * o1 = new ThreadX();

    HANDLE   hth1;
    unsigned  uiThread1ID;

    hth1 = (HANDLE)_beginthreadex( NULL,         
                                   0,            
                                   ThreadX::ThreadStaticTMain,
                                   o1,           
                                   CREATE_SUSPENDED, 
                                   &uiThread1ID );

    if ( hth1 == 0 )
        printf("Failed to create thread 1\n");

    DWORD   dwExitCode;

    GetExitCodeThread( hth1, &dwExitCode );
    printf( "initial thread 1 exit code = %u\n", dwExitCode );

    o1->threadName = "t1";

    ThreadX * o2 = new ThreadX();

    HANDLE   hth2;
    unsigned  uiThread2ID;

    hth2 = (HANDLE)_beginthreadex( NULL,         
                                   0,            
                                   ThreadX::ThreadStaticTMain,
                                   o2,           
                                   CREATE_SUSPENDED, 
                                   &uiThread2ID );

    if ( hth2 == 0 )
        printf("Failed to create thread 2\n");

    GetExitCodeThread( hth2, &dwExitCode ); 
    printf( "initial thread 2 exit code = %u\n", dwExitCode );

    o2->threadName = "t2";

    ResumeThread( hth1 );   
    ResumeThread( hth2 );   

    WaitForSingleObject( hth1, INFINITE );  
    WaitForSingleObject( hth2, INFINITE );  

    GetExitCodeThread( hth1, &dwExitCode );
    printf( "thread 1 exited with code %u\n", dwExitCode );

    GetExitCodeThread( hth2, &dwExitCode );
    printf( "thread 2 exited with code %u\n", dwExitCode );

    // The handle returned by _beginthreadex() has to be closed
    // by the caller of _beginthreadex().

    CloseHandle( hth1 );
    CloseHandle( hth2 );

    delete o1;
    o1 = NULL;

    delete o2;
    o2 = NULL;

    printf("Primary thread terminating.\n");
    return 0;
}

Answer 1

m_CriticalSection member is different for each instance of your ThreadX class. So, each ThreadX object uses its own critical section - and this is why it's not affect to each other.

Answer 2

CRITICAL_SECTION m_CriticalSection; is a member (instance) variable in your ThreadX class. That means every time you create an instance of ThreadX (which you do twice), you are creating a new CRITICAL_SECTION. This does no good, because each instance is going to enter its own critical section, no problem, and proceed to trash the variables you are trying to protect.

If you look at the documentation for EnterCriticalSection, you'll see that it mentions creating just one CRITICAL_SECTION, which each thread will try to enter.

Instead, you need to create just one CRITICAL_SECTION which all instances of ThreadX will use. Ideally, this would be a static member variable in ThreadX. However, C++ doesn't have static constructors (like C#), so you'll have to make the call to InitializeCriticalSection in some other fashion. You could always just make it a static variable that you initialize in main().

Answer 3

It is because you define one critical section for each thread. You need to define a global critical section in main and pass that critical section to each thread class.