Merge pull request #7677 from Techjar/netplay-peer-init-fix

NetPlay: Fix server peer initialization hang

Source/Core/Core/NetPlayServer.cpp

@@ -148,6 +148,20 @@ NetPlayServer::NetPlayServer(const u16 port, const bool forward_port,
   }
 }
 
+static PlayerId* PeerPlayerId(ENetPeer* peer)
+{
+  return static_cast<PlayerId*>(peer->data);
+}
+
+static void ClearPeerPlayerId(ENetPeer* peer)
+{
+  if (peer->data)
+  {
+    delete PeerPlayerId(peer);
+    peer->data = nullptr;
+  }
+}
+
 // called from ---NETPLAY--- thread
 void NetPlayServer::ThreadFunc()
 {
@@ -195,26 +209,10 @@ void NetPlayServer::ThreadFunc()
       {
       case ENET_EVENT_TYPE_CONNECT:
       {
-        ENetPeer* accept_peer = netEvent.peer;
+        // Actual client initialization is deferred to the receive event, so here
-        unsigned int error;
+        // we'll just log the new connection.
-        {
+        INFO_LOG(NETPLAY, "Peer connected from: %x:%u", netEvent.peer->address.host,
-          std::lock_guard<std::recursive_mutex> lkg(m_crit.game);
+                 netEvent.peer->address.port);
-          error = OnConnect(accept_peer);
-        }
-
-        if (error)
-        {
-          sf::Packet spac;
-          spac << (MessageId)error;
-          // don't need to lock, this client isn't in the client map
-          Send(accept_peer, spac);
-          if (netEvent.peer->data)
-          {
-            delete (PlayerId*)netEvent.peer->data;
-            netEvent.peer->data = nullptr;
-          }
-          enet_peer_disconnect_later(accept_peer, 0);
-        }
       }
       break;
       case ENET_EVENT_TYPE_RECEIVE:
@@ -222,18 +220,37 @@ void NetPlayServer::ThreadFunc()
         sf::Packet rpac;
         rpac.append(netEvent.packet->data, netEvent.packet->dataLength);
 
-        auto it = m_players.find(*(PlayerId*)netEvent.peer->data);
+        if (!netEvent.peer->data)
-        Client& client = it->second;
-        if (OnData(rpac, client) != 0)
         {
-          // if a bad packet is received, disconnect the client
+          // uninitialized client, we'll assume this is their initialization packet
-          std::lock_guard<std::recursive_mutex> lkg(m_crit.game);
+          unsigned int error;
-          OnDisconnect(client);
-
-          if (netEvent.peer->data)
           {
-            delete (PlayerId*)netEvent.peer->data;
+            std::lock_guard<std::recursive_mutex> lkg(m_crit.game);
-            netEvent.peer->data = nullptr;
+            error = OnConnect(netEvent.peer, rpac);
+          }
+
+          if (error)
+          {
+            sf::Packet spac;
+            spac << static_cast<MessageId>(error);
+            // don't need to lock, this client isn't in the client map
+            Send(netEvent.peer, spac);
+
+            ClearPeerPlayerId(netEvent.peer);
+            enet_peer_disconnect_later(netEvent.peer, 0);
+          }
+        }
+        else
+        {
+          auto it = m_players.find(*PeerPlayerId(netEvent.peer));
+          Client& client = it->second;
+          if (OnData(rpac, client) != 0)
+          {
+            // if a bad packet is received, disconnect the client
+            std::lock_guard<std::recursive_mutex> lkg(m_crit.game);
+            OnDisconnect(client);
+
+            ClearPeerPlayerId(netEvent.peer);
           }
         }
         enet_packet_destroy(netEvent.packet);
@@ -244,17 +261,13 @@ void NetPlayServer::ThreadFunc()
         std::lock_guard<std::recursive_mutex> lkg(m_crit.game);
         if (!netEvent.peer->data)
           break;
-        auto it = m_players.find(*(PlayerId*)netEvent.peer->data);
+        auto it = m_players.find(*PeerPlayerId(netEvent.peer));
         if (it != m_players.end())
         {
           Client& client = it->second;
           OnDisconnect(client);
 
-          if (netEvent.peer->data)
+          ClearPeerPlayerId(netEvent.peer);
-          {
-            delete (PlayerId*)netEvent.peer->data;
-            netEvent.peer->data = nullptr;
-          }
         }
       }
       break;
@@ -267,23 +280,14 @@ void NetPlayServer::ThreadFunc()
   // close listening socket and client sockets
   for (auto& player_entry : m_players)
   {
-    delete (PlayerId*)player_entry.second.socket->data;
+    ClearPeerPlayerId(player_entry.second.socket);
-    player_entry.second.socket->data = nullptr;
     enet_peer_disconnect(player_entry.second.socket, 0);
   }
 }
 
 // called from ---NETPLAY--- thread
-unsigned int NetPlayServer::OnConnect(ENetPeer* socket)
+unsigned int NetPlayServer::OnConnect(ENetPeer* socket, sf::Packet& rpac)
 {
-  sf::Packet rpac;
-  ENetPacket* epack;
-  do
-  {
-    epack = enet_peer_receive(socket, nullptr);
-  } while (epack == nullptr);
-  rpac.append(epack->data, epack->dataLength);
-
   // give new client first available id
   PlayerId pid = 1;
   for (auto i = m_players.begin(); i != m_players.end(); ++i)
@@ -320,7 +324,6 @@ unsigned int NetPlayServer::OnConnect(ENetPeer* socket)
   rpac >> player.revision;
   rpac >> player.name;
 
-  enet_packet_destroy(epack);
   // try to automatically assign new user a pad
   for (PadMapping& mapping : m_pad_map)
   {
@@ -402,7 +405,7 @@ unsigned int NetPlayServer::OnConnect(ENetPeer* socket)
   // add client to the player list
   {
     std::lock_guard<std::recursive_mutex> lkp(m_crit.players);
-    m_players.emplace(*(PlayerId*)player.socket->data, std::move(player));
+    m_players.emplace(*PeerPlayerId(player.socket), std::move(player));
     UpdatePadMapping();  // sync pad mappings with everyone
     UpdateWiimoteMapping();
   }

Source/Core/Core/NetPlayServer.h

@@ -125,7 +125,7 @@ private:
   void SendToClients(const sf::Packet& packet, PlayerId skip_pid = 0,
                      u8 channel_id = DEFAULT_CHANNEL);
   void Send(ENetPeer* socket, const sf::Packet& packet, u8 channel_id = DEFAULT_CHANNEL);
-  unsigned int OnConnect(ENetPeer* socket);
+  unsigned int OnConnect(ENetPeer* socket, sf::Packet& rpac);
   unsigned int OnDisconnect(const Client& player);
   unsigned int OnData(sf::Packet& packet, Client& player);