Getting Releasing unheld lock CriticalSection in Windows C++ code

Question

I'm getting a warning "Releasing unheld lock 'csCriticalSection'" at the 2nd try below.

Here is my code, but without all the ADO code. I don't know why I'm getting this warning, because if the Execute() function fails, the catch will run a LeaveCriticalSection function. If the Execute() function succeeds, I call LeaveCriticalSection.

#include <windows.h>
#include <synchapi.h>
#include <comdef.h>
#include <conio.h>

CRITICAL_SECTION csCriticalSection;

_ConnectionPtr connectioncreate = nullptr;
_RecordsetPtr recordset = nullptr;

int main()
{
InitializeCriticalSectionAndSpinCount(&csCriticalSection, 4000);

// *** all ADO code here ***
//
// *** all ADO code here ***

BOOL bRanLeaveCriticalSection = FALSE;
try
    {
    EnterCriticalSection(&csCriticalSection);
    // Run an SQL command with ADO.
    /*
    connectioncreate->BeginTrans();
    connectioncreate->Execute(sSQL.data(), nullptr, adCmdText);
    connectioncreate->CommitTrans();
    */
    LeaveCriticalSection(&csCriticalSection);
    bRanLeaveCriticalSection = TRUE;
    }

catch (CONST _com_error& err)
    {
    connectioncreate->CommitTrans();
    if (bRanLeaveCriticalSection == FALSE)
        LeaveCriticalSection(&csCriticalSection);
    }


try
    {
    // From compiler at the next line "if (recordset)":
    // Warning  C26117  Releasing unheld lock 'csCriticalSection'
    if (recordset)
        if (recordset->State == adStateOpen)
            recordset->Close();
    }
catch (CONST _com_error& err)
    {
    err;
    }

_getch();
}

Can anyone help me to for the reason why I'm getting this warning and how to fix it?

Answer 1

You are not initializing csCriticalSection before using it, eg:

...
CRITICAL_SECTION csCriticalSection;
...

int main()
{
    InitializeCriticalSection(&csCriticalSection); // <-- ADD THIS
    ...
    DeleteCriticalSection(&csCriticalSection); // <-- ADD THIS
}

But also, you really shouldn't be using bRanLeaveCriticalSection at all, this is just bad code design.

You can use a __try/__finally block instead (if your compiler supports that), eg:

...
CRITICAL_SECTION csCriticalSection;
...

int main()
{
    ...

    try
        {
        EnterCriticalSection(&csCriticalSection);
        __try
            {
            // Run an SQL command with ADO.
            }
        __finally
            {
            LeaveCriticalSection(&csCriticalSection);
            }
        }
    catch (CONST _com_error& err)
        {
        ...
        }

    ...
}

However, the best option is to use an RAII wrapper instead, let it call EnterCriticalSection() and LeaveCriticalSection() for you, eg:

struct CRITICAL_SECTION_LOCK
{
    CRITICAL_SECTION &m_cs;

    CRITICAL_SECTION_LOCK(CRITICAL_SECTION &cs) : m_cs(cs) {
        EnterCriticalSection(&m_cs);
    }

    ~CRITICAL_SECTION_LOCK() {
        LeaveCriticalSection(&m_cs);
    }
};

...

CRITICAL_SECTION csCriticalSection;

int main()
{
    ...

    try
        {
        CRITICAL_SECTION_LOCK lock(csCriticalSection);
        // Run an SQL command with ADO.
        }
    catch (CONST _com_error& err)
        {
        ...
        }

    ...
}

Or better, use a C++ std::mutex instead, with a proper RAII lock such as std::lock_guard, eg:

#include <mutex>

std::mutex mtx;

...

int main()
{
    try
        {
        std::lock_guard<std::mutex> lock(mtx);
        // Run an SQL command with ADO.
        }
    catch (CONST _com_error& err)
        {
        ...
        }

    ...
}