[ACCEPTED]-Problems using EnterCriticalSection-critical-section

Accepted answer
Score: 20

Just declare cs as:


or else remove the const 12 clause on size()

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

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

Score: 7

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

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

class CriticalSectionLock
        CriticalSectionLock(CRITICAL_SECTION& cs)
            : criticalSection(cs)
        CRITICAL_SECTION&  criticalSection;

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

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

Score: 3

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

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

Then in your class methods you 1 would code it as:

template <typename T>
void SharedVector::PushBack(const T& value) {
   CS_Acquire acquire(&cs);
Score: 1

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

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

Score: 1

I see you have declared an empty copy constructor:

SharedVector(const SharedVector& rhs) {}

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

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

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

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

Score: 0

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

More Related questions