5
\$\begingroup\$

I'm trying to implement a blocking counter that may have several readers and writers at the same time:

class BlockingCounter {
public:
    BlockingCounter(int count) : count_(count) {
        // init lock_;
    }
    ~BlockingCounter() {
        // destroy lock_;
    }

    void Increment() {
        pthread_rwlock_wrlock(&lock_);
        count_++;
        pthread_rwlock_unlock(&lock_);

    }

    void Decrement() {
        pthread_rwlock_wrlock(&lock_);
        count_--;
        pthread_rwlock_unlock(&lock_);

    }

    int GetCount() {
        pthread_rwlock_rdlock(&lock_);
        int count = count_;
        pthread_rwlock_unlock(&lock_);
        return count;
    }

private:
    pthread_rwlock_t lock_;
    int count_;

};

Does this code have problems under multi-threading circumstances?

I intended to use it like this:

Main thread:

BlockingCounter *counter = new BlockingCounter(10);

// Here 10 threads are started. See code below.
while (counter->GetCount() > 0) {
  ConditionVar.wait();
}
// Main thread resumes execution.

Other 10 thread(s):

// Do stuff
counter.Decrement();
ConditionVar.notify();
\$\endgroup\$
  • 4
    \$\begingroup\$ If you use a C++11 std::atomic, you won't need all that. \$\endgroup\$ – glampert Dec 22 '14 at 17:07
  • 2
    \$\begingroup\$ Yep all I see is a bunh of C code that happens to be wrapped in a class. C++ has language specific features to deal with locks, threads and maintaining consistent access to memory. You should learn those. \$\endgroup\$ – Martin York Dec 22 '14 at 17:12
  • \$\begingroup\$ What is the purpose of the class? How can it be used? It all looks like educational attempt to use rwlock somehow, but lacks real usefulness. \$\endgroup\$ – firda Dec 22 '14 at 18:32
4
\$\begingroup\$

You are really writing C

C++ has its own language specific features to handle multi threading.

Your interface is broken.

The only way to test the value of the counter is via GetCount() but by the time you can use the value the counter may have changed.

 Thread1                      Thread2                    Thread3
 if (b.GetCount() == 0) {     if (b.GetCount() == 0) {   if (b.GetCount() == 0) {
     b.Increment();                b.Increment();             b.Increment();
     // I have the lock.
     Do stuff                      Do stuff                   Do stuff
 }                            }                          }

As you can see your mechanism does not stop all three threads from doing something. The only real use for the counter is to print the count (and even that may not be accurate but at least it wont break anything).

\$\endgroup\$
  • \$\begingroup\$ What if each thread has either one reader or one writer? eg. some threads write (Increment()/Decrement()) but never read(GetCount()), while others read but never write. \$\endgroup\$ – Lion Dec 23 '14 at 6:13
  • \$\begingroup\$ @Lion: So what is the use of GetCount(). You are guaranteed at the point you call GetCount() that it is correct but once you call unlock that guarantee is gone. So by the time the caller of GetCount() gets the value there are no guarantees about its value. So the question becomes what are you using it for? Provide a mechanism for the GetCount() to use the the value while it still holds the lock (some type of functor). Only release the lock when you have used the value. \$\endgroup\$ – Martin York Dec 23 '14 at 6:58

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.