mardi 5 juillet 2022

How can I implement the PIMPL idiom in a thread-safe manner?

I have a class that is implemented using the PIMPL idiom. A single instance of this class will be used across multiple threads within a multithreaded program. Inside the implementation of the class I made sure I did locking anywhere this class may read/write its own data, or use a shared resource.

#include <iostream>
#include <memory>
#include <mutex>
#include <thread>

////////////////////////////////////////////////////////////////////////////////
// Define the "Foo" class using the PIMPL idiom
class Foo final
{
public:
    Foo();

    ~Foo();

    void DoSomething(const int thread_num) const;

private:
    struct Impl;
    std::unique_ptr<Impl> m_impl;
};

////////////////////////////////////////////////////////////////////////////////
// Define the implementation of "Foo"
struct Foo::Impl final
{
    void DoSomething(const int thread_num) const {

        // Lock before accessing the shared resource of "std::cout"
        std::lock_guard<std::mutex> lock(m_mutex);
        std::cout << "Doing something from thread " << thread_num << std::endl;
    }

private:
    mutable std::mutex m_mutex;
};

////////////////////////////////////////////////////////////////////////////////
// Forward calls to the implementation
Foo::Foo()
    : m_impl(new Impl()) // Using C++11 so std::make_unique is not available
{}

Foo::~Foo() = default;

void Foo::DoSomething(const int thread_num) const
{
    // POSSIBLE LOCKING POINT A

    // Is dereferencing the "m_impl" pointer safe to do from multiple threads
    // without additional locking?
    m_impl->DoSomething(thread_num);
}

////////////////////////////////////////////////////////////////////////////////
// Use "Foo"
int main()
{
    Foo foo;

    // Is using "Foo" without locking safe here because it does a lock inside
    // DoSomething before using the shared resource "cout"?
    std::thread t1([&] {

        // POSSIBLE LOCKING POINT B

        foo.DoSomething(1);
    });

    std::thread t2([&] {

        // POSSIBLE LOCKING POINT B

        foo.DoSomething(2);
    });

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

The question is... is this enough to be thread-safe? Do I also need to lock before dereferencing m_impl (POSSIBLE LOCKING POINT A), or is that well defined? To take it even further do I need to lock before even accessing foo at all between multiple threads (POSSIBLE LOCKING POINT B), or is that well defined?

My code seems to run consistently without issues the way it is locked above, but I just want to verify that I don't have any undefined behavior.

If the locking in the above example is not sufficient, then what is the correct way to lock in order to use a PIMPL class in a thread-safe manner?

Aucun commentaire:

Enregistrer un commentaire