Class that provides thread-safe access to data of different types (Proposal, Code review)

Question

Still on a beginner level, I am currently writing a multithreaded application for raspi 4 in C++ that performs a bunch of operations on frames coming from a time-of-flight depth camera.

Situation

My threads, as well as callbacks from the depth-camera-library, produce various kinds of data (ranging from bools to more complex types like opencv mats, etc.). I want to collect some relevant data at a single place and then send it to a smartphone monitoring-app from time to time and via UDP, allowing me to monitor the behaviour of the threads...

I have no control over when a thread accesses parts of the data nor can I guarantee, that they aren't accessing it concurrently. I, therefore, searched for a way that enables me to write and read data into a struct without having to worry about thread-safeness at all. But I couldn't find a good solution for my needs so far.

"Don't use globals"
I know that this is a global-like concept and that it should be avoided, if possible. As this is some kind of logging/monitoring, I would consider it as a cross-cutting-concern and manage it this way...

Code / Proposal

So I came up with this one, which I would be very happy to see reviewed by a concurrency expert:

You can also run the code here online!

#include <chrono>
#include <iostream>
#include <mutex>
#include <thread>

// Class that provides a thread-safe / protected data struct -> "ProtData"
class ProtData {
 private:
  // Struct to store data.
  // Core concern: How can I access this in a thread-safe manner?
  struct Data {
    int testInt;
    bool testBool;
    // OpenCV::Mat (CV_8UC1)
    // ... and a lot more types
  };
  Data _data;         // Here my data gets stored
  std::mutex _mutex;  // private mutex to achieve protection

  // As long it is in scope this protecting wrapper keeps the mutex locked
  // and provides a public way to access the data structure
  class ProtectingWrapper {
   public:
    ProtectingWrapper(Data& data, std::mutex& mutex)
        : data(data), _lock(mutex) {}
    Data& data;
    std::unique_lock<std::mutex> _lock;
  };

 public:
  // public function to return an instance of this protecting wrapper
  ProtectingWrapper getAccess();
};

// public function to return an instance of this protecting wrapper
ProtData::ProtectingWrapper ProtData::getAccess() {
  return ProtectingWrapper(_data, _mutex);
}

// Thread Function:
// access member of given ProtData after given time in a thread-safe manner
void waitAndEditStruct(ProtData* pd, int waitingDur, int val) {
  std::cout << "Start thread and wait\n";

  // wait some time
  std::this_thread::sleep_for(std::chrono::milliseconds(waitingDur));
  // thread-safely access testInt by calling getAccess()
  pd->getAccess().data.testInt = val;
  std::cout << "Edit has been done\n";
}

int main() {
  // Instace of protected data struct
  ProtData protData;
  // Two threads concurrently accessing testInt after 100ms
  std::thread thr1(waitAndEditStruct, &protData, 100, 50);
  std::thread thr2(waitAndEditStruct, &protData, 100, 60);
  thr1.join();
  thr2.join();

  // access and print testInt in a thread-safe manner
  std::cout << "testInt is: " << protData.getAccess().data.testInt << "\n";

  // Intended: Errors while accessing private objects:
  // std::cout << "this won't work: " << protData._data.testInt << "\n";

  // Or:
  // auto wontWork = protData.ProtectingWrapper(/*data obj*/, /*mutex obj*/);
  // std::cout << "won't work as well: " << wontWork.data.testInt << "\n";

  return 0;
}

Question

So considering this code I can now access a variable of the struct by protData.getAccess().data.testInt from everwhere.

• But is it truly thread safe?
• Would you consider the class as "good code" (performance, readability)?

I did my best to make the code understandable. If you have any question please write a comment and I will try to explain it more deeply...

Thanks in Advance

Answer 1

No this isn't thread safe. Consider:

ProtData::Data& data_ref = pd->getAccess().data;

Now I have a reference to the data and the mutex that was locked when creating a ProtectingWrapper is already unlocked, because the temporary wrapper is gone. Even a const reference would not fix that because then I could read from that reference while a different thread writes to data.

My rule of thumb is: Don't let references (whether const or not) leak out of a locked scope.

Would you consider the class as "good code" (performance, readability)?

This is very opinin-based. Though you should consider that synchronisation isnt soemthing you want to use all over the place, rather only when necessary. In your example you can modify testInt and testBool, but to do so you need to lock the same mutex twice. If you have a class with many members that require synchronisation then it can get even worse. Consider this, which is simpler and cannot be misused:

template <typename T>
struct locked_access {
    private:
        T data;
        std::mutex m;
    public:
        void set(const T& t) {
            std::unique_lock<std::mutex> lock(m);
            data = t;
        }
        T get() {
            std::unique_lock<std::mutex> lock(m);
            return data;
        }
};

However, even this I would probably not use because it does not scale. If I have a type with two locked_access members, then I am back at step 1: I want to have detailed control on whether I modify only one of them or both at once. I know it is tempting to write a thread-safe wrapper, but to my experience it just doesn't scale. Rather thread-safety needs to be baked into the design of a type.

PS: You declared Data in the private section of ProtData, but once an instance of Data is accesible via a public method, also the type is accesible. Only the name of the type is private. I should have used auto in the line above, but I prefer it that way because it makes more clear what is going on.