trjhtr

I'm trying to find data racing in this code. I don't know why. Please, review it and let me know what can I change.

class Engine {

public:
 Engine(const unsigned int &fuel_amount, const unsigned int &time_interval) :fuel_amount(fuel_amount),
 time_interval(time_interval), disconnect(false) 
 std::thread thd( [this, fuel_amount, time_interval] 
 std::unique_lock<std::mutex> lk(m2);

 while (!fuel_tanks.empty() && !disconnect) 
 return false;
 );
 

 ~Engine() 
 std::lock_guard<std::mutex> lk(m2);
 disconnect = true;
 current_tank.notify_one();

 if (thd.joinable()) thd.join();
 

 void addTank(const std::shared_ptr<FuelTank> & fuel_tank) 
 std::lock_guard<std::mutex> lk(m2);
 fuel_tanks.emplace_back(fuel_tank);
 current_tank.notify_one();
 
//...

Here is Engine class which I use to take fuel in some time intervals using multithreading and share pointers. There also a FuelTank class, but helgrind found data racing in this class.

Bug

Well you have a bug in your code.

This is an automatic variable.

 std::thread thd( [this, fuel_amount, time_interval] /* STUFF */ );

So it goes out of scope at the end of the constructor. Which means the destructor is called. Which means that std::terminate will be called if the thread has started. If the "thread of execution" has not started (ie it is not joinable) then it is just destroyed and no "thread of execution" was ever created.

So either this will cause the application to terminate or no thread will be created.

Code Review

Lets not place a huge function in the middle of the constructor. Lambdas' are nice but there best for one liners. If you have a whole complex function run then move that code into its own method and call it.

Don't use an if statement to return true/false. Just return the expression you are using for the test.

 if (!fuel_tanks.empty()) return true;
 return false;

 // easier to read/write as

 return !fuel_tanks.empty();

Its curious how you use completely different tests here:

 current_tank.wait(lk, [this] 
 return !fuel_tanks.empty();
 );
 if (!fuel_tanks.empty() || !disconnect) {

So the conditional variable will never exit if disconnect is set to true. So to cause the worker thread to exit you need to enter the destructor then add fuel.

I think you actuall need:

 current_tank.wait(lk, [this] 
 // Exit if fuel tank is empty
 // or the disconnect has been set to true.
 return !fuel_tanks.empty() && !disconnect;
 );
 if (disconnect) 
 // If we disconnected we want the thread to exit.
 // so force the exit with a return.
 return;
 
 // You only get here if there is fuel in the tank.

Your moving is not doing what you think

void addTank(const std::shared_ptr<FuelTank> & fuel_tank) // pass by const reference.
 std::lock_guard<std::mutex> lk(m2);
 fuel_tanks.emplace_back(fuel_tank); // this is creating a copy
 // to pass as a parameter.
 current_tank.notify_one();

What it should look like:

void addTank(std::shared_ptr<FuelTank>&& fuel_tank) // pass by r-value ref
 std::lock_guard<std::mutex> lk(m2);
 fuel_tanks.push_back(std::move(fuel_tank)); // still needs to be moved
 // but you use push.
 current_tank.notify_one();