Problems using EnterCriticalSection

Question

I need to work with array from several threads, so I use CRITICAL SECTION to give it an exclusive access to the data.
Here is my template:

#include "stdafx.h"
#ifndef SHAREDVECTOR_H
#define SHAREDVECTOR_H

#include <vector>
#include <windows.h>

template<class T>
class SharedVector {
    std::vector<T> vect;
    CRITICAL_SECTION cs;
    SharedVector(const SharedVector<T>& rhs) {}
public:
    SharedVector();
    explicit SharedVector(const CRITICAL_SECTION& CS);
    void PushBack(const T& value);
    void PopBack();
    unsigned int size() const;
    T& operator[](int index);
    virtual ~SharedVector();
};

template<class T>
SharedVector<T>::SharedVector() {
    InitializeCriticalSection(&cs);
}

template<class T>
SharedVector<T>::SharedVector(const CRITICAL_SECTION& r): cs(r) {
    InitializeCriticalSection(&cs);
}

template<class T>
void SharedVector<T>::PushBack(const T& value) {
    EnterCriticalSection(&cs);
    vect.push_back(value);
    LeaveCriticalSection(&cs);
}

template<class T>
void SharedVector<T>::PopBack() {
    EnterCriticalSection(&cs);
    vect.pop_back();
    LeaveCriticalSection(&cs);
}

template<class T>
unsigned int SharedVector<T>::size() const {
    EnterCriticalSection(&cs);
    unsigned int result = vect.size();
    LeaveCriticalSection(&cs);
    return result;
}

template<class T>
T& SharedVector<T>::operator[](int index) {
    EnterCriticalSection(&cs);
    T result = vect[index];
    LeaveCriticalSection(&cs);
    return result;
}

template<class T>
SharedVector<T>::~SharedVector() {
    DeleteCriticalSection(&cs);
}

While compiling I have such a problem for calling EnterCriticalSection(&cs) and LeaveCriticalSection(&cs):

'EnterCriticalSection' : cannot convert parameter 1 from 
'const CRITICAL_SECTION *' to 'LPCRITICAL_SECTION'

I do not know what is wrong. May be you can see. Just because I always used it this way and it was alright. windows.h is included

Answer 1

Just declare cs as:

mutable CRITICAL_SECTION cs;

or else remove the const clause on size()

Entering a critical section modifies the CRITICAL_SECTION, and leaving modifies it again. Since entering and leaving a critical section doesn't make the size() method call logically non-const, I'd say leave it declared const, and make cs mutable. This is the type of situation mutable was introduced for.

Also - take a look at Martin York's and Joe Mucchiello's suggestions - use RAII whenever possible to deal with any kind of resources that need to be cleaned up. This works just as well for critical sections as it does for pointers and file handles.

Answer 2

Also the code above is not Exception safe.
There is no guarantee that push_back() pop_back() will not throw. If they do they will leave your critical section permanently locked. You should create a locker class that calls EnterCriticalSection() on construction and LeaveCriticalSection() on destruction.

Also this makes your methods a lot easier to read. (see below)

class CriticalSectionLock
{
    public:
        CriticalSectionLock(CRITICAL_SECTION& cs)
            : criticalSection(cs)
        {
            EnterCriticalSection(&criticalSection);
        }
        ~CriticalSectionLock()
        {
            LeaveCriticalSection(&criticalSection);
        }
    private:
        CRITICAL_SECTION&  criticalSection;
};


// Usage
template
unsigned int SharedVector::size() const
{
    CriticalSectionLock  lock(cs);
    return vect.size();
}

Another thing you should worry about. Is making sure that when you destroy the object you have ownership and that nobody else tries to take ownership during destruction. Hopefully your DestoryCriticalSection() takes care of this.