CODEWITHSUNDEEP
c++multithreadingc++11thread-safetymutex
Bruice
Bruice

Reputation: 663

Check resource before and after the lock

I've run into code that simplified looks like this

inline someClass* otherClass::getSomeClass()
{
    if (m_someClass)
        return m_someClass.get();

    std::unique_lock<std::shared_mutex> lock(m_lock);
    if (m_someClass)
        return m_someClass.get();

    m_someClass= std::make_unique<someClass>(this);
    return m_someClass.get();
}

So it seems it's a pattern to be sure thread safety of creation of someClass object. I don't have much experience in multithreading, but this code doesn't look nice to me. Is there some other way to rewrite this or it's a way it should be?

Upvotes: 0

Views: 128

Answers (1)

Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275800

The biggest problem here is that you are violating the C++ memory model. In the C++ memory model, a write operation and a read operation to the same data must be synchronized.

The m_someClass at the front is reading what is written to in the mutex.

It is possible that the operator bool on m_someClass is atomic somehow.

Also, your code doesn't handle the object ever being destroyed.

If it is atomic, then you should possibly be using atomic operations to update it and not a lock. Such a pattern can result in "wasted" objects being created; often this is worth the cost of removing the lock.

make m_someClass be std::atomic<std::shared_ptr<someClass>>.

Return std::shared_ptr<someClass> from getSomeClass.

auto existing = m_someClass.load();
if (existing)
  return existing;

auto created = std::make_shared<someClass>(this);
if (
  m_someClass.compare_exchange_strong(existing, created)
) {
  return created;
} else {
  return existing;
}

Two threads can both create a new someClass if they both try to get at the same time, but only one will persist, the other will be discarded, and the function will return the one that persists.

Upvotes: 1

Related Questions