0

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); } 
2
  • 2
    Watch out with std::move(fileHandler). You try to access it immediately after. Try using fileHandlers.back() to get the latest file handler. Commented Jan 5, 2017 at 20:40
  • After you push back the moved handler, your fileHandler remains in unspecified state, which most likely means it does not point to anything anymore. Next, you store your lambda inside a thread in a container that persists past the function return. So, when the thread is joined, the fileHandler inside the lambda is a dangling reference to a non-existent value that had already been moved-from even in the days of yore when it did exist. Commented Jan 5, 2017 at 20:47

1 Answer 1

2

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.

Sign up to request clarification or add additional context in comments.

1 Comment

Could you please provide code example? As I am from Java-environment, I am not really sure I understood everything correctly.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.