CODEWITHSUNDEEP
c++multithreadinglambda
Jakub Gruber
Jakub Gruber

Reputation: 775

How to properly pass variable to lambda function in thread

I am having leaks in this piece of code. I know that I am passing objects to vector and to lambda function incorrectly, but I am not really sure how to solve that. Could you please give me any code review and correction?

std::vector<std::thread> threads;
std::vector<std::unique_ptr<FileHandler>> fileHandlers;

for (std::string argument : filesToParse) 
{
    std::unique_ptr<FileHandler> fileHandler(new FileHandler(argument));
    fileHandlers.push_back(std::move(fileHandler));

    threads.push_back(std::thread([&fileHandler]() 
    {
        fileHandler->processFile();
    }));
}

for(auto i = 0; i < threads.size(); ++i)
{
    threads.at(i).join();
    fileHandlers.at(i)->mergeMaps(finalMap);
}

Upvotes: 0

Views: 813

Answers (1)

Sam Varshavchik
Sam Varshavchik

Reputation: 118445

There are several problems with the shown logic.

fileHandlers.push_back(std::move(fileHandler));

The contents of the fileHandler unique_ptr have been moved away here. Immediately after this:

threads.push_back(std::thread([&fileHandler]() {

This passes, to each new thread, a reference to a unique_ptr whose contents have just been moved away from. This unique_ptr is gone to meet its maker. It ceased to exist. Joined the choir invisible. It's an ex-unique_ptr.

fileHandlers.push_back(std::move(fileHandler));

Back to this statement. You get two logical errors for the price of one, here.

fileHandlers is a vector. Adding a value to a vector may reallocate the vector. Reallocation invalidates all existing iterators, pointers, or references to the existing contents of the vector. Passing a reference to something in this vector, then subsequently adding something to this vector on the next iteration of the loop will blow up in your face if the vector reallocates.

The obvious intent here is to populate the fileHandlers vector with a list of all parameters to all threads. There are two basic ways to do it correctly:

  1. Use reserve() to make sure that no reallocation subsequently occurs.

  2. Populate the vector first, with all the values, and only then spawn off all the threads, passing to each thread a reference to its own parameter in the now-complete vector.

You have several ways to fix these problems:

  1. Populate the vector first, or reserve its contents, then pass to each thread not the fileHandler unique_ptr, but a reference to the unique_ptr in the fileHandlers array that has already been populated.

  2. Alternatively, it's possible to avoid either reserving, or populating the vector in advance by switching to shared_ptrs, and capturing by value each thread's shared_ptr parameter.

Upvotes: 2

Related Questions