Win7 atomic access class

Question

I have a simple atomic access class designed to work with Windows threads:

template <class aa_type>
class AtomicAccess{
private:
    CRITICAL_SECTION lock;
    aa_type value;
public:
    AtomicAccess(){
        InitializeCriticalSection(&lock);
    }

    ~AtomicAccess(){
        DeleteCriticalSection(&lock);
    }

    AtomicAccess &operator=(const aa_type &val){
        EnterCriticalSection(&lock);
        value = val;
        LeaveCriticalSection(&lock);
        return *this;
    }

    AtomicAccess &operator++(int){
        EnterCriticalSection(&lock);
        value++;
        LeaveCriticalSection(&lock);
        return *this;
    }

    AtomicAccess &operator--(int){
        EnterCriticalSection(&lock);
        value--;
        LeaveCriticalSection(&lock);
        return *this;
    }

    bool operator==(const aa_type &val){
        aa_type current;
        EnterCriticalSection(&lock);
        current = value;
        LeaveCriticalSection(&lock);
        return current == val;
    }

    bool operator!=(const aa_type &val){
        aa_type current;
        EnterCriticalSection(&lock);
        current = value;
        LeaveCriticalSection(&lock);
        return current != val;
    }
};

typedef AtomicAccess<int> int_aa;

Right now I am using it to only atomic-access integers, and the only operators I need are = (for assignment), ++, --, == and !=. If I decide I need more operators, I will add them.

Testing race conditions is very tricky, so I would like to ask here if my approach will work or if I am missing something.

Answer 1

Konrad's answer has a big advantage: it is safe! In your code sample, if an operator raise an exception before you leave the critical section, this one will never be released. In Konrad's answer, not matter what, the critical section will be unlocked when you exit the scope, even if you don't catch an exception.

Answer 2

I'm pretty bad at concurrency, and by no means a C++ expert, but this question has gotten surprisingly little attention, so I'll take a stab at it. Note that I've focused mostly on coding style rather than thread safety. Once I've had some more time to think about the code, I'll edit and post more about that.

---

operator=(AtomicAccess)

The following will evoke undefined behavior:

int_aa x;
int_aa y;
x = y;

The reason why is that CRITICAL_SECTION is not movable or assignable. (It technically is, but not safely.) The default operator= will copy the CRITICAL_SECTION.

---

Constructors

int_aa x = 5;

Interestingly, MSVC++ 2010 will not consider this valid. I would have expected it to default-construct x then use x.operator=(5). Apparently my C++ knowledge is worse than I thought though :).

Anyway, I consider it bad design to have an object in an unknown state.

int_aa x;

Is in an unknown state as the value of the int inside of x is unknown. I would have the default constructor default construct value, and I would also provide a constructor to set the value:

AtomicAccess(aa_value& val = aa_value()) : value(val)
{
    InitializeCriticalSection(&lock);
}

This could arguably be written with a lock around the assignment, however, I do not think that a different thread should be touching the object while it's being constructed anyway.

I might also provide a constructor to copy from a different AtomicAccess:

AtomicAccess(const AtomicAccess& aa)
{
    InitializeCriticalSection(&lock);
}

This one will definitely require locking.

---

Unnecessary copy in operator== and operator!=

Instead of storing a temporary copy of value, you can store a temporary bool:

bool operator==(const aa_type &val){
    EnterCriticalSection(&lock);
    bool equals = (this->value == val);
    LeaveCriticalSection(&lock);
    return equals;
}

---

operator!= could be implemented as !operator==

Unless there's some compelling reason to implement them separately (which there almost never is), I would implement operator!= as the opposite of operator==:

bool operator!=(const aa_type &val)
{
    return !(operator==(val));
}

This will of course always call aa_type::operator== instead of aa_type::operator!=, but this shouldn't matter.

Answer 3

In addition to Corbin’s remarks (in particular the bug relating to copying!), the one thing that immediately jumps at me when looking at the code is the code duplication:

Basically, every function of the class has the pattern

EnterCriticalSection(&lock);
some_action
LeaveCriticalSection(&lock);

This can be reduced by the clever use of RAII:

scoped_lock sl(lock);
some_action

… where scoped_lock is a simple scoped guard implementation that acquires the lock in its constructor and releases it in its destructor.

The implementation is straightforward (but could be made even cleaner by using a std::unique_ptr in C++11):

class scoped_lock {
    CRITICAL_SECTION& lock;
    scoped_lock(scoped_lock const&); // = delete

public:

    explicit scoped_lock(CRITICAL_SECTION& lock) : lock(lock) {
        EnterCriticalSection(&lock);
    }

    ~scoped_lock() {
        LeaveCriticalSection(&lock);
    }
};

And yes, taken together this is still more code than your version but it importantly removes code duplication and that is more important than raw line count.