I am trying to make the copy constructor of a class thread safe like this:

class Base
{
   public:
      Base ( Base const & other )
      {
         std::lock_guard<std::mutex> lock ( other.m_Mutex );
         ...
      }
   protected:
      
      std::mutex m_Mutex;
}

class Derived : public Base
{
   public:
      Derived ( Derived const & other ) : Base ( other )
      {
         std::lock_guard<std::mutex> lock ( other.m_Mutex );
         ...
      }

}

My problem is that in the derived class I need to lock the mutex before the base class constructor call in the initializer list to guarantee consistency. Any idea how I could achieve that?

Regards.

CodePudding user response：

The advice of @AnthonyWilliams is to lock the constructor as an argument of a delegated constructor, see here for the full article:

class A
{
private:
  A(const A &a, const std::lock_guard<std::mutex> &)
   : i(a.i), i_squared(a.i_squared) {}
public:
  A(const A &a) : A(a, std::lock_guard<std::mutex>(a.mtx)) {}
  ...
};

Note that you lock the other object in order to prevent it being changed by another thread during copy (construction of the object itself is safe). Moreover, the same approach is valid for a move constructor.

An application to your example could look like this (untested & uncompiled):

class Base
{
   public:
      Base(Base const& other) : Base(other, std::lock_guard<std::mutex>(other.m_Mutex)) {}   
      virtual ~Base() = default; //virtual destructor!

   protected:
      Base(Base const& other, std::lock_guard<std::mutex> const&) {}

      mutable std::mutex m_Mutex; //should be mutable in order to be lockable from const-ref
};

class Derived : public Base
{
   protected:
      Derived(Derived const& other, std::lock_guard<std::mutex> const& lock)
          : Base(other, lock)
      {}

   public:
      Derived(Derived const& other)
          : Derived(other, std::lock_guard<std::mutex>(other.m_Mutex))
      {}
};

CodePudding user response：

It's not the place of the object being constructed to lock the object it's copying from.

In your example, there is no way to guarantee m_Mutex stays locked between the end of the Base constructor and the beginning of the Derived constructor (after the initializer list). Not only that, if you write yet another class DerivedAgain that derives from Derived, you would need to handle its copy constructor too. Moreover, what about the move constructor?

Instead of locking a mutex in a constructor, lock it beforehand, construct your new object, and then unlock it. Probably the easiest way to do this is to write a "factory":

Derived Derived::make_copy() {
    std::lock_guard<std::mutex> lock(m_Mutex);
    return Derived(*from);
}

CodePudding user response：

I think about solution where I define two types of the constructor:

• Public copy ctor, which locks and unlocks
• Protected "copy" ctor, which does not lock

The drawback is that it is easy to forget to not call Base(Base const&, void*) in the Derived class. If so, that will lead to lock/unlock inconsistency.

#include <iostream>

struct Mutex
{
    void lock() { std::cout << "Lock\n"; }
    void unlock() { std::cout << "Unlock\n"; }
};

struct MutexLocker
{
    MutexLocker() = default;
    MutexLocker(Mutex& mutex) { mutex.lock(); }
};

class Base: private MutexLocker
{
public:
    Base() {}

    virtual ~Base() = default;

    Base(Base const& other)
        : Base(other, nullptr)
    {
        mutex_.unlock();
    }
    
protected:
    // Implement copy logic here
    Base(Base const& other, void* tag)
        : MutexLocker(other.mutex_)
    {
        std::cout << "Base\n";
    }

protected:
    mutable Mutex mutex_;
};

class Derived: public Base
{
public:
    Derived() = default;

    Derived(Derived const& other)
        : Derived(other, nullptr)
    {
        mutex_.unlock();
    }

protected:
    // Implement copy logic here
    Derived(Derived const& other, void* tag)
        : Base(other, tag)
    {
        std::cout << "Derived\n";
    }
};

class DerivedOfDerived: public Derived
{
public:
    DerivedOfDerived() = default;

    DerivedOfDerived(DerivedOfDerived const& other)
        : DerivedOfDerived(other, nullptr)
    {
        mutex_.unlock();
    }

protected:
    // Implement copy logic here
    DerivedOfDerived(Derived const& other, void* tag)
        : Derived(other, tag)
    {
        std::cout << "DerivedOfDerived\n";
    }
};

int main()
{
    DerivedOfDerived d;
    DerivedOfDerived d2 {d};
}