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

Bellebelleek

Don't write template <typename T>, but template <typename T> – Inanity

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.

Guff

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.

Brickey

Answer 3

I prefer using a separate Acquisition object over your code. Your code if fragile when an exception occurs between the Enter and Leave calls:

class CS_Acquire {
    CRITICAL_SECTION &cs;
public:
    CS_Acquire(CRITICAL_SECTION& _cs) : cs(_cs) { EnterCriticalSection(cs); }
    ~CS_Acquire() { LeaveCriticalSection(cs); }
};

Then in your class methods you would code it as:

template <typename T>
void SharedVector::PushBack(const T& value) {
   CS_Acquire acquire(&cs);
   vect.push_back(value);
}

Molehill

Answer 4

EnterCriticalSection doesn't take a const argument. That's a compilation error, BTW, not a linking error...

Also, are you sure you want to pass in a critical section into your ctor, and then have the ctor perform the InitializeCriticalSection call? If you want to share your critical section, I suppose you'd initialize it first, and then hand it out.

Candlepower

Answer 5

I see you have declared an empty copy constructor:

SharedVector(const SharedVector& rhs) {}

As I'm sure you are aware, this function does nothing, and it also leaves cs uninitialised. Because your class contains an instance of a CRITICAL_SECTION, you must be sure to disallow copy constructor and assignment operator calls unless you are going to implement them completely. You can do this by placing the following declarations in the private section of your class:

SharedVector(const SharedVector &);
SharedVector &operator=(const SharedVector &);

This prevents the compiler from auto-generating incorrect versions of these methods, and also prevents you from calling them in other code you write (because these are only declarations, but not definitions with {} code blocks).

Also, as Arnout mentioned, the constructor that takes a CRITICAL_SECTION& argument seems wrong. What your implementation does is copy the passed-in critical section to cs (which is not a valid thing to do with a CRITICAL_SECTION), then calls InitializeCriticalSection(&cs) which overwrites the copy you just did and creates a new critical section. To the caller who passed in a critical section, this seems to do the wrong thing by effectively ignoring whatever was passed in.

Spacing

Answer 6

so, it`s something wrong with access rights. I made size() method non-const and now it is ok.

Bellebelleek

The selected answer (use mutable) is definitely better. – Strobel