CODEWITHSUNDEEP
c++thread-safety
PaulH
PaulH

Reputation: 7873

is this synchronization object implementation thread safe?

I have am reviewing a colleague's Visual Studio 2008 C++03 application application and I've come across an implementation of a thread synchronization primitive (below).

Assuming SyncObject is implemented correctly, is the use of a boolean in the below code to know if the resource is locked or unlocked thread-safe? If no, can you walk through a "ThreadA" does this and "ThreadB" does that situation so I understand your logic?

class CMyLock
{
public:
    CMyLock(SyncObject* object) 
        : object_(object), acquired_(false) 
    { 
    };

    // return true if the resource is locked within the given timeout period
    bool Lock(DWORD dwTimeOut = INFINITE)
    {
        acquired_ = object_->Lock(dwTimeOut);
        return acquired_;
    };

    // return true if the resource is unlocked
    bool Unlock()
    {
        if (acquired_)
            acquired_ = !object_->Unlock();
        return !acquired_;
    };

    // return true if the resource is locked
    bool IsLocked() { return acquired_; };

private:
    bool acquired_;
    // some thread synchronization primitive
    SyncObject* object_;
};

Upvotes: 2

Views: 277

Answers (4)

John Dibling
John Dibling

Reputation: 101506

I see several serious problems with this code. In a code review, I would reject it.

  1. It is not clear what this class is intended to be. It might be a thin proxy to a primitive. It might be an automatic locker. In either case, the design is wrong, and the documentation (none) does not elaborate.

  2. It does not use RAII. This is a good idea no matter what this object is intended to be, but in the case of an automatic locker it is especially important.

  3. It retains it's own state, which could be out of sync with other instances in the same thread. If, for example, you create 2 instances of this object on thread A, set one to locked and check the state of the other one, it should say locked but it won't.

  4. Possibly most importantly, it is re-inventing the wheel at best.

Upvotes: 2

Tobias Langner
Tobias Langner

Reputation: 10828

no it's not - at least from what I can see. What may happen is that one thread calls lock and gets the lock and another thread accesses m_bAcquired before it's updated by the thread the causes the lock.

That's why you need a lock for reading as well asl Matthieu M. stated.

A: Lock after m_pObject is locked but before m_bAcquired is set B: IsLocked --> returns false A - still in Lock: m_pObject = true

so B has false information.

Other problem: Unlock relies on m_bAcquired.

I think this object is meant to be used from within one single thread. So each thread has its own CSingleLock instance but they all use the same SyncObject. In this case, only SyncObject needs to be thread safe and it works.

Upvotes: 1

smerlin
smerlin

Reputation: 6566

It is not threadsafe.

directly after m_pObject->Unlock() returns, another thread waiting on m_pObject->Lock(dwTimeOut) can return and set m_bAcquired to true, then the unlocking thread sets m_bAcquired to false and overwrites the locked state incorrectly (IsLocked will return false while the object is locked).

Upvotes: 2

Matthieu M.
Matthieu M.

Reputation: 300419

Short answer: no

You need to lock for reading too, or you risk seeing a stale state.

Upvotes: 1

Related Questions