Nested locks (critical section) not working

Question

I am learning about Critical Section (for the purpose of multithreading) and I found a class online using it. I don't understand why my code doesn't work though - I should get "success" on the console display but I don't.

Am I locking it incorrectly? I am sure I am entering and exiting the sections accurately - but I do not know why the third thread (mul) does not seem to work.

Here is the main code (doing this on VS 2012):

#include "stdafx.h"
#include <windows.h>
#include <process.h>
#include <iostream>
#include <assert.h>
#include <queue>
#include "Lock.h"

//File: CriticalSectionExample.cpp

#define MAX_THREADS 2

using namespace std;

static unsigned int counter = 100;
static bool alive = true;
static examples::Lock lock_1;
static examples::Lock lock_2;

queue<int> test_q;
queue<int> later_q;

static unsigned __stdcall sub(void *args)
{
    while(alive)
    {
        cout << "tq";
        lock_1.acquire();
        test_q.push(1);
        lock_1.release();

        ::Sleep(500);
    }
    return 0;
}

static unsigned __stdcall add(void *args)
{
    while(alive)
    {
        if (!test_q.empty())
        {
            int first = test_q.front();
            //cout << first << endl;

            lock_1.acquire();
            test_q.pop();
            lock_1.release();

            lock_2.acquire();
            cout << "lq" << first << endl;
            later_q.push(first);
            lock_2.release();
        }

        ::Sleep(500);
    }
    return 0;
}

static unsigned __stdcall mul(void *args)
{
    while(alive)
    {
        if (!later_q.empty())
        {
            cout << "success" << endl;
            lock_2.acquire();
            test_q.pop();
            lock_2.release();
        }

        ::Sleep(500);
    }
    return 0;
}

int main()
{
    // create threads
    unsigned tadd;
    HANDLE hadd = (HANDLE) ::_beginthreadex(0, 0, &add, 0, CREATE_SUSPENDED, &tadd);
    assert(hadd != 0);

    unsigned tsub;
    HANDLE hsub = (HANDLE) ::_beginthreadex(0, 0, &sub, 0, CREATE_SUSPENDED, &tsub);
    assert(hsub != 0);

     unsigned tmul;
    HANDLE hmul = (HANDLE) ::_beginthreadex(0, 0, &mul, 0, CREATE_SUSPENDED, &tsub);
    assert(hmul != 0);

    // start threads
    ::ResumeThread(hadd);
    ::ResumeThread(hsub);

    ::Sleep(10000); // let threads run for 10 seconds

    // stop & cleanup threads
    alive = false;
    ::WaitForSingleObject(hsub, INFINITE);
    ::CloseHandle(hsub);
    ::WaitForSingleObject(hadd, INFINITE);
    ::CloseHandle(hadd);

    return 0;
}

and this is the header file including Critical Section:

#ifndef _Lock_H_
#define _Lock_H_


#include <windows.h>

/**
*@description: A simple Lock implementation using windows critical section object
*/

namespace examples
{
    class Lock
    {
    public:
        Lock()
        {
            ::InitializeCriticalSection(&m_cs);
        }

        ~Lock()
        {
            ::DeleteCriticalSection(&m_cs);
        }

        void acquire()
        {
            ::EnterCriticalSection(&m_cs);
        }

        void release()
        {
            ::LeaveCriticalSection(&m_cs);
        }

    private:
        Lock(const Lock&);
        Lock& operator=(const Lock&);

        CRITICAL_SECTION m_cs;
    };

}

#endif //_Lock_H_

Answer 1

It seems that you forgot to Resume your third thread.

I'd like to say that there is several potential flaws in your code. I'd recommend you to Lock each place where you work with your shared variables ( yes, even if you only read their value ). This time it may work, but sometimes even reading object which is being modified by other thread can be dangerous.

Also you can apply a bit more complex Free/Lock pattern to your code, so you don't need to call acquire/release manually

class AutoLock
{
public:
    AutoLock(Lock& l)
    :lock(l)
    {
        lock.acquire();
    }

    ~AutoLock()
    {
        lock.release();
    }

    Lock& lock;
 };

Then you can rewrite your functions to make them cleaner and safer:

static unsigned __stdcall sub(void *args)
{
    while(alive)
    {
        cout << "tq";
        {
            examples::AutoLock lock(lock_1);
            test_q.push(1);
        }
        ::Sleep(500);
    }
    return 0;
}

Note that separate scope is required because otherwise critical section lock_1 will be locked until ::Sleep(500) executed which is not what you normally want.