Static Variables in Multithreaded Applications

Hi,

Pl. refer the following sample code and let me know whether the code is thread safe in accessing the static variable.

I already test run the code for more than 3 days continuously but the application didn't crash. I want to know abt the use of static variable in Mulithreaded applications, as one of my application is using the same logic.

------ Classone.cpp -------

CClassOne::CClassOne()
{
m_iCount =0;
}

CClassOne::~CClassOne()
{

}

void CClassOne::RunClassOne()
{
if (s_iloopCount >=100)
m_icount =0;
else
m_icount++;
}

void CClassOne::UpdateStaticValue()
{
s_iloopCount = s_iloopCount +2;
}


------ Classone.h -------
static int s_iloopCount =0;
class CClassOne
{
public:
CClassOne();
virtual ~CClassOne();
void RunClassOne();
void UpdateSaticValue();
protected:
int m_iCount;
};


------- main --------

DWORD WINAPI CThreads::ThreadRoutine1(LPVOID lpv)
{
DWORD dwTimeInterval = 12 * 1000;
CClassOne *pClassOne =NULL;
pClassOne = new CClassOne;
do
{
returnvalue=WaitForSingleObject(hEvent,dwInterval);
if (returnvalue==WAIT_OBJECT_0)
break;
pClassOne->RunClassOne();
}while(true);
if (pClassOne !=NULL)
delete pClassOne ;
return 0;
}

DWORD WINAPI CThreads::ThreadRoutine2(LPVOID lpv)
{
DWORD dwInterval = 15 * 1000;
CClassOne *pClassOne =NULL;
pClassOne = new CClassOne;
do
{
returnvalue=WaitForSingleObject(hEvent,dwInterval);
if (returnvalue==WAIT_OBJECT_0)
break;
pClassOne->UpdateStaticValue();
}while(true);
if (pClassOne !=NULL)
delete pClassOne ;
return 0;
}


int main(int argc, char* argv[])
{
DWORD dwThID = 0;
HANDLE hThread1, hThread2;
HANDLE hEvent;
hEvent = CreateEvent(NULL,false,false,NULL);
hThread1 = ::CreateThread(NULL, 0, ThreadRoutine1, NULL, 0, &dwThID);
hThread2 = ::CreateThread(NULL, 0, ThreadRoutine2, NULL, 0, &dwThID);

.....
do
{
Sleep(60000);
} while (true);

}

There is a small chance of UpdateStaticValue being called after

if (s_iloopCount >=100)

but before

m_icount++;

In the count, m_icount is not used as an array index but if it were, there would be an occassional occurence of crashitis.

If you add an array of 102 elements to ClassOne and initialize its values to 0, then set array[m_icount] to 1 after the if statement in UpdateStaticValue, you may find that occassionally arra[101] is set.

Can't add much to cup's reply, but on a stylistic note: don't declare the static in the header file - you'll get a different copy in every implementation file that you include it in - ideed there's no reason to publically declare its existance. Secondly, modern practice is to use unnamed namespaces, not file-level statics.

Code:

---

------ Classone.cpp -------

namespace
{
    int s_iLoopCount = 0;
}

CClassOne::CClassOne() : m_iCount(0) // Preferred method of initialising data members
{
}

CClassOne::~CClassOne()
{
}

void CClassOne::RunClassOne()
{
    if (s_iloopCount >=100)
        m_icount =0;
    else
        m_icount++;
}

void CClassOne::UpdateStaticValue()
{
    s_iloopCount = s_iloopCount +2;
}

------ Classone.h -------

class CClassOne
{
public:
    CClassOne();
    virtual ~CClassOne();
    void RunClassOne();
    void UpdateSaticValue();
protected:
    int m_iCount;
};

---

Other points to note:

Are you intending to derive other classes from CClassOne? If not, remove the "virtual" from the destructor. m_iCount should probably be private, not protected, since, even if you are going to derive from CClassOne, you probably don't want to give derived classes access to it.

Code:

---

DWORD WINAPI CThreads::ThreadRoutine1(LPVOID lpv)
{
    DWORD dwTimeInterval = 12 * 1000;
    // Avoid NULL in C++ programs - use 0 instead.
    CClassOne *pClassOne =NULL;
    // combine with previous line: CClassOne* p = new CClassOne
    // or (preferably) just declare an object, rather than a pointer
    pClassOne = new CClassOne;
    do
    {
        returnvalue=WaitForSingleObject(hEvent,dwInterval);
        // Reverse the test and avoid an ugly break:
        // if (returnvalue != WAIT_OBJECT_0)
        //      pClassOne->RunClassOne();
        if (returnvalue==WAIT_OBJECT_0)
            break;
        pClassOne->RunClassOne();
    // Then put in a proper termination test here
    }while(true);
    // Don't need to test for pointer being 0, "delete 0" is valid C++
    // Anyway, if pClassOne is 0, then code above this has already
    // crashed the program.
    if (pClassOne !=NULL)
        delete pClassOne ;
    return 0;
}

---