Fix: split the UDP blocking of sockets to only the socket involved, and when another thread is busy do not attempt to process the packets of that socket

src/network/network_udp.cpp

@@ -33,9 +33,6 @@
 
 #include "../safeguards.h"
 
-/** Mutex for all out threaded udp resolution and such. */
-static std::mutex _network_udp_mutex;
-
 /** Session key to register ourselves to the master server */
 static uint64 _session_key = 0;
 
@@ -43,14 +40,46 @@ static const std::chrono::minutes ADVERTISE_NORMAL_INTERVAL(15); ///< interval b
 static const std::chrono::seconds ADVERTISE_RETRY_INTERVAL(10); ///< re-advertise when no response after this amount of time.
 static const uint32 ADVERTISE_RETRY_TIMES     =              3; ///< give up re-advertising after this much failed retries
 
-NetworkUDPSocketHandler *_udp_client_socket = nullptr; ///< udp client socket
-NetworkUDPSocketHandler *_udp_server_socket = nullptr; ///< udp server socket
-NetworkUDPSocketHandler *_udp_master_socket = nullptr; ///< udp master socket
-
 static bool _network_udp_server;         ///< Is the UDP server started?
 static uint16 _network_udp_broadcast;    ///< Timeout for the UDP broadcasts.
 static uint8 _network_advertise_retries; ///< The number of advertisement retries we did.
 
+/** Some information about a socket, which exists before the actual socket has been created to provide locking and the likes. */
+struct UDPSocket {
+	const std::string name;                     ///< The name of the socket.
+	std::mutex mutex;                           ///< Mutex for everything that (indirectly) touches the sockets within the handler.
+	NetworkUDPSocketHandler *socket;            ///< The actual socket, which may be nullptr when not initialized yet.
+	std::atomic<int> receive_iterations_locked; ///< The number of receive iterations the mutex was locked.
+
+	UDPSocket(const std::string &name_) : name(name_), socket(nullptr) {}
+
+	void Close()
+	{
+		std::lock_guard<std::mutex> lock(mutex);
+		socket->Close();
+		delete socket;
+		socket = nullptr;
+	}
+
+	void ReceivePackets()
+	{
+		std::unique_lock<std::mutex> lock(mutex, std::defer_lock);
+		if (!lock.try_lock()) {
+			if (++receive_iterations_locked % 32 == 0) {
+				DEBUG(net, 0, "[udp] %s background UDP loop processing appears to be blocked. Your OS may be low on UDP send buffers.", name.c_str());
+			}
+			return;
+		}
+
+		receive_iterations_locked.store(0);
+		socket->ReceivePackets();
+	}
+};
+
+static UDPSocket _udp_client("Client"); ///< udp client socket
+static UDPSocket _udp_server("Server"); ///< udp server socket
+static UDPSocket _udp_master("Master"); ///< udp master socket
+
 /**
  * Helper function doing the actual work for querying the server.
  * @param address The address of the server.
@@ -67,11 +96,11 @@ static void DoNetworkUDPQueryServer(NetworkAddress &address, bool needs_mutex, b
 	item->manually = manually;
 	NetworkGameListAddItemDelayed(item);
 
-	std::unique_lock<std::mutex> lock(_network_udp_mutex, std::defer_lock);
+	std::unique_lock<std::mutex> lock(_udp_client.mutex, std::defer_lock);
 	if (needs_mutex) lock.lock();
 	/* Init the packet */
 	Packet p(PACKET_UDP_CLIENT_FIND_SERVER);
-	if (_udp_client_socket != nullptr) _udp_client_socket->SendPacket(&p, &address);
+	if (_udp_client.socket != nullptr) _udp_client.socket->SendPacket(&p, &address);
 }
 
 /**
@@ -479,7 +508,8 @@ void NetworkUDPQueryMasterServer()
 	p.Send_uint8(NETWORK_MASTER_SERVER_VERSION);
 	p.Send_uint8(SLT_AUTODETECT);
 
-	_udp_client_socket->SendPacket(&p, &out_addr, true);
+	std::lock_guard<std::mutex> lock(_udp_client.mutex);
+	_udp_client.socket->SendPacket(&p, &out_addr, true);
 
 	DEBUG(net, 2, "[udp] master server queried at %s", out_addr.GetAddressAsString().c_str());
 }
@@ -492,7 +522,7 @@ void NetworkUDPSearchGame()
 
 	DEBUG(net, 0, "[udp] searching server");
 
-	NetworkUDPBroadCast(_udp_client_socket);
+	NetworkUDPBroadCast(_udp_client.socket);
 	_network_udp_broadcast = 300; // Stay searching for 300 ticks
 }
 
@@ -512,8 +542,8 @@ static void NetworkUDPRemoveAdvertiseThread()
 	p.Send_uint8 (NETWORK_MASTER_SERVER_VERSION);
 	p.Send_uint16(_settings_client.network.server_port);
 
-	std::lock_guard<std::mutex> lock(_network_udp_mutex);
+	std::lock_guard<std::mutex> lock(_udp_master.mutex);
-	if (_udp_master_socket != nullptr) _udp_master_socket->SendPacket(&p, &out_addr, true);
+	if (_udp_master.socket != nullptr) _udp_master.socket->SendPacket(&p, &out_addr, true);
 }
 
 /**
@@ -564,8 +594,8 @@ static void NetworkUDPAdvertiseThread()
 	p.Send_uint16(_settings_client.network.server_port);
 	p.Send_uint64(_session_key);
 
-	std::lock_guard<std::mutex> lock(_network_udp_mutex);
+	std::lock_guard<std::mutex> lock(_udp_master.mutex);
-	if (_udp_master_socket != nullptr) _udp_master_socket->SendPacket(&p, &out_addr, true);
+	if (_udp_master.socket != nullptr) _udp_master.socket->SendPacket(&p, &out_addr, true);
 }
 
 /**
@@ -607,22 +637,22 @@ void NetworkUDPAdvertise()
 void NetworkUDPInitialize()
 {
 	/* If not closed, then do it. */
-	if (_udp_server_socket != nullptr) NetworkUDPClose();
+	if (_udp_server.socket != nullptr) NetworkUDPClose();
 
 	DEBUG(net, 1, "[udp] initializing listeners");
-	assert(_udp_client_socket == nullptr && _udp_server_socket == nullptr && _udp_master_socket == nullptr);
+	assert(_udp_client.socket == nullptr && _udp_server.socket == nullptr && _udp_master.socket == nullptr);
 
-	std::lock_guard<std::mutex> lock(_network_udp_mutex);
+	std::scoped_lock lock(_udp_client.mutex, _udp_server.mutex, _udp_master.mutex);
 
-	_udp_client_socket = new ClientNetworkUDPSocketHandler();
+	_udp_client.socket = new ClientNetworkUDPSocketHandler();
 
 	NetworkAddressList server;
 	GetBindAddresses(&server, _settings_client.network.server_port);
-	_udp_server_socket = new ServerNetworkUDPSocketHandler(&server);
+	_udp_server.socket = new ServerNetworkUDPSocketHandler(&server);
 
 	server.clear();
 	GetBindAddresses(&server, 0);
-	_udp_master_socket = new MasterNetworkUDPSocketHandler(&server);
+	_udp_master.socket = new MasterNetworkUDPSocketHandler(&server);
 
 	_network_udp_server = false;
 	_network_udp_broadcast = 0;
@@ -632,22 +662,16 @@ void NetworkUDPInitialize()
 /** Start the listening of the UDP server component. */
 void NetworkUDPServerListen()
 {
-	_network_udp_server = _udp_server_socket->Listen();
+	std::lock_guard<std::mutex> lock(_udp_server.mutex);
+	_network_udp_server = _udp_server.socket->Listen();
 }
 
 /** Close all UDP related stuff. */
 void NetworkUDPClose()
 {
-	std::lock_guard<std::mutex> lock(_network_udp_mutex);
+	_udp_client.Close();
-	_udp_server_socket->Close();
+	_udp_server.Close();
-	_udp_master_socket->Close();
+	_udp_master.Close();
-	_udp_client_socket->Close();
-	delete _udp_client_socket;
-	delete _udp_server_socket;
-	delete _udp_master_socket;
-	_udp_client_socket = nullptr;
-	_udp_server_socket = nullptr;
-	_udp_master_socket = nullptr;
 
 	_network_udp_server = false;
 	_network_udp_broadcast = 0;
@@ -657,13 +681,11 @@ void NetworkUDPClose()
 /** Receive the UDP packets. */
 void NetworkBackgroundUDPLoop()
 {
-	std::lock_guard<std::mutex> lock(_network_udp_mutex);
-
 	if (_network_udp_server) {
-		_udp_server_socket->ReceivePackets();
+		_udp_server.ReceivePackets();
-		_udp_master_socket->ReceivePackets();
+		_udp_master.ReceivePackets();
 	} else {
-		_udp_client_socket->ReceivePackets();
+		_udp_client.ReceivePackets();
 		if (_network_udp_broadcast > 0) _network_udp_broadcast--;
 	}
 }