2
\$\begingroup\$

I'm writing a multi-threaded program where some external part of the program needs access to a large piece of data that's controlled by my class. I'm trying to return a reference to this data in a thread-safe way.

class Chunk
{
public:

    Chunk(){
        chunkData = std::make_shared<vector<XMFLOAT3>>();
        chunkMutex = std::make_shared<std::mutex>();
    };

    struct LockedData
    {
        LockedData(std::vector<float> const& d, std::mutex& mtx) : data(d), lk(mtx) {};
        std::vector<XMFLOAT3> const& data;
    private:
        std::lock_guard<mutex>lk;
    };

    inline LockedData const&& RequestChunkData()const { return LockedData(*chunkData,chunkMutex); };

private:
    
    mutable std::shared_ptr<std::mutex> chunkMutex;
    std::shared_ptr<vector<float>> chunkData;

}

Is this over-complicated? will this actually protect my data, and am I using the rvalue references (&&) correctly?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to CodeReview@SE. Is this actual code from a project, yours to put under a Creative Commons licence and change? \$\endgroup\$
    – greybeard
    Commented Dec 31, 2020 at 22:35
  • \$\begingroup\$ @greybeard thanks for the welcome, this is actual code from a project I am currently writing, it is my code :) \$\endgroup\$
    – Jay
    Commented Jan 1, 2021 at 15:26

2 Answers 2

3
\$\begingroup\$

Martin York has done a fine job of reviewing the actual code, so I’ll defer to that, but I want to go a different direction on the design review.

Your design concept is unwise

Regardless of the details of how it’s actually implemented, the basic concept of your design is not a good one.

If you have only a single chunk in your entire program… and there are no other synchronization objects at all in the entire program… then you’re fine.

However….

The moment you add even a single other synchronization object, you’re playing with fire.

For example, imagine you have just two chunks in the entire program:

auto chunk_1 = Chunk{};
auto chunk_2 = Chunk{};

// thread 1
{
    auto data_1 = chunk_1.RequestChunkData();
    auto data_2 = chunk_2.RequestChunkData();
}

// thread 2
{
    auto data_2 = chunk_2.RequestChunkData();
    auto data_1 = chunk_1.RequestChunkData();
}

Boom. Deadlock.

And this could happen with any additional lock in the program, and it won’t always be this obvious:

auto chunk = Chunk{};

// thread 1: work with data, then print
{
    auto data = chunk.RequestChunkData();

    // do stuff with data

    // now print
    auto cout_lock = std::scoped_lock(cout_mutex);
    std::cout << data;
}

// thread 2: just printing data
{
    auto cout_lock = std::scoped_lock(cout_mutex);
    std::cout << chunk.RequestChunkData();
}

Boom. Deadlock. (In case it’s not obvious, thread 1 locks the chunk mutex then the cout mutex, while thread 2 locks the cout mutex, then the chunk mutex.)

The problem is that with your design, it is functionally impossible to write a program that is guaranteed deadlock-free unless you’re really, REALLY careful. Hiding the mutex isn’t doing anyone any favours. If the chunk mutex were available, you could write safe code:

auto chunk_1 = Chunk{};
auto chunk_2 = Chunk{};

// thread 1
{
    auto lock = std::scoped_lock(chunk_1.mutex, chunk_2.mutex);

    // safe to work with both chunk 1 and chunk 2
}

// thread 2
{
    auto lock = std::scoped_lock(chunk_2.mutex, chunk_1.mutex);

    // safe to work with both chunk 1 and chunk 2
}

I understand the impulse to hide the synchronization stuff so that code looks single-threaded… but I don’t think that’s wise in the general case, for several reasons. One, expanded below, is that I don’t believe that concurrent programming has evolved to the point that there’s a one-size-fits-all solution (and may never get to that point). Another reason is that it takes away power from client code. For example, if you expose the mutex, you can use try_lock() to make an attempt to grab the data, and if it’s not available, do something else instead. (You can also do multi-locking, as illustrated in the code block above.)

So, if this design concept is wrong, what is the right design?

Well….

There is no one-size-fits-all solution

So, your design uses just a mutex. In his answer, Martin York’s (first) solution adds a condition variable. Question: Is he right? Would the design be better with a condition variable?

Answer: 🤷🏼

Some areas of computer science are more-or-less “solved”, and we have a single one-size-fits-all solution that we can apply automatically, by-default, in virtually all cases. Like, I remember the bad old days where some programming languages differentiated between “functions” (that return values) and “procedures” (that don’t return values)… now most languages just have functions, and the concept of returning “nothing”, or void. (There are some languages that seem determined to regress toward the primordial ooze, though, like CMake. But don’t get me started on CMake.)

Concurrent programming is a LOT better than it used to be ~20-25 years ago—it’s no longer the wild west—but it’s still hardly settled science. There is no single, elegant solution that works well in virtually all cases.

For example: adding a condition variable into the mix would make sense if you expect to be spending long periods waiting on the data to become available. A condition variable sorta… “turns off” a thread completely, making it stop wasting CPU resources until you notify it (or it gets spuriously notified). Sounds great… but that “turning off” isn’t free (nor is the waking up). That’s multiple dives into kernel space. If you expect to be waiting for multiple time slices, then sure, that cost is amortized away.

But on the other hand, if you don’t expect to be waiting long periods, then a condition variable is overkill. Indeed, a mutex might even be overkill. If you expect very low contention, and wait times that are on the order of a single thread time slice or less, then you could probably get away with just an atomic flag:

auto _data = std::vector<XMFLOAT3>{};
auto _flag = std::atomic<bool>{};

// thread 1: the basic idea
{
    // wait for data to be available...
    auto available = false;
    while (not _flag.compare_exchange_strong(available, true)) // (1)
        // data not available, so just give up our time slice then try again later.
        std::this_thread::yield();

    // (1): could also use:
    // _flag.compare_exchange_strong(available, true, std::memory_order_release, std::memory_order_relaxed)

    // _data is now safe to use.

    // we're done, so:
    _flag = false; // (2)

    // (2): could also use:
    // _flag.store(false, std::memory_order_release);
}

// thread 2: using RAII
class chunk_lock
{
    std::atomic<bool>* _flag = nullptr;

public:
    chunk_lock(std::atomic<bool>& flag) : _flag{&flag}
    {
        auto available = false;
        while (not _flag->compare_exchange_strong(available, true, std::memory_order_release, std::memory_order_relaxed))
            std::this_thread::yield();
    }

    ~chunk_lock()
    {
        _flag->store(false, std::memory_order_release);
    }

    // no copy/move
    chunk_lock(chunk_lock const&) = delete;
    chunk_lock(chunk_lock&&) = delete;

    auto operator=(chunk_lock const&) -> chunk_lock& = delete;
    auto operator=(chunk_lock&&) -> chunk_lock& = delete;
};

{
    auto lock = chunk_lock(_flag);

    // _data is now safe to use.

    // auto unlock
}

In C++20, atomics now have wait/notify semantics, too:

auto _data = std::vector<XMFLOAT3>{};
auto _flag = std::atomic<bool>{};

// thread
{
    _flag.wait(false /*, std::memory_order::acquire */);

    // _data is now safe to use.

    _flag.store(false /*, std::memory_order::release */);
    _flag.notify_one();
}

// and of course, you could use an RAII object to do the store and notify

That would probably be the best solution if you expect very little contention for the data, and very short lock times when there is contention.

But there are still other patterns that might be better still. For example, if you are expecting lots and lots of contention, but most of that will be from threads that just want to read the data, while there will only be a tiny few actually modifying it, then you might consider using a shared mutex:

auto _data = std::vector<XMFLOAT3>{};
auto _mutex = std::shared_mutex{};

// read-only thread
{
    auto lock = std::shared_lock(_mutex);

    // this thread now has SHARED access to _data, which it may share with
    // other read-only threads.
    // _data is safe to READ, but not modify.

    // auto unlock
}

// modifying thread
{
    auto lock = std::scoped_lock(_mutex);

    // this thread now has EXCLUSIVE access to _data.
    // _data is safe to read/modify.

    // auto unlock
}

That allows multiple threads to read the data at the same time… but if a thread needs to modify the data, it will get exclusive access. There is no blocking or waiting unless someone actually wants to change the data, which is presumably rare.

A variant of the shared_mutex without any mutexes is to use an atomic shared pointer to const data. This requires C++20, but it looks like:

auto p_data = std::atomic<std::shared_ptr<std::vector<XMFLOAT3> const>>{};

// create the chunk data somehow:
p_data = std::make_shared<std::vector<XMFLOAT3>>();

// read-only thread
{
    auto const p = p_data.load(/* std::memory_order::acquire */); // or maybe even relaxed

    // data in *p is now safe to read
}

// modify thread
{
    auto new_data = std::shared_ptr<std::vector<XMLFLOAT3> const>{};

    if (a_copy_of_the_existing_data_is_needed)
        new_data = p_data.load(/* std::memory_order::acquire */); // or maybe even relaxed

    // fill new_data with new data somehow, then

    p_data.store(new_data /*, std::memory_order::release */); // or relaxed
}

(You could do this without atomic<shared_ptr> pre-C++20, but it’ll be harder because you’ll have to manually maintain ref counts.)

The benefit of doing it this way is that there are (functionally) no locks, no blocking, and no waiting. Every thread will be busy, pretty much 100% of the time. The downside is that some of those threads will be using stale data. Thing is, if you’re allowing the data to be modified at arbitrary points, then that’s more-or-less unavoidable, even if you’re using a shared_mutex. The difference is that with shared_mutex, when the data does need to be modified, everything grinds to a halt until the data is modified, then everything starts up again with the modified data, while with the shared pointer design, nothing ever really stops (so some threads will be working with stale data until the most recent modifications propagate through the system).

None of the solutions I’ve shown is the “right” solution for all cases. Which one is best for you depends entirely on your situation, which you didn’t describe, so no one can possibly give you the best answer.

A better design concept

The primary problem with your existing design concept is that it acquires a (hidden) lock, and then lets that lock spill out of its interface. This is why it has deadlock issues; when you do:

auto chunk = Chunk{};
auto data = chunk.RequestChunkData();

// !!! THERE IS A LOCK HELD HERE !!!

// other code... that may try to obtain other locks or do other stuff that
// might lead to a deadlock

One way to “fix” this problem is to say that if the mutex is an implementation detail of the type, so too should be any locking. Locking should not be visible from outside the interface.

That would mean that any operations you may want to do on a chunk should be class member functions:

class Chunk
{
    std::vector<XMFLOAT3> _data;
    std::mutex _mutex;

public:
    Chunk()
    {
        // initialize somehow
    }

    auto transmogrify()
    {
        auto lock = std::scoped_lock(_mutex);

        // transmogrify _data here
    }

    auto frobnicate()
    {
        auto lock = std::scoped_lock(_mutex);

        // frobnicate _data here
    }

    auto swizzle()
    {
        auto lock = std::scoped_lock(_mutex);

        // swizzle _data here
    }

    // for when you actually want to get the raw data itself:
    auto get_data() const
    {
        auto lock = std::scoped_lock(_mutex);

        // copy the data, and return it
        auto res = _data;
        return res;
    }
};

This is safe because you know that within each member function, no other locking or synchronization is being done. There’s no chance of deadlock.

The downside is that you either have to implement every imaginable algorithm you may conceivably want to use as a member function of the class, OR accept that there will be possibly undesirable locking and unlocking—and the data may potentially change—if you build larger algorithms out of multiple member function calls.

Martin York suggests a much more flexible solution, roughly:

class Chunk
{
    std::vector<XMFLOAT3> _data;
    std::mutex _mutex;

public:
    Chunk()
    {
        // initialize somehow
    }

    template <typename Func>
    auto access(Func&& func)
    {
        auto lock = std::scoped_lock(_mutex);

        return func(_data);
    }

    template <typename Func>
    auto access(Func&& func) const
    {
        auto lock = std::scoped_lock(_mutex);

        return func(_data);
    }
};

But… eeeeeehhhh… not really keen on this. This is the moral equivalent of using const_cast to cast away const. Sure, it’ll “work” if you’re disciplined and careful. But if you’re allowing arbitrary functions WITHIN YOUR PRIVATE LOCK… you’re just in potential deadlock territory all over again. Frankly, I wouldn’t approve this design in the general case.

If I were going to do this, I wouldn’t name the function with something as benign as access. I would give it a name with “lock” in there somewhere, to make it clear what kind of dangerous territory you’re in; something like with_data_locked() or lock_data_and_then().

There may be a better answer, one that might sound a little crazy:

You might not need synchronization at all

The only time you need to do any synchronization at all is when you have data that is:

  • shared; and
  • mutable

Your data is shared (presumably). However… is it mutable?

As your code is written, the answer is no. Your LockedData trucks the data around as a const reference. That makes all the difference.

If the data is const, then it cannot be modified. So there’s no need to synchronize access. That means the entirety of the LockedData structure, the mutex, everything, is all pointless.

This is not a rare situation at all! In fact, it’s quite common. It’s a very common use pattern to have one thread that loads/generates a big amount of data, then have multiple threads do stuff with it. For example, you’d load an image from disk in a single thread, but then once it’s loaded, you can often break it into subsections and process those subsections independently in multiple threads. In situations like that, you don’t need any synchronization; you just load the data into an immutable object (a const object, or a class that has only const member functions) and then… you’re done. You can safely share that among multiple threads.

For example:

auto chunk = std::vector<XMFLOAT3>{};

// initialize chunk with its data here

// now chunk is ready to use, so create some threads to do stuff with the data:

auto func1(std::vector<XMFLOAT3> const&) -> void;
auto func2(std::vector<XMFLOAT3> const&) -> void;

auto t1 = std::thread{func1, std::cref(chunk)};
auto t2 = std::thread{func2, std::cref(chunk)};

// and that's that

t1.join();
t2.join();

That’s it; not a synchronization primitive in sight.

If for some reason you can’t start the processing threads after the load/generate thread (like, all threads have to be started at the same time for some reason), then you need some sort of synchronization signal so the processing threads know when the data is ready. For example you could wait on an atomic flag (in C++20), or a condition variable, or a shared future. Once a processing thread has the green light, though, no further synchronization is necessary.

Summary

Given that you haven’t really given any information about your project, how these “chunks” are going to be used, the amount and type contention you expect for the shared data, or even if the data really needs to be shared at all, you’re not going to get any useful, concrete advice from any code review. Reviewers are just going to have to make blind guesses, and maybe one of them will get lucky and guess exactly your situation… but likely not.

If I base my understanding of how you’re going to use this chunk data based only on the code you’ve given, then I’d say yes, your code is WILDLY over-complicated. ALL of the code you wrote can essentially be replaced with this:

using Chunk = std::vector<XMFLOAT3> const;

That will make Chunk a thread-safe type that will allow you to share chunk data across multiple threads.

If, on the other hand, there’s more to your code than you’ve let on, and you need to allow concurrent read AND WRITE access, then maybe the best solution is a shared_mutex. Unless you’re not expecting much contention, in which case you might be able to get away with just an atomic flag.

One thing you need to be cautious about are designs that allow locks to leach out of an interface, or that allow the execution of arbitrary code while a lock is being held. That is the key problem with your current design: a lock is still being held when the LockedData constructor returns, which you may say, “yeah, duh, that’s the point”… but which I say, yeah, that’s the problem. Anything may happen between that constructor and the destructor (which releases the lock), which is why you have a deadlock problem.

You may object and say that your LockedData just like std::lock_guard, and if std::lock_guard is fine, why isn’t LockedData. The answer to that is: std::lock_guard isn’t “fine”. It’s “okay” in simple cases. But there’s a reason C++17 added std::scoped_lock, and you shouldn’t use std::lock_guard as of C++17✳, but rather prefer std::scoped_lock instead (unless you need to use std::unique_lock for special use cases, like dealing with condition variables). And LockedData is not like std::scoped_lock. (Though I suppose you could make it more like std::scoped_lock… but that would only be a partial solution, because it would allow locking multiple locks, but still wouldn’t give you the ability to try, adopt, or defer or anything else.) But anyway, even if it were exactly like std::scoped_lock, std::scoped_lock is still dangerous; we just accept the risks because they’re unavoidable, and balance that by being cautious whenever we see any locks being acquired. Hiding locks makes everything more dangerous; at least when it’s openly visible, programmers know to be cautious around std::scoped_locks.

✳ (This is a rule of thumb, not a rule. Obviously if std::lock_guard works safely, then fine, leave it… but the default choice as of C++17 should be std::scoped_lock. It works everywhere std::lock_guard does, and in places std::lock_guard cannot work.)

Now, you may decide to just go ahead with your design anyway, accept the limitations, and rely on programmer discipline to avoid the pitfalls. That’s not a “wrong” solution, and with some tweaks to the interface it might even be practical. At some point, you have to rely on disciplined programmers anyway; compilers just aren’t smart enough to detect poorly written concurrency code, and even sanitizers are’t perfect at detecting data races and deadlocks. So it’s not “wrong” to just say: “Fuck it, I know this design has deadlock risks, so I’ll just make sure all code that uses it does so carefully.” In that case… I guess the best option is the Martin York option 2 design (with a better name than access(), like while_locked() or something, and maybe an overload that allows locking multiple mutexes).

Concurrency programming is still in its infancy, and we don’t have perfect, generic, one-size-fits-all solutions yet. No one can give you the “right” answer for your situation without knowing it in detail. I hope I’ve given you a comprehensive set of options, with the pros and cons of each. But ultimately, you’ll have to consider your situation, and make the call.

\$\endgroup\$
0
1
\$\begingroup\$

Overview

This is not a good solution.

You are basically setting up a busy wait on the mutex.

You should be using a condition variable to give the OS the opportunity to reassign the processor to do other work.

Returning the LockedData is just making this much more complex than it needs to be. Use a get/release set of methods (but make them private) then use RAII to call get/release so that it is done correctly.

Code Review

class Chunk
{
    // I put private member variables at the top.
    // When writting the constructor I want to double check that
    // I have correctly set up all members.
    // I put all private methods at the bottom.
    // Not everybody agrees with me on this. But its what I like. :-)
    private:            
        mutable std::shared_ptr<std::mutex> chunkMutex;
        std::shared_ptr<vector<float>> chunkData;

    public:
    
        // Use the initializer list to set up members
        Chunk(){
            // I don't think these need to be shared dynamically like this.
            // Just make them members of chunk.
            chunkData = std::make_shared<vector<XMFLOAT3>>();
            chunkMutex = std::make_shared<std::mutex>();
        };


    
    
        // don't use `inline` here (it means nothing).
        // Dont add && on the return type it does nothing.
        inline LockedData const&& RequestChunkData()const { return LockedData(*chunkData,chunkMutex); };
    
    
};

How I would do it:

class ChunkAccess;
class Chunk
{
    private:
        
        std::mutex              chunkMutex;
        std::condition_variable chunkCondition;
        std::vector<float>      chunkData;
        bool                    inUse;      

    public:
        Chunk()
            : inUse(false)
        {}
    private:
        friend class ChunkAccess;
        void getUniqueAccesses()
        {
            std::lock_guard<mutex>  lock(chunkMutex);
            chunkCondition.wait(lock, [&inUse](){return !inUse;});
            inUse = true;
        }
        void releaseUniqueAccesses()
        {
            std::lock_guard<mutex>  lock(chunkMutex);
            inUse = false;
            chunkCondition.notify_one();
        }
};
class ChunkAccess
{
    private:
        Chunk&  chunk;
    public:
        ChunkAccess(Chunk& chunk)
            : chunk(chunk)
        {
            chunk.getUniqueAccesses();
        }
        ~ChunkAccess()
        {
            chunk.releaseUniqueAccesses();
        }
        // Don't want multiple access objects so prevent copy.
        ChunkAccess(ChunkAccess const&)           = delete;
        ChunkAccess operator=(ChunkAccess const&) = delete;
        // With a bit of tinkering you can make this moveable.

        // Simplify data accesses
        float&       operator[](std::size_t index)       {return chunk.chunkData[index];}
        float const& operator[](std::size_t index) const {return chunk.chunkData[index];}
};

int main()
{
    Chunk         data;
    ChunkAccess   dataAccess(data);

    std::cout << dataAccess[3] << "\n";
}

How I could do (Option 2)

class Chunk
{
    private:
        
        std::mutex              chunkMutex;
        std::vector<float>      chunkData;

    public:
        Chunk()
            : inUse(false)
        {}

        void access(std::function<void(std::vector<float>&)> const& action)
        {
            std::lock_guard<mutex>  lock(chunkMutex);
            action(chunkData);
        }
};

int main()
{
    Chunk         data;
    data.access([](std::vector<float>& value){
        std::cout << value[3] << "\n";
    });
}
\$\endgroup\$
3
  • \$\begingroup\$ There is no busy wait in OP's code. Trying to take a mutex that is already locked will cause the current thread to sleep until the mutex is released. Maybe using a std::shared_lock would be best here. \$\endgroup\$
    – G. Sliepen
    Commented Dec 31, 2020 at 21:36
  • \$\begingroup\$ @G.Sliepen Removed busy wait statement. There does not seem like a shared lock situation. The OP seems to want exclusive ownership by a thread, which means condition variable is required (unless you want to hold the lock constantly until all your work is done). I suppose we could use an exclusive lock and pass a functor to do the work required. \$\endgroup\$ Commented Dec 31, 2020 at 23:04
  • \$\begingroup\$ There's nothing wrong per se about keeping a lock until all work is done. But I mentioned the shared lock because RequestChunkData() is a const function, and the resulting LockedData points to a const vector, so in principle multiple users could have read access simultaneously. \$\endgroup\$
    – G. Sliepen
    Commented Dec 31, 2020 at 23:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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