6

Hello I am trying to create a multi-thread encoder, which needs a mutex and I would like that mutex to be a member of the class. However, I keep getting: copy constructor of 'simple_encoder' is implicitly deleted because field 'm_mutex' has an inaccessible copy constructor as an error message when I compile. After a hour of so of searching I cannot find my mistake.

Please for the love of something help me, my constructor and members look like this: 

namespace master_thesis
{
class simple_encoder
{
public:

    simple_encoder(uint32_t symbols, uint32_t symbol_size, fifi::api::field field,
                   std::vector<uint8_t> data) :
        m_symbols(symbols), m_symbol_size(symbol_size),
        m_completed(0), m_field(field), m_data(data)
    {

    }
...
private:

    uint32_t m_symbols;
    uint32_t m_symbol_size;
    uint32_t m_completed; 

    fifi::api::field m_field;

    std::mutex m_mutex;

    std::vector<uint8_t> m_data;
    std::vector<std::vector<uint8_t>> m_result;

    std::vector<std::shared_ptr<kodo_rlnc::full_vector_encoder>> m_encoders;
};
}
Lars Nielsen
  • 2,005
  • 2
  • 25
  • 48

2 Answers2

19

In order to copy objects containing a mutex you have to write a custom copy constructor and copy assignment operator.

You don't need to copy the mutex to copy the object because the mutex is not part of the object's value, it is just there as a tool to protect access.

When copying, the mutexes of both objects need to be used to protect the value members during the copy.

For example:

class simple_encoder
{
public:

    simple_encoder() {}

    // copy constructor
    simple_encoder(simple_encoder const& other)
    {
        // no need to lock this objec because no other thread
        // will be using it until after construction
        // but we DO need to lock the other object
        std::unique_lock<std::mutex> lock_other(other.m_mutex);

        // safely copy the data
        m_symbols = other.m_symbols;
        m_symbol_size = other.m_symbol_size;
        m_completed = other.m_completed;

        // ... etc...
    }

    // copy assignment operator
    simple_encoder& operator=(simple_encoder const& other)
    {
        if(&other != this)
        {
            // lock both objects
            std::unique_lock<std::mutex> lock_this(m_mutex, std::defer_lock);
            std::unique_lock<std::mutex> lock_other(other.m_mutex, std::defer_lock);

            // ensure no deadlock
            std::lock(lock_this, lock_other);

            // safely copy the data
            m_symbols = other.m_symbols;
            m_symbol_size = other.m_symbol_size;
            m_completed = other.m_completed;

            // ... etc...
        }

        return *this;
    }

private:
    // value data
    uint32_t m_symbols;
    uint32_t m_symbol_size;
    uint32_t m_completed;

    // non value utility
    mutable std::mutex m_mutex; // needs to be mutable
};

Similar functions can also be written for moving the object.

If you have C++14 you can use a std::shared_timed_mutex and substitute a std::shared_lock<std::shared_timed_mutex> when locking the other object during copying which should be more efficient. You still need std::unique_lock on both when moving.

Galik
  • 47,303
  • 4
  • 80
  • 117
2

Your class may contain a mutex as a member, but then your class is automatically non-copyable, because a mutex is not copyable and the mutex is now part of your class.

So, wherever you're copying your class (in code you haven't shown us), you simply can't do that any more.

Your class is also now non-moveable for the same reason.

If this is a problem, you could hide the mutex behind something like a shared pointer. However I'd then be concerned about synchronisation of the shared pointer, so try not to…

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055