c++ generic threadsafe container

Question

Because c++ does not provide thread-safe container out-of-the-box, I am trying to create a generic thread-safe container.

In a multithreaded environment, many threads may modify my list (or any other container). I want to ensure that my list is thread-safe and only a single thread can modify my list at any given time.

My code is below. I'd like to know if it would work in a

template<typename T>
class ThreadSafeContainer {
    public:
      ThreadSafeContainer(): lck{mtx}{};
      T getContainer(){
          return container;
      }
    private:
      T container;
      std::lock_guard<std::mutex> lck;
      std::mutex mtx;
};

int main(){
    ThreadSafeContainer<std::list<int>> list;
    std::list<int> my_list = list.getContainer();
    
     // multiple threads can access my_list in a thread-safe way

    return 0;
}

To know if a piece of code works, you can run it against tests. What testing have you done to test this? Any reason you think it might not work? — JohnFilleau, Jan 10 '22 at 17:10
Why do you believe that "multiple threads can access my_list in a thread-safe way"? There's nothing in the shown code that proves this. And duplicating the entire container, in this manner, is unlikely to be very useful. The only thing that the shown code proves is that the container will always be completely empty. Can you explain, exactly, in words, how you expect the above to work? — Sam Varshavchik, Jan 10 '22 at 17:10
Look at this, you'll see that different container classes, and value types may have requirements, you can't handle generically: https://www.google.com/search?q=site%3Astackoverflow.com+%22c%2B%2B%22+thread+safe+wrapper+for+standard+containers&client=tablet-android-samsung-nf-rev1&sxsrf=AOaemvIKe0cldXeUR7OFymHyTwU7_AHSpw%3A1641831186390&ei=ElvcYa-MF5L_7_UPtLqeyAw&oq=site%3Astackoverflow.com+%22c%2B%2B%22+thread+safe+wrapper+for+standard+containers&gs_lcp=ChNtb2JpbGUtZ3dzLXdpei1zZXJwEAM6BwgAEEcQsANKBAhBGABQjw1Yz_ACYLP5AmgGcAF4AYABaIgBqiqSAQQ3Mi4zmAEAoAEByAEIwAEB&sclient=mobile-gws-wiz-serp — πάντα ῥεῖ, Jan 10 '22 at 17:14
Oh, and the shown code is also undefined behavior. Locking a mutex that has not been constructed yet does not have very good chances of succeeding. — Sam Varshavchik, Jan 10 '22 at 17:15
`getContainer` returns a copy of the container, ie its rather useless if the aim is to let multiple threads modify the same container — 463035818_is_not_an_ai, Jan 10 '22 at 17:19
usually you want to have as little shared data as possible and the sections that do work on shared data as narrow as possible while doing as much as possible while holding the lock. In other words, I never encountered a situation where I wanted a container that synchronizes every access — 463035818_is_not_an_ai, Jan 10 '22 at 17:21
`std::lock_guard` as a long-lived member is probably a sign of "you're doing it wrong". Unless `ThreadSafeContainer` is itself meant to be a short-lived RAII wrapper, but it doesn't look like it. — , Jan 10 '22 at 17:27
Just using a lock does not make code thread-safe. (analogy: If you apply a physical lock to a random part of your bicycle, that may not make it any less stealable) You have to do the right stuff with it. — , Jan 10 '22 at 17:34
you should probably make the basic functionality thread safe, not the container itself : insert, delete, get etc. — Raildex, Jan 10 '22 at 17:49
Thanks @Raildex, but there are alot of methods (may be 20+). Also, how about iterators (e.g. when using std::for_each or some kind of algorithm)? — raneshu, Jan 10 '22 at 17:59
How about something like: `std::lock_guard lockk(mtx); list.push_back(...); ...` — raneshu, Jan 10 '22 at 18:00
It should be a tip that something is wrong when you see that that class comes with both a mutex and something that locks that mutex -- they live right beside each other in the class and have the same lifetime. How would I ever get a mutex that is not locked then? It's like buying a locked car that already has a driver in it and the driver never gets out. It's about as useful as an occupied toilet. — Wyck, Jan 10 '22 at 18:23

score 3 · Answer 1 · answered Jan 10 '22 at 17:33

3

No, it won't work. Once the container is created, it is created with a mutex and that mutex is immediately locked and stays locked until the container is destroyed. No one checks that mutex. There is no locking mechanism at all. Every thread trying to access it, will access it.

You need to redefine all the access operations and lock the mutex when accessing the container.

answered Jan 10 '22 at 17:33

mmomtchev

2,497
1
8
23

>you need to redefine all the access operations and lock the mutex when accessing the container. Thanks, but that would be very hard/near-impossible. – raneshu Jan 10 '22 at 17:36
>Once the container is created, it is created with a mutex and that mutex is immediately locked and stays locked until the container is destroyed If the container is created on the stack, the lock would exists only until the container goes out of scope - which is fine. – raneshu Jan 10 '22 at 17:36
You need to acquire the lock before accessing the container and release it after the operation is finished. This is what prevents multiple threads from accessing it simultaneously. Currently you acquire the lock once, then everyone reads and writes as he wishes, creating a horrible mess, then you release the lock. – mmomtchev Jan 10 '22 at 17:39

Aykhan Hagverdili · Answer 2 · 2022-01-10T17:53:53.230

1

The standard library doesn't provide a generic thread-safe container because it doesn't make sense. Say you take a reference to one of the elements. How could the container ensure that you don't hold on to the reference?

On the other hand, if you have a specific goal, like multiple threads appending elements at the same time. Or a synchronized hash table where the elements are immutable. These can be done. The details of how would depend a lot on the task at hand.

That is to say, threading is hard and there is no silver bullet to make your code correct. Try to minimize shared state and carefully synchronize whatever is left.

edited Jan 10 '22 at 17:53

answered Jan 10 '22 at 17:47

Aykhan Hagverdili

28,141
6
41
93

thanks. Do you mean something like: ```std::lock_guard lck(mtx); list.push_back(...); ...``` – raneshu Jan 10 '22 at 17:49
1

@raneshu that would a way to do that. Another way would be to use separate lists and combine at the end. – Aykhan Hagverdili Jan 10 '22 at 17:50
basically, not to use a wrapperClass like `ThreadSafeContainer`. But instead, use std::list with a mutex; – raneshu Jan 10 '22 at 17:50
@raneshu It would still be difficult to synchronize read-write access to a generic list unless you nail down exactly what the specific task is. – Aykhan Hagverdili Jan 10 '22 at 17:51
basically, I'm using `push_back` and begin()/end() in std::for_each in `std::list` There will be multiple threads accessing the list. It's unlikely (but possible) that some two-step non-atomic code like: `if(!list.empty()){ list.pop_back()}` could result in errors/invalid data. – raneshu Jan 10 '22 at 17:57
I also need to worry about `iterators` (I'm using begin()/end() in a `std::for_each` loop. But not sure if that's an issue with mutexes. – raneshu Jan 10 '22 at 17:58
1

@raneshu as long as none of the threads keep references to the elements, it should be easy. Have a look at `shared_mutex`. Lock the mutex appropriately when you access the list in any way and unlock it as soon as possible. Use a lock guard *as a local variable* for that. – Aykhan Hagverdili Jan 10 '22 at 18:53

score 1 · Answer 3 · answered Jan 10 '22 at 18:12

1

Instead of making a datatype threadsafe you can also make the calls to it threadsafe. For example (very basic approach, executors that serialize calls to either a safe section or a seperate thread are better).

#include <mutex>
#include <type_traits>
#include <vector>

template<typename Fn>
auto call_ts(Fn fn) -> decltype(fn())
{
    static std::mutex mtx;
    std::unique_lock<std::mutex> lock(mtx);

    if constexpr (std::is_same_v<void, decltype(fn())>)
    {
        fn();
        return;
    }
    else
    {
        return fn();
    }
}

int main()
{
    std::vector<int> values;

    call_ts([&] {values.push_back(1); });
    call_ts([&] {values.push_back(2); });
    auto size = call_ts([&] { return values.size(); });
    return 0;
}

answered Jan 10 '22 at 18:12

Pepijn Kramer

9,356
2
8
19

would this work for `begin()` and `end()` as well? for example - in a `std::for_each(...)` – raneshu Jan 11 '22 at 06:07
1

Yes as long as you place that in a lambda as well. But as said this is a basic example that will give you threadsafety but will not give you the best performance. For more performance you might want to make distinctions between read/write operations (const/non-const calls). E.g. https://stackoverflow.com/questions/244316/reader-writer-locks-in-c – Pepijn Kramer Jan 11 '22 at 08:07
understood. thank you. But in your example above, shouldn't `call_ts` wrap everything - `call_ts([&]{values.push_back(1); values.push_back(2); ...values.size();})` to be meaningful? – raneshu Jan 11 '22 at 08:38
isn't `values.push_back(1);` by itself atomic? Whereas `values.push_back(1); values.push_back(2);` is not guaranteed to be in that order without a mutex. – raneshu Jan 11 '22 at 08:40
2

No library function is threadsafe (atomic) unless explicitly stated in its description. And push_back doesn't fall into that category. You only need to put two push_backs in one call if you want to see that as ONE atomic transaction on your vector, however that will block other threads somewhat longer, so it is a tradeoff. – Pepijn Kramer Jan 11 '22 at 08:45

c++ generic threadsafe container

3 Answers3