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

@@ -52,7 +52,6 @@
 #include "simulation2/system/TurnManager.h"
 
 #include <algorithm>
-#include <chrono>
 #include <cstring>
 #include <functional>
 #include <iterator>
@@ -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<std::pair<CStr, u16>>&& n
 	if (!m_Host)
 		return;
 
-	const std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now();
-
 	for (const std::pair<CStr, u16>& 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);
 }
 
 

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 <chrono>
 #include <ctime>
 #include <js/RootingAPI.h>
 #include <js/TypeDecls.h>
@@ -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<HolePunchTarget> m_HolePunchTargets;
+	StunClient::HolePuncher m_HolePuncher;
 
 private:
 	// Thread-related stuff:

source/network/StunClient.cpp

@@ -27,6 +27,7 @@
 #include "ps/CStr.h"
 #include "ps/ConfigDB.h"
 
+#include <algorithm>
 #include <bit>
 #include <cerrno>
 #include <chrono>
@@ -34,6 +35,7 @@
 #include <cstddef>
 #include <cstdlib>
 #include <cstring>
+#include <optional>
 #include <thread>
 #include <type_traits>
 #include <vector>
@@ -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<std::chrono::steady_clock::time_point> 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<std::chrono::steady_clock::time_point> 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)

source/network/StunClient.h

@@ -22,7 +22,10 @@
 #include "lib/external_libraries/enet.h"
 #include "lib/types.h"
 
+#include <chrono>
+#include <optional>
 #include <string>
+#include <vector>
 
 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<std::chrono::steady_clock::time_point> 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<Target> m_Targets;
+};
 
 /**
  * Return the local IP.