4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Doing this just for simple integer ops seems like overkill. You should look into CAS/intrinsic ops (InterlockedIncrement & associated functions). \$\endgroup\$ – OJ. Aug 3 '12 at 10:18
3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Assignment operator is never invoked in an initialisation. That explains why int_aa x = 5; doesn’t work. \$\endgroup\$ – Konrad Rudolph Jun 8 '12 at 9:56
  • \$\begingroup\$ The assignment operator works for me, if I declare it as a class field and assign a value in the constructor. And I also don't want to copy Critical sections. They are meant only as a necessary evil and if I do the assignment from one AtomicAcess to another, then the inner value should be copied only. This is a good remark, thanks, even though I have no such assignments in the code. \$\endgroup\$ – Jakub Zaverka Jun 8 '12 at 15:44
  • \$\begingroup\$ And other remarks do not lack relevance as well. Thanks. Just one clarification: The way I intend to design it is only through typedefs. So if I typedef int_aa, then I want int_aa to behave in every way (or at least in ways I need) as a normal integer, but with synchronized access. The code using it should neved use the AtomicAccess class directly. \$\endgroup\$ – Jakub Zaverka Jun 8 '12 at 15:48
  • \$\begingroup\$ And I now realized I don't even have operator=(AtomicAccess), I have operator=(aa_type). (I am assigning value at initialisation and then only ++ing or --ing.) \$\endgroup\$ – Jakub Zaverka Jun 8 '12 at 16:00
  • \$\begingroup\$ @KonradRudolph That explains it then. I had always thought that converting type x = ...; to type x (...) was an optional optimization. \$\endgroup\$ – Corbin Jun 8 '12 at 22:46
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks, this definitely looks interesting. I would stick with the original version, however. Maybe it is not as elegant and quite verbose and repetitive, but I like how the code is more readable - it is immediately apparent what the code acomplishes. \$\endgroup\$ – Jakub Zaverka Jun 8 '12 at 15:54
  • \$\begingroup\$ @Konrad What are your thoughts on completely abstracting away the critical section from the class? Before I posted my answer, I thought for a while on the critical section duplicated code and couldn't decide how I felt. I thought about suggesting a second template parameter that is the locking class, then using blah.lock() and blah.unlock() to 'hide' the critical section code (and make it interchangeable with a different locking mechanism). Ultimately I could not decide if this would be worth it though, and am curious what others think. \$\endgroup\$ – Corbin Jun 8 '12 at 22:56
  • 1
    \$\begingroup\$ @Jakub My code uses an idiomatic pattern of C++, you must familiarise yourself with it, it’s a core part of C++ code bases. In idiomatic C++, my code is at least as readable as yours, arguably more so, because it uses an established pattern to express the concept of scoped lifetime. (This is in addition to what 20c has said.) \$\endgroup\$ – Konrad Rudolph Jun 9 '12 at 11:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.