From 05afbf18052b2235a4a1516f6b5c19e88d055f72 Mon Sep 17 00:00:00 2001 From: josue Date: Fri, 12 Jun 2026 18:56:32 +0200 Subject: [PATCH] Deduplicate the hole punching pacing between client and server The synchronous client-side SendHolePunchingMessages and the server worker each implemented the fw_punch config reading, the target resolution and the message pacing. Move them to a StunClient::HolePuncher class which sends due messages without blocking, and express the blocking client-side variant as a loop over it. The single-message StunClient::SendHolePunchingMessage is no longer needed. As a side effect, the client no longer sleeps for fw_punch.delay after the last message, and skips sending if the server address cannot be resolved instead of punching an unresolvable address. Requested by Phosit in the review of #8977. --- source/network/NetServer.cpp | 43 ++------------------- source/network/NetServer.h | 18 ++------- source/network/StunClient.cpp | 70 +++++++++++++++++++++++++++-------- source/network/StunClient.h | 44 +++++++++++++++++++--- 4 files changed, 101 insertions(+), 74 deletions(-) diff --git a/source/network/NetServer.cpp b/source/network/NetServer.cpp index 49be00f5aa..5aee6f4259 100644 --- a/source/network/NetServer.cpp +++ b/source/network/NetServer.cpp @@ -52,7 +52,6 @@ #include "simulation2/system/TurnManager.h" #include -#include #include #include #include @@ -476,11 +475,7 @@ bool CNetServerWorker::RunStep() LOGMESSAGE("Net server: Received connection from %s:%u", hostname, (unsigned int)event.peer->address.port); // The peer connected, so the hole punching for it succeeded or wasn't needed. - std::erase_if(m_HolePunchTargets, [&](const HolePunchTarget& target) - { - return target.address.host == event.peer->address.host && - target.address.port == event.peer->address.port; - }); + m_HolePuncher.RemoveTarget(event.peer->address); // Set up a session object for this peer @@ -1661,42 +1656,10 @@ void CNetServerWorker::ProcessHolePunching(std::vector>&& n if (!m_Host) return; - const std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now(); - for (const std::pair& request : newRequests) - { - // A negative number of messages means punching until the peer connects. - const int numMessages{g_ConfigDB.Get("lobby.fw_punch.num_msg", 3)}; - if (numMessages == 0) - continue; + m_HolePuncher.AddTarget(request.first, request.second); - ENetAddress address; - address.port = request.second; - if (enet_address_set_host(&address, request.first.c_str()) != 0) - { - LOGWARNING("Net server: Failed to resolve hole punching target %s", request.first.c_str()); - continue; - } - m_HolePunchTargets.push_back({address, numMessages, now}); - } - - if (m_HolePunchTargets.empty()) - return; - - const std::chrono::milliseconds delay{g_ConfigDB.Get("lobby.fw_punch.delay", 200)}; - for (HolePunchTarget& target : m_HolePunchTargets) - { - if (now < target.nextSendTime) - continue; - - StunClient::SendHolePunchingMessage(*m_Host, target.address); - if (target.remainingMessages > 0) - --target.remainingMessages; - target.nextSendTime = now + delay; - } - - std::erase_if(m_HolePunchTargets, - [](const HolePunchTarget& target) { return target.remainingMessages == 0; }); + m_HolePuncher.Tick(*m_Host); } diff --git a/source/network/NetServer.h b/source/network/NetServer.h index 5e78a50711..729e776a82 100644 --- a/source/network/NetServer.h +++ b/source/network/NetServer.h @@ -22,9 +22,9 @@ #include "lib/config2.h" #include "lib/types.h" #include "network/NetHost.h" +#include "network/StunClient.h" #include "ps/CStr.h" -#include #include #include #include @@ -325,20 +325,10 @@ private: std::time_t m_LastConnectionCheck{0}; /** - * A peer which should receive hole punching messages until it connects. + * Sends the hole punching messages to the peers which requested them + * until they connect. */ - struct HolePunchTarget - { - ENetAddress address; - /// A negative number means sending until the peer connects. - int remainingMessages; - std::chrono::steady_clock::time_point nextSendTime; - }; - - /** - * The peers currently being sent hole punching messages. - */ - std::vector m_HolePunchTargets; + StunClient::HolePuncher m_HolePuncher; private: // Thread-related stuff: diff --git a/source/network/StunClient.cpp b/source/network/StunClient.cpp index dbcc2c9fc6..36a45f4d41 100644 --- a/source/network/StunClient.cpp +++ b/source/network/StunClient.cpp @@ -27,6 +27,7 @@ #include "ps/CStr.h" #include "ps/ConfigDB.h" +#include #include #include #include @@ -34,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -355,25 +357,63 @@ bool FindPublicIP(ENetHost& transactionHost, CStr& ip, u16& port) void SendHolePunchingMessages(ENetHost& enetClient, const std::string& serverAddress, u16 serverPort) { - // Convert ip string to int64 - ENetAddress addr; - addr.port = serverPort; - enet_address_set_host(&addr, serverAddress.c_str()); + HolePuncher puncher; + if (!puncher.AddTarget(serverAddress, serverPort)) + return; - const int delay{g_ConfigDB.Get("lobby.fw_punch.delay", 200)}; - const int numMsg{g_ConfigDB.Get("lobby.fw_punch.num_msg", 3)}; - - // Send an UDP message from enet host to ip:port - for (int i = 0; i < numMsg || numMsg == -1; ++i) - { - SendStunRequest(enetClient, addr); - std::this_thread::sleep_for(std::chrono::milliseconds(delay)); - } + while (const std::optional nextSendTime = puncher.Tick(enetClient)) + std::this_thread::sleep_until(*nextSendTime); } -void SendHolePunchingMessage(ENetHost& enetClient, const ENetAddress& addr) +bool HolePuncher::AddTarget(const std::string& address, u16 port) { - SendStunRequest(enetClient, addr); + const int numMessages{g_ConfigDB.Get("lobby.fw_punch.num_msg", 3)}; + if (numMessages == 0) + return false; + + ENetAddress addr; + addr.port = port; + if (enet_address_set_host(&addr, address.c_str()) != 0) + { + LOGWARNING("StunClient: failed to resolve hole punching target %s", address.c_str()); + return false; + } + + m_Targets.push_back({addr, numMessages, std::chrono::steady_clock::now()}); + return true; +} + +void HolePuncher::RemoveTarget(const ENetAddress& address) +{ + std::erase_if(m_Targets, [&](const Target& target) + { + return target.address.host == address.host && target.address.port == address.port; + }); +} + +std::optional HolePuncher::Tick(ENetHost& enetClient) +{ + const std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now(); + const std::chrono::milliseconds delay{g_ConfigDB.Get("lobby.fw_punch.delay", 200)}; + + for (Target& target : m_Targets) + { + if (now < target.nextSendTime) + continue; + + SendStunRequest(enetClient, target.address); + if (target.remainingMessages > 0) + --target.remainingMessages; + target.nextSendTime = now + delay; + } + + std::erase_if(m_Targets, [](const Target& target) { return target.remainingMessages == 0; }); + + if (m_Targets.empty()) + return std::nullopt; + + return std::min_element(m_Targets.begin(), m_Targets.end(), + [](const Target& lhs, const Target& rhs) { return lhs.nextSendTime < rhs.nextSendTime; })->nextSendTime; } bool FindLocalIP(CStr& ip) diff --git a/source/network/StunClient.h b/source/network/StunClient.h index 90f165ef8c..44f73cf0f8 100644 --- a/source/network/StunClient.h +++ b/source/network/StunClient.h @@ -22,7 +22,10 @@ #include "lib/external_libraries/enet.h" #include "lib/types.h" +#include +#include #include +#include class CStr8; @@ -47,12 +50,43 @@ bool FindPublicIP(ENetHost& enetClient, CStr8& ip, u16& port); void SendHolePunchingMessages(ENetHost& enetClient, const std::string& serverAddress, u16 serverPort); /** - * Send a single hole punching message to the target address. - * Unlike SendHolePunchingMessages this doesn't block, so the caller - * is responsible for repeating the message at a sensible interval. - * @see SendHolePunchingMessages + * Sends hole punching messages (@see SendHolePunchingMessages) to any number + * of targets without blocking: each call to Tick sends the messages that are + * due and returns when the next one is. Not thread-safe. */ -void SendHolePunchingMessage(ENetHost& enetClient, const ENetAddress& addr); +class HolePuncher +{ +public: + /** + * Resolve the target and schedule lobby.fw_punch.num_msg messages to it + * (a negative number meaning until RemoveTarget is called). + * @return whether messages were scheduled. + */ + bool AddTarget(const std::string& address, u16 port); + + /** + * Stop sending messages to the target, e.g. because it connected. + */ + void RemoveTarget(const ENetAddress& address); + + /** + * Send all due messages from the given ENet host/socket. + * @return the time the next message is due, or std::nullopt if no + * messages remain to be sent. + */ + std::optional Tick(ENetHost& enetClient); + +private: + struct Target + { + ENetAddress address; + /// A negative number means sending until RemoveTarget is called. + int remainingMessages; + std::chrono::steady_clock::time_point nextSendTime; + }; + + std::vector m_Targets; +}; /** * Return the local IP.