3

I have a class with deleted copy constructors and I'm trying to put a mutex member in something like this:

struct A {
    A(const A &other) = delete;
    A& operator=(const A &other) = delete;
    A(A&& other) = default;
    A& operator=(A &&other) = default;

    std::mutex lock;
};

The compiler is complaining that I'm trying to call the deleted copy constructor, which I summarise to be due to the std::mutex type being non-movable. How can I make the mutex member play with the move constructors with minimal fuss? I don't actually want to move the mutex member itself into the newly constructed object, and would actually like each moved object to just construct it's own mutex

Madden
  • 1,024
  • 1
  • 9
  • 27
  • 2
    This might be interesting: [SO: Move constructor for std::mutex](https://stackoverflow.com/a/7557212/7478597). – Scheff's Cat Apr 04 '19 at 13:56
  • I'm not sure whether a `default` move constructor/assignment is a good idea. Shouldn't the `lock` being responsible to guard something? Shouldn't the move construction/assigment consider that `lock` of the moved object? – Scheff's Cat Apr 04 '19 at 14:05

3 Answers3

6

I don't actually want to move the mutex member itself into the newly constructed object, and would actually like each moved object to just construct it's own mutex

Then simply define your move constructor to construct a new mutex:

struct A {
    A(const A &other) = delete;
    A& operator=(const A &other) = delete;
    A(A&& other)
        : lock()
    {
    }

    A& operator=(A &&other) = delete;

    std::mutex lock;
};

Move assignment will still be a problem and should probably just be deleted. Unless you can answer the question: what happens to the existing mutex member when you're being assigned a new value? Particularly: what if you are assigned a new value while the existing mutex is locked?

Michael Kenzel
  • 15,508
  • 2
  • 30
  • 39
  • I assume being assigned a new value should simply keep the mutex: I understand the OP's design so that the mutex protects the (other) contents of `A`, so copying/moving new contents into `this` should still `this`'s original mutex. – Angew is no longer proud of SO Apr 04 '19 at 14:01
  • 1
    @Angew I also think that that's what OP wants. However, I do see this as potentially dangerous. The fact that there is an `std::mutex` member here would seem to indicate that that mutex is supposed to protect some critical sections. It should probably not be possible to just change the value of the existing object while the mutex is currently locked… – Michael Kenzel Apr 04 '19 at 14:05
  • True indeed, that basically invalidates my answer. I will have to add a caveat. – Angew is no longer proud of SO Apr 04 '19 at 14:06
  • @Angew please do add that and keep my upvote. I do think the wrapper is a nice solution to achieve exactly what was asked for. It's just that what was asked for is potentially problematic to begin with… – Michael Kenzel Apr 04 '19 at 14:08
  • I added a discussion to my answer, I hope it covers it well enough. – Angew is no longer proud of SO Apr 04 '19 at 14:12
  • 1
    Yes, the idea was that the mutex protects other members of A, and that ultimately a new assignment should just have a new mutex member. This type will not be moved whilst the mutex is locked, although that isn't currently expressed semantically in code, but locking the mutex in the move constructors as mentioned in other answers sounds appropriate for safety – Madden Apr 04 '19 at 14:26
  • @Angew I guess it's fine. Of course, it's always a bit dangerous to acquire one lock while holding another, as that opens up potential for deadlock. Self-assignment, e.g., would seem to be an issue then. Using an `std::recursive_mutex` could help with that. Maybe you still wanna point out that that's something to be aware of with the dual lock approach… – Michael Kenzel Apr 04 '19 at 14:43
  • @MichaelKenzel `std::scoped_lock` (and `std::lock()`) at least prevent deadlock, but self-assignment is a good point; I will mention it. – Angew is no longer proud of SO Apr 04 '19 at 14:45
  • @Angew True, so that then assumes that whoever is moving already owns the lock on the object being moved from. That's probably the only way to stay sane here… – Michael Kenzel Apr 04 '19 at 14:47
  • @MichaelKenzel It actually assumes the opposite. `std::scoped_lock` is the RAII equivalent of `std::lock()` which is "lock all these lockables at once, in a deadlock-free way." – Angew is no longer proud of SO Apr 04 '19 at 14:50
  • @Angew ah, I was not aware of the deadlock avoidance algorithm in `std::scoped_lock`. That's good to know! – Michael Kenzel Apr 04 '19 at 15:02
5

As an alternative to providing custom move operations for your class, you could create a generic wrapper:

template <class T>
class PerObject
{
  T data;
public:
  PerObject() = default;
  PerObject(const PerObject&) {}
  PerObject& operator= (const PerObject&) { return *this; }
  T& operator* () const { return data; }
  T* operator-> () const { return &data; }
};

And use it like this:

struct A {
    A(const A &other) = delete;
    A& operator=(const A &other) = delete;
    A(A&& other) = default;
    A& operator=(A &&other) = default;

    PerObject<std::mutex> lock;
};

The wrapper's copy (and move) operations are no-ops, so the object containing the wrapper will always contain the one it started with.

Caveat: However, based on how your class uses the mutex, the above could actually be dangerous. If the mutex is used to protect other data within the class, then it should likely be locked while the object is being assigned to, so you will have to provide manual move operations anyway. In such case, the code would likely look something like this:

struct A {
    A(A&& other) : lock{}, /* anything else you need to move-construct */
    {
      // Note: it might even be necessary to lock `other.lock` before moving from it, depending on your class's exact semantics and expected use.
    }
    A& operator=(A &&other)
    {
      if (this == &other) return *this;  // Otherwise, double-locking would be possible

      // If you need to lock only this object:
      std::unique_lock<std::mutex> l(lock);
      // Alternatively, if you need to lock both objects:
      std::scoped_lock l(lock, other.lock);

      // Now move data from other to this

      return *this;
    }

    std::mutex lock;
};
Angew is no longer proud of SO
  • 167,307
  • 17
  • 350
  • 455
2

One way is to make your move constructor to create new mutex when called.

 A(A&& other): lock()
 {
     //... move other things
 }

You can also use a std::unique_ptr() to the std::mutex since it is movable.

struct A {
    A(const A &other) = delete;
    A& operator=(const A &other) = delete;
    A(A&& other) = default;
    A& operator=(A &&other) = default;

    std::unique_ptr<std::mutex> lock;
};

A::A() : lock(new std::mutex())

With this approach you'll not create new mutex each time you move the object which will remove some of the overhead.

Petar Velev
  • 2,305
  • 12
  • 24