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;
}

score 2 · Answer 1 · edited May 23 '17 at 11:49

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().

Tutankhamen · Answer 2 · 2012-09-11T05:28:56.133

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.

edited Sep 11 '12 at 05:28

answered Sep 11 '12 at 05:18

Tutankhamen

3,532
1
30
38

@Tutankhamen, and I'd say it's best to use an *appropriately-scoped* critical section. If it's only going to be used in `ThreadX`, then it should be a static field in `ThreadX`. – Jonathon Reinhart Sep 11 '12 at 05:29
Yes you're right. It's better to use singletoin with reference counting in c-tor and d-tor. Critical section intializes when first copy of ThreadX will be created and deinitializes when destructor of last copy will be called. – Tutankhamen Sep 11 '12 at 05:32
Thanks Tutankhamen. So in this sample program, does '#define WITH_SYNCHRONIZATION' make any difference at all? So what is the author's point to use critical section then? – user1559625 Sep 11 '12 at 08:39
That define will not affect to the program behavior because each of your ThreadX instances will operate with their personal critical section. Can you name the book with such example? – Tutankhamen Sep 11 '12 at 13:09

score 0 · Accepted Answer · answered Sep 11 '12 at 05:19

0

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.

answered Sep 11 '12 at 05:19

wich

16,709
6
47
72

Thanks wich. So is critical section in this program of any use at all? – user1559625 Sep 11 '12 at 08:41
As it is used now it does not do anything; each section only has one user so both threads just steam ahead. If you make a global section that is used by all threads then it does work like you expect, though the two threads may be executed in any order, if thread 2 happens to reach the critical section first it will enter it and stall thread 1, then you will get thread 2: 50'020, thread 1: 100'020. But the other way around is equally valid. Which you get probably depends more on the implementation than luck in a simple case as this. – wich Sep 11 '12 at 09:03

Critical section usage in multithreading?

3 Answers3