From dd2e32a9532b7b13843e7f9191d5b3919f57306f Mon Sep 17 00:00:00 2001 From: Benau Date: Fri, 9 Mar 2018 18:33:19 +0800 Subject: [PATCH] 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 | 5 +- src/network/event.hpp | 8 +- src/network/protocols/client_lobby.cpp | 2 - src/network/protocols/connect_to_peer.cpp | 7 + src/network/protocols/server_lobby.cpp | 2 - src/network/stk_host.cpp | 179 +++++++--------------- src/network/stk_host.hpp | 46 +++--- src/network/stk_peer.cpp | 21 +-- src/network/stk_peer.hpp | 7 +- 9 files changed, 101 insertions(+), 176 deletions(-) diff --git a/src/network/event.cpp b/src/network/event.cpp index 33803cb0a..ed1cfbd97 100644 --- a/src/network/event.cpp +++ b/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 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() ) { diff --git a/src/network/event.hpp b/src/network/event.hpp index f044c636f..37860d4ab 100644 --- a/src/network/event.hpp +++ b/src/network/event.hpp @@ -30,6 +30,8 @@ #include "enet/enet.h" +#include + 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 m_peer; /** Arrivial time of the event, for timeouts. */ double m_arrival_time; public: - Event(ENetEvent* event); + Event(ENetEvent* event, std::shared_ptr 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. diff --git a/src/network/protocols/client_lobby.cpp b/src/network/protocols/client_lobby.cpp index b819e772c..2a414c5ac 100644 --- a/src/network/protocols/client_lobby.cpp +++ b/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 //----------------------------------------------------------------------------- diff --git a/src/network/protocols/connect_to_peer.cpp b/src/network/protocols/connect_to_peer.cpp index e20ad3083..6118622c0 100644 --- a/src/network/protocols/connect_to_peer.cpp +++ b/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) { diff --git a/src/network/protocols/server_lobby.cpp b/src/network/protocols/server_lobby.cpp index d3b1730e7..26156475e 100644 --- a/src/network/protocols/server_lobby.cpp +++ b/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 diff --git a/src/network/stk_host.cpp b/src/network/stk_host.cpp index 1df516a9e..fd5834d3c 100644 --- a/src/network/stk_host.cpp +++ b/src/network/stk_host.cpp @@ -295,7 +295,7 @@ STKHost::STKHost(const irr::core::stringw &server_name) addr.host = STKHost::HOST_ANY; addr.port = NetworkConfig::get()->getServerPort(); - m_network= new Network(NetworkConfig::get()->getMaxPlayers(), + m_network = new Network(NetworkConfig::get()->getMaxPlayers(), /*channel_limit*/2, /*max_in_bandwidth*/0, /*max_out_bandwidth*/ 0, &addr, @@ -359,12 +359,7 @@ STKHost::~STKHost() delete m_game_setup; m_game_setup = NULL; - // Delete all connected peers - while (!m_peers.empty()) - { - delete m_peers.back(); - m_peers.pop_back(); - } + m_peers.clear(); 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 = new Event(&event); - STKPeer* peer = stk_event->getPeer(); - if (stk_event->getType() == EVENT_TYPE_CONNECTED) + + Event* stk_event = NULL; + if (event.type == ENET_EVENT_TYPE_CONNECT) { - Log::info("STKHost", "A client has just connected. There are " - "now %lu peers.", m_peers.size()); - Log::debug("STKHost", "Addresses are : %lx, %lx", - stk_event->getPeer(), peer); - } // EVENT_TYPE_CONNECTED - else if (stk_event->getType() == EVENT_TYPE_DISCONNECTED) + auto stk_peer = std::make_shared(event.peer); + m_peers[event.peer] = stk_peer; + stk_event = new Event(&event, stk_peer); + TransportAddress addr(event.peer->address); + Log::info("STKHost", "%s has just connected. There are " + "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 - else if (stk_event->getType() == EVENT_TYPE_MESSAGE) + // Use the previous stk peer so protocol can see the network + // 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,14 +897,15 @@ void STKHost::handleDirectSocketRequest(Network* direct_socket) */ bool STKHost::peerExists(const TransportAddress& peer) { - ENetHost *host = m_network->getENetHost(); - for (unsigned int i = 0; i < host->peerCount ; i++) + std::lock_guard lock(m_peers_mutex); + 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 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; iisSamePeer(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 lock(m_peers_mutex); + for (auto p : m_peers) { - STKPeer* p = m_peers[i]; - if (!p->isSamePeer(peer)) + STKPeer* stk_peer = p.second.get(); + 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 lock(m_peers_mutex); + assert(m_peers.size() == 1); + assert(NetworkConfig::get()->isClient()); + m_peers.begin()->second->sendPacket(data, reliable); +} // sendToServer diff --git a/src/network/stk_host.hpp b/src/network/stk_host.hpp index 7ebda5ff9..966049eb0 100644 --- a/src/network/stk_host.hpp +++ b/src/network/stk_host.hpp @@ -38,7 +38,9 @@ #include #include +#include #include +#include #include 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 m_peers; + std::map > 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); 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 - { return m_stun_address; } + const TransportAddress& getStunAddress() const { return m_stun_address; } // ------------------------------------------------------------------------ - uint16_t getPrivatePort() const - { return m_private_port; } + uint16_t getPrivatePort() const { 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 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 &getPeers() { return m_peers; } + std::vector getPeers() + { + std::lock_guard lock(m_peers_mutex); + std::vector 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 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) - { - assert(NetworkConfig::get()->isClient()); - m_peers[0]->sendPacket(data, reliable); - } // sendToServer + void sendToServer(NetworkString *data, bool reliable = true); // ------------------------------------------------------------------------ /** True if this is a client and server in graphics mode made by server * creation screen. */ diff --git a/src/network/stk_peer.cpp b/src/network/stk_peer.cpp index 6aa5d8844..e3e819300 100644 --- a/src/network/stk_peer.cpp +++ b/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 @@ -66,7 +67,7 @@ void STKPeer::sendPacket(NetworkString *data, bool reliable) TransportAddress a(m_enet_peer->address); Log::verbose("STKPeer", "sending packet of size %d to %s at %f", data->size(), a.toString().c_str(),StkTime::getRealTime()); - + ENetPacket* packet = enet_packet_create(data->getData(), data->getTotalSize(), (reliable ? ENET_PACKET_FLAG_RELIABLE @@ -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. */ diff --git a/src/network/stk_peer.hpp b/src/network/stk_peer.hpp index 589c1bebf..604727601 100644 --- a/src/network/stk_peer.hpp +++ b/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; - uint16_t getPort() const; + const TransportAddress& getAddress() const { return m_peer_address; } bool isSamePeer(const STKPeer* peer) const; bool isSamePeer(const ENetPeer* peer) const; std::vector getAllPlayerProfiles();