2
\$\begingroup\$

For the code, if one considers using RAII to encapsulate CriticalSection (global scope), two RAII layers are needed: InitializeCriticalSection layer (main function scope), enter EnterCriticalSection layer (multi-thread based Proc scope).

// Global variable
CRITICAL_SECTION CriticalSection; 

int main( void )
{
    // Initialize the critical section one time only.
    if (!InitializeCriticalSection(&CriticalSection) ) 
        return;
    ...

    // Release resources used by the critical section object.
    DeleteCriticalSection(&CriticalSection);
}

DWORD WINAPI ThreadProc( LPVOID lpParameter )
{
    // Request ownership of the critical section.
    EnterCriticalSection(&CriticalSection); 

    // Access the shared resource.

    // Release ownership of the critical section.
    LeaveCriticalSection(&CriticalSection);

    return 1;
}

As CriticalSection and the two layers all have different scopes, and also enter InitializeCriticalSection layer must be on top of EnterCriticalSection layer.

class CriticalSectionGuard {
public:
    CriticalSectionGuard(CRITICAL_SECTION * cs) : m_CS(cs) {
        InitializeCriticalSection(m_CS);
    }
    ~CriticalSectionGuard() {
        DeleteCriticalSection(m_CS);
    }
    void lock() {
        EnterCriticalSection(m_CS);
    }
    void unlock{
        LeaveCriticalSection(m_CS);
    }
private:
    CRITICAL_SECTION * m_CS;
}

class GetLock {
public:
    GetLock(CriticalSectionGuard* plock) : m_csg(plock) {
        m_csg->lock();
    }
    ~GetLock(void) {
        m_csg->unlock();
    }
private:
    CriticalSectionGuard * m_csg;
};



// Global variable
CRITICAL_SECTION CriticalSection;

int main(void)
{
    // Initialize the critical section one time only.
    CriticalSectionGuard CSG(&CriticalSection);
    ...
}
DWORD WINAPI ThreadProc(LPVOID lpParameter)
{
    // we need pass along the CSG Pointer, which is messy.
    GetLock(&CSG);
    ...

        return 1;
}

It looks quite bad, as the CriticalSectionGuard object is not in the global scope, it need pass along to threads.

Please share any suggestions or criticisms to improve the code.

\$\endgroup\$
3
\$\begingroup\$

We can simplify that a lot.

You are not really using RAII on the CriticalSection as you declare it in one place then initialize it in another. But CRITICAL_SECTION is a type as you pass a pointer around.

Why not simplify a bit:

class GetLock;
class CriticalSection {
public:
    CriticalSection() {
        InitializeCriticalSection(&m_CS); // get a pointer to the member
    }
    ~CriticalSection() {
        DeleteCriticalSection(&m_CS);
    }
private:
    friend class GetLock;   // Only GetLock should be locking/unlocking this object.
    void lock() {
        EnterCriticalSection(&m_CS);
    }
    void unlock{
        LeaveCriticalSection(&m_CS);
    }
private:
    CRITICAL_SECTION    m_CS;   // Make this a member
                                // Now make the constructor initialize it.
}

Now its use case becomes.

CriticalSection  criticalSection;

int main() {
    // Critical section variable already up and running.
}

If we look at the GetLock.
I would not passes pointers as parameters. This means people can accidentally pass a nullptr and thus cause your code to break. Always pass by reference if you have to have parameter there.

class GetLock {
public:
    // Pass a reference to the CriticalSection
    GetLock(CriticalSection& plock)
        : m_csg(plock)
    {
        m_csg.lock();
    }
    ~GetLock(void) {
        m_csg.unlock();
    }
private:
    CriticalSectionGuard& m_csg;   // Keep a reference to the critical section.
};

Now this can be used like:

DWORD WINAPI ThreadProc(LPVOID lpParameter)
{
    GetLock lock(criticalSection);

    return 1;
}

Note the difference above to your version:

    GetLock(&CSG);   // This creates a temporary object.
                     // This object is destroyed at the end of the
                     // statement (which is the ;).
                     // This mean you call lock() and unlock() before
                     // the end of the line.

I should also note that using global variables is considered bad practice. You should try and design your program so that you pass the things being manipulated around as parameters (or part of an object). I can't really suggest an improvement with knowing more about the code.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the excellent answer. What if you put 'CriticalSection criticalSection;' in main(maybe some other functions you need a new criticalSection), then how to call GetLock in thread proc, if you do not pass the pointer along. \$\endgroup\$ – phy nju Dec 13 '17 at 19:22
  • \$\begingroup\$ what if you put 'CriticalSection criticalSection;' in main( Then you pass a reference to the critical section as a parameter to that piece of code. That also makes that piece of code more general as you can specify the exact critical section it depends on. \$\endgroup\$ – Martin York Dec 13 '17 at 19:26
  • \$\begingroup\$ how to call GetLock in thread proc. Same answer as above. Pass it as a parameter (not nesacerily directly it can be part of another structure). But you can pass parameters when a thread is started in fact all threading code allow you to pass context. \$\endgroup\$ – Martin York Dec 13 '17 at 19:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.