Thread Overriding Main (?)

[Charsmud]

I am currently stuck in a situation where I initialize a thread and launch it, but it seems to halt my main.  Here are the affected files:

#include "ClientHandler.hpp"

#include <algorithm>
#include <iostream>

ClientHandler::ClientHandler()
{
}

void ClientHandler::addConnection(PlayerSession player)
{
        mUsers.push_back(player);
        Heartbeat beat(player.getIp());
        mHeartbeat.push_back(&beat);
        std::cout << "Finished adding connection + started heartbeat!" << std::endl;
}

Heartbeat::Heartbeat(sf::IpAddress ip)
        : mThread(std::bind(&Heartbeat::heart, this, ip))
{
        mThread.launch();
}

void Heartbeat::heart(sf::IpAddress ip)
{
        bool isAlive = true;
        sf::UdpSocket sock;
        sf::Packet packet;
        sf::IpAddress recIp;
        unsigned short recPort;
        sock.bind(54001);
       
        while(isAlive)
        {
                sf::sleep(sf::milliseconds(1000));
                sendHeartbeat(ip);
                std::cout << "In Heartbeat! " << std::endl;




                if(sock.receive(packet, recIp, recPort) != sf::Socket::Done)
                {
                        std::cout << "SOMETHING WENT WRONG WHEN TRYING TO RECEIVE" << std::endl;
                        isAlive = false;
                }
                int x;
                if(packet >> x)
                {
                        if(x == ClientPacket::ReplyHeartbeat)
                        {
                                std::cout << "RECIEVED REPLY HEARTBEAT" << std::endl;
                        }
                        if(x == ClientPacket::PlayerConnect)
                        {
                                std::cout << "THIS ISN'T SUPPOSED TO BE HAPPENING HERE!" << std::endl;
                        }
                }
        }
}

void ServerCore::enterServerLoop()
{
        if(mSocket.bind(mPort) != sf::Socket::Done)
        {
                std::cout << "SOMETHING DIDNT WORK" << std::endl;
        }
        std::cout << "Initialized Server" << std::endl;

        unsigned short port;
        sf::IpAddress ip;
        while(true)
        {
                std::cout << ":D" << std::endl;

                sf::Packet packet;

                std::size_t received;

                if(mSocket.receive(packet, ip, port) != sf::Socket::Done)
                {
                       
                }
               
                int x;

                if(packet >> x)
                {
                        std::cout << x << std::endl;
                        if(x == ClientPacket::PlayerConnect)
                        {
                                std::cout << "Player Connected!" << std::endl;
                                sf::Packet sPacket;

                                sf::String username;
                                packet >> username;

                                PlayerSession user(ip, username);

                                mClients.addConnection(user);
                                std::cout << "AFTER ADDCONNECTION(USER)" << std::endl;
                                std::cout << mClients.size() << std::endl;

                                sPacket << ServerPacket::ServerStop;
                                std::cout << sPacket << " " << ip << " " << (std::string)username << std::endl;
                                //TESTING THE BROADAST IP
                                if(mSocket.send(sPacket, ip, 54000) != sf::Socket::Done)
                                {
                                        std::cout << "I AM SAD" << std::endl;
                                }
                                ...
 

Here is the server output log:

PREPARING USER DATABASE
Initialized Server
:D
1
Player Connected!
Finished adding connection + started heartbeat!
In Heartbeat!
RECIEVED REPLY HEARTBEAT
In Heartbeat!
RECIEVED REPLY HEARTBEAT
In Heartbeat!
RECIEVED REPLY HEARTBEAT
In Heartbeat!
RECIEVED REPLY HEARTBEAT
In Heartbeat!
RECIEVED REPLY HEARTBEAT
In Heartbeat!
 

The print after the addConnection call is never reached.  Any suggestions? 

[Nexus]

void ClientHandler::addConnection(PlayerSession player)
{
    mUsers.push_back(player);
    Heartbeat beat(player.getIp());
    mHeartbeat.push_back(&beat);
    std::cout << "Finished adding connection + started heartbeat!" << std::endl;
}

How long does beat live?
What happens when it dies?
Hint: it's the same problem that Laurent showed you last time.

And please use minimal complete examples in the future. If your problem is related to threads, nobody cares about networking and custom classes.

[Laurent]

Your Hearbeat object is local to the addConnection function, therefore it is destroyed at the end of it. When it is destroyed, its sf::Thread member is destroyed too, and the destructor of sf::Thread blocks the current thread until the thread completes.

Another bad side effect of your code is that you're adding to your array the address of an object that will not exist anymore.

The solution is to allocate your Hearbeat objects dynamically, if you want them to live longer than the function where they are created. Wrap them into smart pointers to avoid stupid problems.

[Nexus]

The solution is to allocate your Hearbeat objects dynamically, if you want them to live longer than the function where they are created. Wrap them into smart pointers to avoid stupid problems.

Or make them movable and transfer ownership to the container. Since sf::Thread is not movable, it would have to be wrapped in a std::unique_ptr, however standard threads (std::thread) are movable.

With std::thread, the destructor would most likely have crashed (destroying running threads is UB), so you would have noticed the problem quicker.

[Charsmud]

Your Hearbeat object is local to the addConnection function, therefore it is destroyed at the end of it. When it is destroyed, its sf::Thread member is destroyed too, and the destructor of sf::Thread blocks the current thread until the thread completes.

Another bad side effect of your code is that you're adding to your array the address of an object that will not exist anymore.

The solution is to allocate your Hearbeat objects dynamically, if you want them to live longer than the function where they are created. Wrap them into smart pointers to avoid stupid problems.

Thanks, dynamically allocating the objects fixed the problem.  As a final question, if I want to make more Heartbeat objects, can I use the same initial object?  IE

class Classname
{
    private:
        Heartbeat* hb;
};

void allocateHearbeat()
{
    hb = new Heartbeat();
}

Would this create a new Heartbeat instance each time, or would it simply override the previous instance?  Sorry if that's a bit of a stupid question. 

[Nexus]

It would create a new instance every time, and lead to a memory leak.

Avoid raw new and delete and use RAII (here std::unique_ptr) instead. Or consider the alternative in my last post: move semantics.

[Charsmud]

It would create a new instance every time, and lead to a memory leak.

Avoid raw new and delete and use RAII (here std::unique_ptr) instead. Or consider the alternative in my last post: move semantics.

Thanks for the link, very interesting!  I might be missing something though, because my implementation does not seem to work; it still follows the problem of the original post.  Here is the updated block:

void ClientHandler::addConnection(PlayerSession player)
{
        mUsers.push_back(player);
        mBeat(new Heartbeat(player.getIp()));
        //mHeartbeat.push_back(mBeat);
        std::cout << "Finished adding connection + started heartbeat!" << std::endl;
}

private:
        std::unique_ptr<Heartbeat> mBeat;

[Nexus]

mBeat(new Heartbeat(player.getIp()));

What should this line do? Did you mean to call reset()?

And I thought you want to add the heartbeat object to a container, not store it as single member, don't you?

[Charsmud]

mBeat(new Heartbeat(player.getIp()));

What should this line do? Did you mean to call reset()?

And I thought you want to add the heartbeat object to a container, not store it as single member, don't you?

That line should create the heartbeat object and the object initialization should start the thread.  The container will not accept the unique pointer,  so could I pass a memory address to it?

[Nexus]

That line should create the heartbeat object

But it does not store it in the unique pointer. I wonder why this even compiles, maybe an overloaded operator() in the standard library?

The container will not accept the unique pointer,  so could I pass a memory address to it?

No... But you have to tell me what you want to do (semantically), not vice versa. Either you want to store it in the container, and then you make it work, or you don't...

[Charsmud]

That line should create the heartbeat object

But it does not store it in the unique pointer. I wonder why this even compiles, maybe an overloaded operator() in the standard library?

The container will not accept the unique pointer,  so could I pass a memory address to it?

No... But you have to tell me what you want to do (semantically), not vice versa. Either you want to store it in the container, and then you make it work, or you don't...

The thread creation code is in the constructor of Heartbeat. I need the array to hold the unique pointer for use later.