CODEWITHSUNDEEP
c++multithreading
Hector Esteban
Hector Esteban

Reputation: 1111

Asynchronous threads race condition

I have been asked to run a task with two asynchronous threads. One that is supposed to move a vehicle and the other calculates and plots the area covered by the vehicle. A simplification of the code is the following:

#include <iostream> 
#include <vector>
#include <math.h> 
#include <chrono>

using namespace std; 


class Robot {

    private:
        bool moving;
        vector<double> position;
        double area_covered;

    public:
        Robot(const vector<double> &initial_position) {
            moving = true;
            position = initial_position;
            area_covered = 0.0;
        }


        void get_area() {
            static vector<double> previous_measure = this->position; // initialized with the first position the robot is in

            while (this->moving) {
                this->area_covered += sqrt(pow(this->position[0] - previous_measure[0]) + pow(this->position[1] - previous_measure[1]));
                previous_measure = this->position; // save the new position for the next iteration
                this_thread::sleep_for(chrono::milliseconds(600)); // sleep for 600 ms
            }
        }


        void move_robot(const vector<vector<double> > &map) {
            for (int i=1; i < map.size(); i++) {
                this->position = map[i];
                this_thread::sleep_for(chrono::milliseconds(500)); // sleep for 500 ms
            }
            this->moving = false;
        }
};
  


int main () {
    vector<vector<double> > path{
      {0.0359, -0.013}, {0.0658, -0.0287},  {0.0736, -0.027}};
    
    Robot r3(path[0]);
    auto thread1 = std::async(&Robot::move_robot, &r3, path);
    auto thread2 = std::async(&Robot::get_area, &r3);
    thread1.join();
    thread2.join();
    return 0;
}

In method get_area() I am using multiple times this.position which may differ because it is changed in the other thread. I cannot block the other thread while doing get_area but I must avoid using different this.position in one loop run. The easiest solution is to create another variable to save the initial value of this.position but I would like to know from you whether there is a better C++ way to do it. It would be something like:

        void get_area() {
            static vector<double> previous_measure = this->position; // initialized with the first position the robot is in
            vector<double> auxiliar;

            while (this->moving) {
                auxiliar = this->position;
                this->area_covered += sqrt(pow(auxiliar[0] - previous_measure[0]) + pow(auxiliar[1] - previous_measure[1]));
                previous_measure = auxiliar; // save the new position for the next iteration
                this_thread::sleep_for(chrono::milliseconds(600)); // sleep for 600 ms
            }
        }

Moreover, I need to notify get_area() when the method / thread of move_robot() finishes so that in the next while iteration it quits as well. Right now I am using the attribute moving but doing so I am checking the condition before each iteration and not at the end. I could add an if at the end to check it but there should be some better approaches.

Last, I would appreciate as well your opinion on how to cleanly pass the object to the two asynchronous threads and wait for them to solve the C++ way.

Upvotes: 0

Views: 159

Answers (1)

Quimby
Quimby

Reputation: 19113

This is code is very unsafe and filled with UB and syntax errors.

  • You cannot write to memory from one thread and read it from another without synchronization.
  • Declaring timespan variable does not put the thread to sleep.
  • What happens if get_area does not get called at all while move_robot is iterating? Unlikely but it is also very unlikely to get 1:1 mapping.
  • Sleep is not a synchronization primitive.
  • this. is always wrong syntax.
  • std::async does not return a thread but std::future instead.

Do some research on threading in C++ - cppreference can also serve as a good tutorial with its examples.

You should rewrite the code with std::atomic<T> for primitive types like this->moving, and locking for others. Of course only if they are accessed from multiple threads and are not naturally thread-safe.

Because you require 1:1 mapping, use 1 producer, 1 consumer queue with std::condition_variable:

  • move_robot will push to the queue and notify the consumer.
  • get_area will wait on the notification, process enqueued elements and go back to sleep.
  • You can add a poison pill to the queue to signal get_area that the work is done.
  • Spawn both threads in main and wait on them.

Upvotes: 2

Related Questions