(svn r15159) -Fix: move the UDP queries that resolve a hostname into threads so they don't freeze OpenTTD when for example the network connection got severed. Thanks to glx for writing the mutex implementation for Windows.

src/network/network_gamelist.cpp

@@ -11,6 +11,8 @@
 #include "../debug.h"
 #include "../newgrf_config.h"
 #include "../core/alloc_func.hpp"
+#include "../thread.h"
+#include "../string_func.h"
 #include "network_internal.h"
 #include "core/game.h"
 #include "network_udp.h"
@@ -19,6 +21,46 @@
 
 NetworkGameList *_network_game_list = NULL;
 
+ThreadMutex *_network_game_list_mutex = ThreadMutex::New();
+NetworkGameList *_network_game_delayed_insertion_list = NULL;
+
+/** Add a new item to the linked gamelist, but do it delayed in the next tick
+ * or so to prevent race conditions.
+ * @param item the item to add. Will be freed once added.
+ */
+void NetworkGameListAddItemDelayed(NetworkGameList *item)
+{
+	_network_game_list_mutex->BeginCritical();
+	item->next = _network_game_delayed_insertion_list;
+	_network_game_delayed_insertion_list = item;
+	_network_game_list_mutex->EndCritical();
+}
+
+/** Perform the delayed (thread safe) insertion into the game list */
+static void NetworkGameListHandleDelayedInsert()
+{
+	_network_game_list_mutex->BeginCritical();
+	while (_network_game_delayed_insertion_list != NULL) {
+		NetworkGameList *ins_item = _network_game_delayed_insertion_list;
+		_network_game_delayed_insertion_list = ins_item->next;
+
+		NetworkGameList *item = NetworkGameListAddItem(ins_item->ip, ins_item->port);
+
+		if (item != NULL) {
+			if (StrEmpty(item->info.server_name)) {
+				memset(&item->info, 0, sizeof(item->info));
+				strecpy(item->info.server_name, ins_item->info.server_name, lastof(item->info.server_name));
+				strecpy(item->info.hostname, ins_item->info.hostname, lastof(item->info.hostname));
+				item->online = false;
+			}
+			item->manually = ins_item->manually;
+			UpdateNetworkGameWindow(false);
+		}
+		free(ins_item);
+	}
+	_network_game_list_mutex->EndCritical();
+}
+
 /** Add a new item to the linked gamelist. If the IP and Port match
  * return the existing item instead of adding it again
  * @param ip the IP-address (inet_addr) of the to-be added item
@@ -36,8 +78,7 @@ NetworkGameList *NetworkGameListAddItem(uint32 ip, uint16 port)
 		prev_item = item;
 	}
 
-	item = MallocT<NetworkGameList>(1);
+	item = CallocT<NetworkGameList>(1);
-	memset(item, 0, sizeof(*item));
 	item->next = NULL;
 	item->ip = ip;
 	item->port = port;
@@ -91,6 +132,8 @@ enum {
 /** Requeries the (game) servers we have not gotten a reply from */
 void NetworkGameListRequery()
 {
+	NetworkGameListHandleDelayedInsert();
+
 	static uint8 requery_cnt = 0;
 
 	if (++requery_cnt < REQUERY_EVERY_X_GAMELOOPS) return;

src/network/network_gamelist.h

@@ -21,6 +21,7 @@ struct NetworkGameList {
 /** Game list of this client */
 extern NetworkGameList *_network_game_list;
 
+void NetworkGameListAddItemDelayed(NetworkGameList *item);
 NetworkGameList *NetworkGameListAddItem(uint32 ip, uint16 port);
 void NetworkGameListRemoveItem(NetworkGameList *remove);
 void NetworkGameListRequery();

src/network/network_udp.cpp

@@ -20,14 +20,18 @@
 #include "../variables.h"
 #include "../newgrf_config.h"
 #include "../core/endian_func.hpp"
+#include "../core/alloc_func.hpp"
 #include "../string_func.h"
 #include "../company_base.h"
 #include "../company_func.h"
 #include "../settings_type.h"
+#include "../thread.h"
 #include "../rev.h"
 
 #include "core/udp.h"
 
+ThreadMutex *_network_udp_mutex = ThreadMutex::New();
+
 enum {
 	ADVERTISE_NORMAL_INTERVAL = 30000, // interval between advertising in ticks (15 minutes)
 	ADVERTISE_RETRY_INTERVAL  =   300, // readvertise when no response after this many ticks (9 seconds)
@@ -352,9 +356,11 @@ void NetworkUDPCloseAll()
 {
 	DEBUG(net, 1, "[udp] closed listeners");
 
+	_network_udp_mutex->BeginCritical();
 	_udp_server_socket->Close();
 	_udp_master_socket->Close();
 	_udp_client_socket->Close();
+	_network_udp_mutex->EndCritical();
 
 	_network_udp_server = false;
 	_network_udp_broadcast = 0;
@@ -420,55 +426,66 @@ void NetworkUDPSearchGame()
 	_network_udp_broadcast = 300; // Stay searching for 300 ticks
 }
 
+/** Simpler wrapper struct for NetworkUDPQueryServerThread */
+struct NetworkUDPQueryServerInfo : NetworkAddress {
+	bool manually; ///< Did we connect manually or not?
+	NetworkUDPQueryServerInfo(const NetworkAddress &address, bool manually) :
+		NetworkAddress(address),
+		manually(manually)
+	{
+	}
+};
+
+/**
+ * Threaded part for resolving the IP of a server and querying it.
+ * @param pntr the NetworkUDPQueryServerInfo.
+ */
+void NetworkUDPQueryServerThread(void *pntr)
+{
+	NetworkUDPQueryServerInfo *info = (NetworkUDPQueryServerInfo*)pntr;
+
+	struct sockaddr_in out_addr;
+	out_addr.sin_family = AF_INET;
+	out_addr.sin_port = htons(info->GetPort());
+	out_addr.sin_addr.s_addr = info->GetIP();
+
+	/* Clear item in gamelist */
+	NetworkGameList *item = CallocT<NetworkGameList>(1);
+	item->ip = info->GetIP();
+	item->port = info->GetPort();
+	strecpy(item->info.server_name, info->GetHostname(), lastof(item->info.server_name));
+	strecpy(item->info.hostname, info->GetHostname(), lastof(item->info.hostname));
+	item->manually = info->manually;
+	NetworkGameListAddItemDelayed(item);
+
+	_network_udp_mutex->BeginCritical();
+	/* Init the packet */
+	Packet p(PACKET_UDP_CLIENT_FIND_SERVER);
+	if (_udp_client_socket != NULL) _udp_client_socket->SendPacket(&p, &out_addr);
+	_network_udp_mutex->EndCritical();
+
+	delete info;
+}
+
 void NetworkUDPQueryServer(NetworkAddress address, bool manually)
 {
-	struct sockaddr_in out_addr;
-	NetworkGameList *item;
-
 	// No UDP-socket yet..
 	if (!_udp_client_socket->IsConnected()) {
 		if (!_udp_client_socket->Listen(0, 0, true)) return;
 	}
 
-	out_addr.sin_family = AF_INET;
+	NetworkUDPQueryServerInfo *info = new NetworkUDPQueryServerInfo(address, manually);
-	out_addr.sin_port = htons(address.GetPort());
+	if (address.IsResolved() || !ThreadObject::New(NetworkUDPQueryServerThread, info)) {
-	out_addr.sin_addr.s_addr = address.GetIP();
+		NetworkUDPQueryServerThread(info);
-
-	// Clear item in gamelist
-	item = NetworkGameListAddItem(address.GetIP(), address.GetPort());
-	if (item == NULL) return;
-
-	if (StrEmpty(item->info.server_name)) {
-		memset(&item->info, 0, sizeof(item->info));
-		strecpy(item->info.server_name, address.GetHostname(), lastof(item->info.server_name));
-		strecpy(item->info.hostname, address.GetHostname(), lastof(item->info.hostname));
-		item->online = false;
 	}
-	item->manually = manually;
-
-	// Init the packet
-	Packet p(PACKET_UDP_CLIENT_FIND_SERVER);
-	_udp_client_socket->SendPacket(&p, &out_addr);
-
-	UpdateNetworkGameWindow(false);
 }
 
-/* Remove our advertise from the master-server */
+void NetworkUDPRemoveAdvertiseThread(void *pntr)
-void NetworkUDPRemoveAdvertise()
 {
-	struct sockaddr_in out_addr;
-
-	/* Check if we are advertising */
-	if (!_networking || !_network_server || !_network_udp_server) return;
-
-	/* check for socket */
-	if (!_udp_master_socket->IsConnected()) {
-		if (!_udp_master_socket->Listen(_network_server_bind_ip, 0, false)) return;
-	}
-
 	DEBUG(net, 1, "[udp] removing advertise from master server");
 
 	/* Find somewhere to send */
+	struct sockaddr_in out_addr;
 	out_addr.sin_family = AF_INET;
 	out_addr.sin_port = htons(NETWORK_MASTER_SERVER_PORT);
 	out_addr.sin_addr.s_addr = NetworkResolveHost(NETWORK_MASTER_SERVER_HOST);
@@ -478,15 +495,54 @@ void NetworkUDPRemoveAdvertise()
 	/* Packet is: Version, server_port */
 	p.Send_uint8 (NETWORK_MASTER_SERVER_VERSION);
 	p.Send_uint16(_settings_client.network.server_port);
-	_udp_master_socket->SendPacket(&p, &out_addr);
+
+	_network_udp_mutex->BeginCritical();
+	if (_udp_master_socket != NULL) _udp_master_socket->SendPacket(&p, &out_addr);
+	_network_udp_mutex->EndCritical();
+}
+
+/* Remove our advertise from the master-server */
+void NetworkUDPRemoveAdvertise()
+{
+	/* Check if we are advertising */
+	if (!_networking || !_network_server || !_network_udp_server) return;
+
+	/* check for socket */
+	if (!_udp_master_socket->IsConnected()) {
+		if (!_udp_master_socket->Listen(_network_server_bind_ip, 0, false)) return;
+	}
+
+	if (!ThreadObject::New(NetworkUDPRemoveAdvertiseThread, NULL)) {
+		NetworkUDPRemoveAdvertiseThread(NULL);
+	}
+}
+
+void NetworkUDPAdvertiseThread(void *pntr)
+{
+	/* Find somewhere to send */
+	struct sockaddr_in out_addr;
+	out_addr.sin_family = AF_INET;
+	out_addr.sin_port = htons(NETWORK_MASTER_SERVER_PORT);
+	out_addr.sin_addr.s_addr = NetworkResolveHost(NETWORK_MASTER_SERVER_HOST);
+
+	DEBUG(net, 1, "[udp] advertising to master server");
+
+	/* Send the packet */
+	Packet p(PACKET_UDP_SERVER_REGISTER);
+	/* Packet is: WELCOME_MESSAGE, Version, server_port */
+	p.Send_string(NETWORK_MASTER_SERVER_WELCOME_MESSAGE);
+	p.Send_uint8 (NETWORK_MASTER_SERVER_VERSION);
+	p.Send_uint16(_settings_client.network.server_port);
+
+	_network_udp_mutex->BeginCritical();
+	if (_udp_master_socket != NULL) _udp_master_socket->SendPacket(&p, &out_addr);
+	_network_udp_mutex->EndCritical();
 }
 
 /* Register us to the master server
      This function checks if it needs to send an advertise */
 void NetworkUDPAdvertise()
 {
-	struct sockaddr_in out_addr;
-
 	/* Check if we should send an advertise */
 	if (!_networking || !_network_server || !_network_udp_server || !_settings_client.network.server_advertise)
 		return;
@@ -514,44 +570,37 @@ void NetworkUDPAdvertise()
 	_network_advertise_retries--;
 	_network_last_advertise_frame = _frame_counter;
 
-	/* Find somewhere to send */
+	if (!ThreadObject::New(NetworkUDPAdvertiseThread, NULL)) {
-	out_addr.sin_family = AF_INET;
+		NetworkUDPAdvertiseThread(NULL);
-	out_addr.sin_port = htons(NETWORK_MASTER_SERVER_PORT);
+	}
-	out_addr.sin_addr.s_addr = NetworkResolveHost(NETWORK_MASTER_SERVER_HOST);
-
-	DEBUG(net, 1, "[udp] advertising to master server");
-
-	/* Send the packet */
-	Packet p(PACKET_UDP_SERVER_REGISTER);
-	/* Packet is: WELCOME_MESSAGE, Version, server_port */
-	p.Send_string(NETWORK_MASTER_SERVER_WELCOME_MESSAGE);
-	p.Send_uint8 (NETWORK_MASTER_SERVER_VERSION);
-	p.Send_uint16(_settings_client.network.server_port);
-	_udp_master_socket->SendPacket(&p, &out_addr);
 }
 
 void NetworkUDPInitialize()
 {
 	assert(_udp_client_socket == NULL && _udp_server_socket == NULL && _udp_master_socket == NULL);
 
+	_network_udp_mutex->BeginCritical();
 	_udp_client_socket = new ClientNetworkUDPSocketHandler();
 	_udp_server_socket = new ServerNetworkUDPSocketHandler();
 	_udp_master_socket = new MasterNetworkUDPSocketHandler();
 
 	_network_udp_server = false;
 	_network_udp_broadcast = 0;
+	_network_udp_mutex->EndCritical();
 }
 
 void NetworkUDPShutdown()
 {
 	NetworkUDPCloseAll();
 
+	_network_udp_mutex->BeginCritical();
 	delete _udp_client_socket;
 	delete _udp_server_socket;
 	delete _udp_master_socket;
 	_udp_client_socket = NULL;
 	_udp_server_socket = NULL;
 	_udp_master_socket = NULL;
+	_network_udp_mutex->EndCritical();
 }
 
 #endif /* ENABLE_NETWORK */

src/thread.h

@@ -40,4 +40,27 @@ public:
 	static bool New(OTTDThreadFunc proc, void *param, ThreadObject **thread = NULL);
 };
 
+/**
+ * Cross-platform Mutex
+ */
+class ThreadMutex {
+public:
+	static ThreadMutex *New();
+
+	/**
+	 * Virtual Destructor to avoid compiler warnings.
+	 */
+	virtual ~ThreadMutex() {};
+
+	/**
+	 * Begin the critical section
+	 */
+	virtual void BeginCritical() = 0;
+
+	/**
+	 * End of the critical section
+	 */
+	virtual void EndCritical() = 0;
+};
+
 #endif /* THREAD_H */

src/thread_none.cpp

@@ -10,3 +10,15 @@
 	if (thread != NULL) *thread = NULL;
 	return false;
 }
+
+/** Mutex that doesn't do locking because it ain't needed when there're no threads */
+class ThreadMutex_None : ThreadMutex {
+public:
+	virtual void BeginCritical() {}
+	virtual void EndCritical() {}
+};
+
+/* static */ ThreadMutex *ThreadMutex::New()
+{
+	return new ThreadMutex_None;
+}

src/thread_pthread.cpp

@@ -83,3 +83,37 @@ private:
 	if (thread != NULL) *thread = to;
 	return true;
 }
+
+/**
+ * POSIX pthread version of ThreadMutex.
+ */
+class ThreadMutex_pthread : public ThreadMutex {
+private:
+	pthread_mutex_t mutex;
+
+public:
+	ThreadMutex_pthread()
+	{
+		pthread_mutex_init(&this->mutex, NULL);
+	}
+
+	/* virtual */ ~ThreadMutex_pthread()
+	{
+		pthread_mutex_destroy(&this->mutex);
+	}
+
+	/* virtual */ void BeginCritical()
+	{
+		pthread_mutex_lock(&this->mutex);
+	}
+
+	/* virtual */ void EndCritical()
+	{
+		pthread_mutex_unlock(&this->mutex);
+	}
+};
+
+/* static */ ThreadMutex *ThreadMutex::New()
+{
+	return new ThreadMutex_pthread();
+}

src/thread_win32.cpp

@@ -93,3 +93,37 @@ private:
 	if (thread != NULL) *thread = to;
 	return true;
 }
+
+/**
+ * Win32 thread version of ThreadMutex.
+ */
+class ThreadMutex_Win32 : public ThreadMutex {
+private:
+	CRITICAL_SECTION critical_section;
+
+public:
+	ThreadMutex_Win32()
+	{
+		InitializeCriticalSection(&this->critical_section);
+	}
+
+	/* virtual */ ~ThreadMutex_Win32()
+	{
+		DeleteCriticalSection(&this->critical_section);
+	}
+
+	/* virtual */ void BeginCritical()
+	{
+		EnterCriticalSection(&this->critical_section);
+	}
+
+	/* virtual */ void EndCritical()
+	{
+		LeaveCriticalSection(&this->critical_section);
+	}
+};
+
+/* static */ ThreadMutex *ThreadMutex::New()
+{
+	return new ThreadMutex_Win32();
+}