Threads doesn't end correctly: it ignores failing loop conditions

Question

I was working on a small combat-code for fun trying to learn to use threads in c++ but stumpled upon a bug which i can't figure out.

I use a mutext for synchronisation:

mutex mux1;

and a threaded function:

void dueler(Player &player, Player &enemy)
{
     do
     {
          Sleep(player.get_as());
          mux1.lock();
          cout << player.get_name()<< " hit " << enemy.get_name() << " for " << player.get_str() << " damage!" << endl;
          enemy.dmg(player.get_str());
          mux1.unlock();
     } while ((enemy.ask_dead() != true) && (player.ask_dead() != true));
 }

which I call in main (full code: http://pastebin.com/1FBf2FCQ):

int main()
{
    Player Player1("kasper", 100, 5, 200);
    Player Player2("Bagger", 150, 8, 3000);
    thread dueler1(dueler,Player1,Player2);
    thread dueler2(dueler, Player2, Player1);
    dueler1.join();
    dueler2.join();
    cout <<endl<< "battle is over!";
}

The threaded function makes to Players (http://pastebin.com/ZCTfUYiS) fight against each other:

class Player
{
public:
    Player(string name_, int maxhp_, int strenght_, int attackspeed_) {
        name = name_; 
        maxhp = maxhp_; 
        hp = maxhp;
        strenght = strenght_;
        attackspeed = attackspeed_;
        isdead = false;
    }
    void fullhp() {  hp = maxhp;  }
    void dmg(int dmg) {
        hp -= dmg;
        if (hp < 0) dead();
    }
    void dead() {
        isdead = true;
        cout << name <<" died like a pessant";
    }
    string get_name() { return name; }
    int get_hp()    { return hp; }
    int get_maxhp() { return maxhp; }
    int get_str()   { return strenght; }
    int get_as()    { return attackspeed; }
    bool ask_dead() {return isdead; }
private:
    string name;
    int maxhp;
    int hp;
    int strenght;
    int attackspeed;
    bool isdead;
};

The problem happens when Player2 gets killed:

• thread 1 ( which handles the damage dealt by player 1) stops as is should,
• thread 2 ( which handles Player2 who's dead) continues to run, and completly ignores the while statement until Player1 is dead.

I would have expected the while to end because of the condition (player.ask_dead() != true) should fail.

Any suggestions?

Answer 1

You have done synchronization properly by putting a mutext to avoid concurrent access to your object. That's already a very good start !

The problem:

The problem is that when you create a thread, it makes a private copy of the arguments you pass (see also this related SO question)

You can easily verify experimentally the references by adding this first statment in dueller() :

cout << "dueller with player " << (void*)&player << " and ennemy " << (void*)&enemy << endl;

When dueler() is called with arguments by reference, the reference refers not to the original object (local variable in main()) but to its clone.

The solution:

You have to use std::ref() to make sure that all the threads really refer to the same objects:

thread dueler1(dueler, std::ref(Player1), std::ref(Player2));
thread dueler2(dueler, std::ref(Player2), std::ref(Player1));

Other remarks:

When a player dies, his dueler() function is most probably waiting for the mutex. So when the mutex becomes free, he performs damages, despite being dead.

Ok ! in a game you could argue that the player is dead but still running with this sword to the victim... But we can also fix it by adding a condition when entering in the protected area:

    mux1.lock();  // we were waiting, a certain time
    if (!player.ask_dead()) {  // so first check if we are still alive
        cout << player.get_name() << " hit " << enemy.get_name() << " for " << player.get_str() << " damage!" << endl;
        enemy.dmg(player.get_str());
    }
    mux1.unlock();