Attempting to reference a deleted function, structure with mutex member

Question

Here's my problem.

I have a structure like this.

struct threadInfo
{
    std::condition_variable cv;
    std::mutex m;
    int priorityLevel;
};

When building my code I get this error

Error C2280 threadInfo::threadInfo(const threadInfo &): attempting to reference a deleted function PriorityListMutex

From my understanding it means that the constructor for threadInfo is called and it tries to copy the mutex which is not possible.

I don't have much experience with c++ and even though I somewhat understand what is happening, I'm not sure how to attempt to fix this. Any help would be great!

Here is the code that uses ThreadInfo

    threadInfo info;
    info.priorityLevel = priority;

    priorityListMutex.lock(); 
    for (std::list<threadInfo>::iterator it = threadList.begin(); it != threadList.end(); it++) 
    {
        if ((*it).priorityLevel < info.priorityLevel)
        {
            threadList.insert(it, info); 
            break; 
        }
        else if (it == threadList.end())
        {
            threadList.push_back(info);
            break;
        }
    }
    priorityListMutex.unlock();
    std::unique_lock<std::mutex> lock(info.m);
    info.cv.wait(lock);

I guess the structure is being copied somewhere in there, but I'm completely missing where.

Don't copy the struct? (Both `condition_variable` and `mutex` have their copy ctor marked as delete) — Borgleader, Jun 13 '17 at 18:24
My bet is that the call to insert does a copy construct. Can you try using [`emplace`](http://en.cppreference.com/w/cpp/container/list/emplace) instead? (Same for `push_back`, try `emplace_back` instead) — Borgleader, Jun 13 '17 at 18:31
Yep, that's what is causing the error. It goes away when commenting out both calls. 'emplace' does not solve it though. — lhbortho, Jun 13 '17 at 18:34
Avoid locking mutex yourself. Use `unique_lock` instead. You are using one already, just in the wrong place... — François Andrieux, Jun 13 '17 at 18:35
You cant just call emplace you need to pass it some "raw arguments" otherwise it still attempts to create an object using the copy ctor. the idea with emplace is you would pass it the priority and it would create "in-place" a thread info using that instead of creating one on the stack and copy constructing the one in the list. — Borgleader, Jun 13 '17 at 18:37
Still pretty unclear to me, I've never used that. I'll try to read on it though. Thanks for the idea! — lhbortho, Jun 13 '17 at 18:39
[See here](http://coliru.stacked-crooked.com/a/2a3c67fb25d8f079) I think that should work. (I've removed some parts of your example because some variables were either not declare or werent relevant to what I wanted to show, which is emplace/emplace_back usage) — Borgleader, Jun 13 '17 at 18:39

score 1 · Accepted Answer · answered Jun 13 '17 at 18:47

1

You can solve your problem by avoiding copies and emplacing the structs directly in the list. This does require a custom constructor though. I've shortened your code sample to only show the emplacing portion:

#include <mutex>
#include <condition_variable>
#include <list>

struct threadInfo
{
    explicit threadInfo(int prio) : priorityLevel(prio) {}

    std::condition_variable cv;
    std::mutex m;
    int priorityLevel;
};

int main()
{
    std::list<threadInfo> threadList;

    int priorityLevel = 0;

    for (std::list<threadInfo>::iterator it = threadList.begin(); it != threadList.end(); it++) 
    {
        if ((*it).priorityLevel < priorityLevel)
        {
            threadList.emplace(it, priorityLevel); 
            break; 
        }
        else if (it == threadList.end())
        {
            threadList.emplace_back(priorityLevel);
            break;
        }
    }

    return 0;
}

answered Jun 13 '17 at 18:47

Borgleader

15,826
5
46
62

You can use `emplace` with `std::move`. `threadInfo` will have an implicit move constructor defined by the compiler. You can do `threadList.emplace(it, std::move(info));` and not require a dedicated constructor. – François Andrieux Jun 13 '17 at 18:49
Yep that works. Simplest answer and very easy to incorporate for other similar problems too. Thanks for showing that existed ! – lhbortho Jun 13 '17 at 18:50
@FrançoisAndrieux I guess that's just personal preference at that point right? Any reason one would be better than the other? – lhbortho Jun 13 '17 at 18:50
@lhbortho You do not need to define a whole new constructor if you use `move`. If you already have the appropriate constructor, then direct construction as suggested by this answer is probably best, it will save you a move construction. – François Andrieux Jun 13 '17 at 18:59

score 0 · Answer 2 · answered Jun 13 '17 at 18:28

In the Standard C++ Library, classes related to threading, like mutex, do not have a copy constructor.

When an assignment involves two objects, like

Class b(10);
Class a = b;

At the second line, we try to create an object initializing from another object. This makes the compiler look for a copy constructor, a constructor developed specifically for this purpose.

Since having two identical copy of a mutex is not good, the library doesn't use such method for thread related classes.

Usually compilers create default copy-constructors in case they are needed, but when a class a property of this type it cannot do so, so it gives you and error.

To solve, you will have to explicitly define a copy constructor and handle manually. Beware, you should keep in mind that things related to threads like mutex and cv should not exists in more than one copy.

It's the `threadList.insert` and `threadList.push_back` calls, that attempt to copy `threadInfo` instances. — IInspectable, Jun 13 '17 at 18:31
This answer seems incomplete. There is also the solution of not making copies using `emplace` type functions with `std::move` instead of `push_back` and `insert`. — François Andrieux, Jun 13 '17 at 18:34
I started answering before the example with the list was added. Yes, that is another solution. — bracco23, Jun 13 '17 at 18:38

score 0 · Answer 3 · answered Jun 13 '17 at 18:34

The copy constructor of mutex is deleted explicitly. You can't copy a mutex, however, if what you are doing is moving rather than copying (e.g. you won't need the old value of you threadInfo object) than you can move the mutex using std::move and writing a move constructor for your threadInfo object.

However, a move constructor can cause hard to spot bug so I wouldn't recommend that. A more straight forward way would be to wrap your "info" object in a pointer and use that. You can achieve that as such:

auto info = std::make_shared<threadInfo>{};
info->priorityLevel = priority;

priorityListMutex.lock(); 
for (std::list<std::shared_ptr<threadInfo>>::iterator it = threadList.begin(); it != threadList.end(); it++) 
{
    if ((*it).priorityLevel < info->priorityLevel)
    {
        threadList.insert(it, info); 
        break; 
    }
    else if (it == threadList.end())
    {
        threadList.push_back(info);
        break;
    }
}
priorityListMutex.unlock();
std::unique_lock<std::mutex> lock(info.m);
info->cv.wait(lock);

Do note however, that in this case I'm using a shared_ptr, which is the "easiest" way to do this since it won't break anything but probably not want you want to do, what you most likely want to do, is give the 'threadList' object ownership of your info object. in which case you would declare it as a unique_ptr:

auto info = std::make_uniq<threadInfo>{};

and move it into the threadList:

threadList.insert(it, std::move(info));

Attempting to reference a deleted function, structure with mutex member

3 Answers3