Implement a thread-safe list of STKPeer

Block re-connect from the same ip and port and clean up add or
remove of peers

src/network/event.cpp

@@ -18,7 +18,6 @@
 
 #include "network/event.hpp"
 
-#include "network/stk_host.hpp"
 #include "network/stk_peer.hpp"
 #include "utils/log.hpp"
 #include "utils/time.hpp"
@@ -28,7 +27,7 @@
 /** \brief Constructor
  *  \param event : The event that needs to be translated.
  */
-Event::Event(ENetEvent* event)
+Event::Event(ENetEvent* event, std::shared_ptr<STKPeer> peer)
 {
     m_arrival_time = (double)StkTime::getTimeSinceEpoch();
 
@@ -61,7 +60,7 @@ Event::Event(ENetEvent* event)
         enet_packet_destroy(event->packet);
     }
 
-    m_peer = STKHost::get()->getPeer(event->peer);
+    m_peer = peer;
     if(m_type == EVENT_TYPE_MESSAGE && m_peer->isClientServerTokenSet() &&
         m_data->getToken()!=m_peer->getClientServerToken() )
     {

src/network/event.hpp

@@ -30,6 +30,8 @@
 
 #include "enet/enet.h"
 
+#include <memory>
+
 class STKPeer;
 
 /*!
@@ -65,13 +67,13 @@ private:
     EVENT_TYPE m_type;
 
     /** Pointer to the peer that triggered that event. */
-    STKPeer* m_peer;
+    std::shared_ptr<STKPeer> m_peer;
 
     /** Arrivial time of the event, for timeouts. */
     double m_arrival_time;
 
 public:
-         Event(ENetEvent* event);
+         Event(ENetEvent* event, std::shared_ptr<STKPeer> peer);
         ~Event();
 
     // ------------------------------------------------------------------------
@@ -80,7 +82,7 @@ public:
 
     // ------------------------------------------------------------------------
     /** Returns the peer of this event. */
-    STKPeer* getPeer() const { return m_peer;  }
+    STKPeer* getPeer() const { return m_peer.get(); }
     // ------------------------------------------------------------------------
     /** \brief Get a const reference to the received data.
      *  This is empty for events like connection or disconnections. 

src/network/protocols/client_lobby.cpp

@@ -208,7 +208,6 @@ void ClientLobby::voteLaps(uint8_t player_id, uint8_t laps,
 void ClientLobby::leave()
 {
     m_server->disconnect();
-    STKHost::get()->removePeer(m_server);
     m_server_address.clear();
 }   // leave
 
@@ -452,7 +451,6 @@ void ClientLobby::disconnectedPlayer(Event* event)
         }
     }   // while
 
-    STKHost::get()->removePeer(event->getPeer());
 }   // disconnectedPlayer
 
 //-----------------------------------------------------------------------------

src/network/protocols/connect_to_peer.cpp

@@ -115,6 +115,13 @@ void ConnectToPeer::asynchronousUpdate()
         }
         case WAIT_FOR_CONNECTION:
         {
+            if (STKHost::get()->peerExists(m_peer_address))
+            {
+                Log::error("ConnectToPeer",
+                    "The peer you want to connect to already exists.");
+                m_state = DONE;
+                break;
+            }
             // Each 2 second for a ping or broadcast
             if (StkTime::getRealTime() > m_timer + 2.0)
             {

src/network/protocols/server_lobby.cpp

@@ -644,8 +644,6 @@ void ServerLobby::clientDisconnected(Event* event)
     }
 
     sendMessageToPeersChangingToken(msg, /*reliable*/true);
-    // Remove the profile from the peer (to avoid double free)
-    STKHost::get()->removePeer(event->getPeer());
     delete msg;
     
 }   // clientDisconnected

src/network/stk_host.cpp

@@ -359,12 +359,7 @@ STKHost::~STKHost()
         delete m_game_setup;
     m_game_setup = NULL;
 
-    // Delete all connected peers
+    m_peers.clear();
-    while (!m_peers.empty())
-    {
-        delete m_peers.back();
-        m_peers.pop_back();
-    }
 
     Network::closeLog();
     stopListening();
@@ -634,27 +629,9 @@ GameSetup* STKHost::setupNewGame()
 */
 void STKHost::deleteAllPeers()
 {
-    // remove all peers
-    for (unsigned int i = 0; i < m_peers.size(); i++)
-    {
-        delete m_peers[i];
-        m_peers[i] = NULL;
-    }
     m_peers.clear();
 }   // deleteAllPeers
 
-// ----------------------------------------------------------------------------
-/** Called when STK exits. It stops the listening thread and the
- *  ProtocolManager.
- */
-void STKHost::abort()
-{
-    // Finish protocol manager first, to avoid that it access data
-    // in STKHost.
-    ProtocolManager::lock()->abort();
-    stopListening();
-}   // abort
-
 // --------------------------------------------------------------------
 /** Sets an error message for the gui.
  */
@@ -726,7 +703,7 @@ bool STKHost::isAuthorisedToControl() const
     // stk logic), no peer is authorised.
     if(m_peers.size()==0)
         return false;
-    return m_peers[0]->isAuthorised();
+    return m_peers.begin()->second->isAuthorised();
 }   // isAuthorisedToControl
 
 // ----------------------------------------------------------------------------
@@ -778,32 +755,46 @@ void STKHost::mainLoop()
             {
                 // Don't create more event if no protocol manager or it will
                 // be exiting
+                enet_packet_destroy(event.packet);
                 continue;
             }
-            // Create an STKEvent with the event data. This will also
+
-            // create the peer if it doesn't exist already
+            Event* stk_event = NULL;
-            Event* stk_event = new Event(&event);
+            if (event.type == ENET_EVENT_TYPE_CONNECT)
-            STKPeer* peer = stk_event->getPeer();
-            if (stk_event->getType() == EVENT_TYPE_CONNECTED)
             {
-                Log::info("STKHost", "A client has just connected. There are "
+                auto stk_peer = std::make_shared<STKPeer>(event.peer);
-                          "now %lu peers.", m_peers.size());
+                m_peers[event.peer] = stk_peer;
-                Log::debug("STKHost", "Addresses are : %lx, %lx",
+                stk_event = new Event(&event, stk_peer);
-                           stk_event->getPeer(), peer);
+                TransportAddress addr(event.peer->address);
-            }   // EVENT_TYPE_CONNECTED
+                Log::info("STKHost", "%s has just connected. There are "
-            else if (stk_event->getType() == EVENT_TYPE_DISCONNECTED)
+                    "now %u peers.", addr.toString().c_str(), getPeerCount());
+            }   // ENET_EVENT_TYPE_CONNECT
+            else if (event.type == ENET_EVENT_TYPE_DISCONNECT)
             {
-                Log::info("STKHost", "A client has just disconnected.");
                 Log::flushBuffers();
-            }   // EVENT_TYPE_CONNECTED
+                // Use the previous stk peer so protocol can see the network
-            else if (stk_event->getType() == EVENT_TYPE_MESSAGE)
+                // profile and handle it for disconnection
+                assert(m_peers.find(event.peer) != m_peers.end());
+                stk_event = new Event(&event, m_peers.at(event.peer));
+                m_peers.erase(event.peer);
+                TransportAddress addr(event.peer->address);
+                Log::info("STKHost", "%s has just disconnected. There are "
+                    "now %u peers.", addr.toString().c_str(), getPeerCount());
+            }   // ENET_EVENT_TYPE_DISCONNECT
+
+            if (!stk_event)
+            {
+                assert(m_peers.find(event.peer) != m_peers.end());
+                stk_event = new Event(&event, m_peers.at(event.peer));
+            }
+            if (stk_event->getType() == EVENT_TYPE_MESSAGE)
             {
                 Network::logPacket(stk_event->data(), true);
-                TransportAddress stk_addr(peer->getAddress());
 #ifdef DEBUG_MESSAGE_CONTENT
                 Log::verbose("NetworkManager",
                              "Message, Sender : %s time %f message:",
-                             stk_addr.toString(/*show port*/false).c_str(),
+                             stk_event->getPeer()->getAddress()
+                             .toString(/*show port*/false).c_str(),
                              StkTime::getRealTime());
                 Log::verbose("NetworkManager", "%s",
                              stk_event->data().getLogMessage().c_str());
@@ -906,15 +897,16 @@ void STKHost::handleDirectSocketRequest(Network* direct_socket)
  */
 bool STKHost::peerExists(const TransportAddress& peer)
 {
-    ENetHost *host = m_network->getENetHost();
+    std::lock_guard<std::mutex> lock(m_peers_mutex);
-    for (unsigned int i = 0; i < host->peerCount ; i++)
+    for (auto p : m_peers)
-    {
-        if (host->peers[i].address.host == ntohl(peer.getIP()) &&
-            host->peers[i].address.port == peer.getPort()        )
     {
+        auto stk_peer = p.second;
+        if (stk_peer->getAddress() == peer ||
+            ((stk_peer->getAddress().isPublicAddressLocalhost() ||
+            peer.isPublicAddressLocalhost()) &&
+            stk_peer->getAddress().getPort() == peer.getPort()))
             return true;
     }
-    }
     return false;
 }   // peerExists
 
@@ -924,39 +916,6 @@ std::vector<NetworkPlayerProfile*> STKHost::getMyPlayerProfiles()
     return m_game_setup->getAllPlayersOnHost(m_host_id);
 }   // getMyPlayerProfiles
 
-// ----------------------------------------------------------------------------
-/** Returns the STK peer belonging to the given enet_peer. If no STKPeer
- *  exists, create a new STKPeer.
- *  \param enet_peer The EnetPeer.
- */
-STKPeer* STKHost::getPeer(ENetPeer *enet_peer)
-{
-    for(unsigned int i=0; i<m_peers.size(); i++)
-    {
-        if(m_peers[i]->isSamePeer(enet_peer))
-            return m_peers[i];
-    }
-
-    // Make sure that a client only adds one other peer (=the server).
-    if(NetworkConfig::get()->isClient() && m_peers.size()>0)
-    {
-        Log::error("STKHost",
-                   "Client is adding more than one server, ignored for now.");
-    }
-
-
-    //FIXME Should we check #clients here? It might be easier to only
-    // handle this at connect time, not in all getPeer calls.
-    STKPeer *peer = new STKPeer(enet_peer);
-    Log::debug("getPeer", 
-               "Creating a new peer, address are STKPeer:%p, Peer:%p",
-               peer, enet_peer);
-
-    m_peers.push_back(peer);
-    m_next_unique_host_id ++;
-    return peer;
-}   // getPeer
-
 // ----------------------------------------------------------------------------
 /** \brief Return the only server peer for client.
  *  \return STKPeer the STKPeer of server.
@@ -965,7 +924,7 @@ STKPeer* STKHost::getServerPeerForClient() const
 {
     assert(m_peers.size() == 1);
     assert(NetworkConfig::get()->isClient());
-    return m_peers[0];
+    return m_peers.begin()->second.get();
 }   // getServerPeerForClient
 
 // ----------------------------------------------------------------------------
@@ -986,44 +945,6 @@ bool STKHost::isConnectedTo(const TransportAddress& peer)
     return false;
 }   // isConnectedTo
 
-
-// ----------------------------------------------------------------------------
-void STKHost::removePeer(const STKPeer* peer)
-{
-    if (!peer || !peer->exists()) // peer does not exist (already removed)
-        return;
-
-    TransportAddress addr(peer->getAddress());
-    Log::debug("STKHost", "Disconnected host: %s", addr.toString().c_str());
-            
-    // remove the peer:
-    bool removed = false;
-    for (unsigned int i = 0; i < m_peers.size(); i++)
-    {
-        if (m_peers[i]->isSamePeer(peer) && !removed) // remove only one
-        {
-            delete m_peers[i];
-            m_peers.erase(m_peers.begin() + i, m_peers.begin() + i + 1);
-            Log::verbose("NetworkManager",
-                "The peer has been removed from the Network Manager.");
-            removed = true;
-        }
-        else if (m_peers[i]->isSamePeer(peer))
-        {
-            Log::fatal("NetworkManager",
-                       "Multiple peers match the disconnected one.");
-        }
-    }   // for i < m_peers.size()
-
-    if (!removed)
-        Log::warn("NetworkManager", "The peer that has been disconnected was "
-                                    "not registered by the Network Manager.");
-
-    Log::info("NetworkManager",
-              "Somebody is now disconnected. There are now %lu peers.",
-              m_peers.size());
-}   // removePeer
-
 //-----------------------------------------------------------------------------
 /** Sends data to all peers except the specified one.
  *  \param peer Peer which will not receive the message.
@@ -1033,13 +954,23 @@ void STKHost::removePeer(const STKPeer* peer)
 void STKHost::sendPacketExcept(STKPeer* peer, NetworkString *data,
                                bool reliable)
 {
-    for (unsigned int i = 0; i < m_peers.size(); i++)
+    std::lock_guard<std::mutex> lock(m_peers_mutex);
+    for (auto p : m_peers)
     {
-        STKPeer* p = m_peers[i];
+        STKPeer* stk_peer = p.second.get();
-        if (!p->isSamePeer(peer))
+        if (!stk_peer->isSamePeer(peer))
         {
-            p->sendPacket(data, reliable);
+            stk_peer->sendPacket(data, reliable);
         }
     }
 }   // sendPacketExcept
 
+//-----------------------------------------------------------------------------
+/** Sends a message from a client to the server. */
+void STKHost::sendToServer(NetworkString *data, bool reliable)
+{
+    std::lock_guard<std::mutex> lock(m_peers_mutex);
+    assert(m_peers.size() == 1);
+    assert(NetworkConfig::get()->isClient());
+    m_peers.begin()->second->sendPacket(data, reliable);
+}   // sendToServer

src/network/stk_host.hpp

@@ -38,7 +38,9 @@
 #include <enet/enet.h>
 
 #include <atomic>
+#include <map>
 #include <memory>
+#include <mutex>
 #include <thread>
 
 class GameSetup;
@@ -58,9 +60,6 @@ public:
         PORT_ANY = 0              //!< Any port.
     };
 
-
-    friend class STKPeer; // allow direct enet modifications in implementations
-
 private:
     /** Singleton pointer to the instance. */
     static STKHost* m_stk_host;
@@ -74,8 +73,11 @@ private:
     /** Network console thread */
     std::thread m_network_console;
 
+    /** Make sure the removing or adding a peer is thread-safe. */
+    std::mutex m_peers_mutex;
+
     /** The list of peers connected to this instance. */
-    std::vector<STKPeer*> m_peers;
+    std::map<ENetPeer*, std::shared_ptr<STKPeer> > m_peers;
 
     /** Next unique host id. It is increased whenever a new peer is added (see
      *  getPeer()), but not decreased whena host (=peer) disconnects. This
@@ -112,9 +114,6 @@ private:
     /** The private port enet socket is bound. */
     uint16_t m_private_port;
 
-    /** An error message, which is set by a protocol to be displayed
-     *  in the GUI. */
-
              STKHost(std::shared_ptr<Server> server);
              STKHost(const irr::core::stringw &server_name);
     virtual ~STKHost();
@@ -156,18 +155,15 @@ public:
     const TransportAddress& getPublicAddress() const
                                                    { return m_public_address; }
     // ------------------------------------------------------------------------
-    const TransportAddress& getStunAddress() const
+    const TransportAddress& getStunAddress() const   { return m_stun_address; }
-                                                     { return m_stun_address; }
     // ------------------------------------------------------------------------
-    uint16_t getPrivatePort() const
+    uint16_t getPrivatePort() const                  { return m_private_port; }
-                                                     { return m_private_port; }
     // ------------------------------------------------------------------------
     void setPrivatePort();
     // ------------------------------------------------------------------------
     void setPublicAddress();
     // ------------------------------------------------------------------------
     virtual GameSetup* setupNewGame();
-    void abort();
     void deleteAllPeers();
     bool connect(const TransportAddress& peer);
 
@@ -195,9 +191,7 @@ public:
     void        startListening();
     void        stopListening();
     bool        peerExists(const TransportAddress& peer_address);
-    void        removePeer(const STKPeer* peer);
     bool        isConnectedTo(const TransportAddress& peer_address);
-    STKPeer    *getPeer(ENetPeer *enet_peer);
     STKPeer    *getServerPeerForClient() const;
     std::vector<NetworkPlayerProfile*> getMyPlayerProfiles();
     void        setErrorMessage(const irr::core::stringw &message);
@@ -229,7 +223,16 @@ public:
     }  // sendRawPacket
     // ------------------------------------------------------------------------
     /** Returns a const reference to the list of peers. */
-    const std::vector<STKPeer*> &getPeers()                 { return m_peers; }
+    std::vector<STKPeer*> getPeers()
+    {
+        std::lock_guard<std::mutex> lock(m_peers_mutex);
+        std::vector<STKPeer*> peers;
+        for (auto p : m_peers)
+        {
+            peers.push_back(p.second.get());
+        }
+        return peers;
+    }
     // ------------------------------------------------------------------------
     /** Returns the next (unique) host id. */
     unsigned int getNextHostId() const
@@ -239,7 +242,11 @@ public:
     }
     // ------------------------------------------------------------------------
     /** Returns the number of currently connected peers. */
-    unsigned int getPeerCount()                 { return (int)m_peers.size(); }
+    unsigned int getPeerCount()
+    {
+        std::lock_guard<std::mutex> lock(m_peers_mutex);
+        return m_peers.size();
+    }
     // ------------------------------------------------------------------------
     /** Sets the global host id of this host. */
     void setMyHostId(uint8_t my_host_id)            { m_host_id = my_host_id; }
@@ -247,12 +254,7 @@ public:
     /** Returns the host id of this host. */
     uint8_t getMyHostId() const                           { return m_host_id; }
     // ------------------------------------------------------------------------
-    /** Sends a message from a client to the server. */
+    void sendToServer(NetworkString *data, bool reliable = true);
-    void sendToServer(NetworkString *data, bool reliable = true)
-    {
-        assert(NetworkConfig::get()->isClient());
-        m_peers[0]->sendPacket(data, reliable);
-    }   // sendToServer
     // ------------------------------------------------------------------------
     /** True if this is a client and server in graphics mode made by server
      *  creation screen. */

src/network/stk_peer.cpp

@@ -30,6 +30,7 @@
 /** Constructor for an empty peer.
  */
 STKPeer::STKPeer(ENetPeer *enet_peer)
+       : m_peer_address(enet_peer->address)
 {
     m_enet_peer           = enet_peer;
     m_is_authorised       = false;
@@ -43,7 +44,7 @@ STKPeer::STKPeer(ENetPeer *enet_peer)
  */
 STKPeer::~STKPeer()
 {
-    m_enet_peer           = NULL;
+    enet_peer_disconnect(m_enet_peer, 0);
     m_client_server_token = 0;
 }   // ~STKPeer
 
@@ -74,22 +75,6 @@ void STKPeer::sendPacket(NetworkString *data, bool reliable)
     enet_peer_send(m_enet_peer, 0, packet);
 }   // sendPacket
 
-//-----------------------------------------------------------------------------
-/** Returns the IP address (in host format) of this client.
- */
-uint32_t STKPeer::getAddress() const
-{
-    return ntohl(m_enet_peer->address.host);
-}   // getAddress
-
-//-----------------------------------------------------------------------------
-/** Returns the port of this peer.
- */
-uint16_t STKPeer::getPort() const
-{
-    return m_enet_peer->address.port;
-}
-
 //-----------------------------------------------------------------------------
 /** Returns if the peer is connected or not.
  */

src/network/stk_peer.hpp

@@ -23,6 +23,7 @@
 #ifndef STK_PEER_HPP
 #define STK_PEER_HPP
 
+#include "network/transport_address.hpp"
 #include "utils/no_copy.hpp"
 #include "utils/types.hpp"
 
@@ -55,6 +56,9 @@ protected:
 
     /** True if this peer is authorised to control a server. */
     bool m_is_authorised;
+
+    TransportAddress m_peer_address;
+
 public:
              STKPeer(ENetPeer *enet_peer);
     virtual ~STKPeer();
@@ -64,8 +68,7 @@ public:
     void disconnect();
     bool isConnected() const;
     bool exists() const;
-    uint32_t getAddress() const;
+    const TransportAddress& getAddress() const { return m_peer_address; }
-    uint16_t getPort() const;
     bool isSamePeer(const STKPeer* peer) const;
     bool isSamePeer(const ENetPeer* peer) const;
     std::vector<NetworkPlayerProfile*> getAllPlayerProfiles();