Two-layer RAII to support a critical section

Question

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.

Answer 1

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.

Answer 2

I was always wondering it if can be done this way:

void some_function () {
     padlock mt_guard{} ;
     // we are all alone here
     // no MT
}

And the padlock might be something akin to this:

struct padlock final {
    padlock () {
        InitializeCriticalSection(&m_CS); 
        EnterCriticalSection(&m_CS);
    }
    ~padlock () {
        LeaveCriticalSection(&m_CS);
        DeleteCriticalSection(&m_CS);
    }
private:
    CRITICAL_SECTION    m_CS;
}

This might work?