network code robustness

  This should address some quirks of both game and query manager
connections while keeping roughly the same overall design.

4 files changed, 114 insertions, 68 deletions

diff --git a/src/common.hh b/src/common.hh
index 28b48de..4206ed6 100644
--- a/src/common.hh
+++ b/src/common.hh
@@ -103,7 +103,6 @@ STATIC_ASSERT(OS_LINUX);
 //#define MAX_OPEN_CONTAINERS 16
 #define MAX_SPELL_SYLLABLES 10
 
-
 // shm.cc
 // =============================================================================
 void StartGame(void);
@@ -161,6 +160,7 @@ char *Capitals(char *Text);
 extern uint32 RoundNr;
 extern uint32 ServerMilliseconds;
 struct tm GetLocalTimeTM(time_t t);
+int64 GetClockMonotonicMS(void);
 void GetRealTime(int *Hour, int *Minute);
 void GetTime(int *Hour, int *Minute);
 void GetDate(int *Year, int *Cycle, int *Day);
diff --git a/src/communication.cc b/src/communication.cc
index e312c92..37223d2 100644
--- a/src/communication.cc
+++ b/src/communication.cc
@@ -630,9 +630,16 @@ int ReadFromSocket(TConnection *Connection, uint8 *Buffer, int Size){
 			// NOTE(fusion): TCP FIN with no more data to read.
 			break;
 		}else if(errno != EINTR){
-			if(errno != EAGAIN || BytesToRead == Size || Attempts <= 0){
-				return -1;
+			// TODO(fusion): This is probably overkill. We only need a way to
+			// differentiate between an ERROR/TIMEOUT and NODATA.
+			if(errno != EAGAIN){
+				return -errno;
+			}else if(Attempts <= 0){
+				return -ETIMEDOUT;
+			}else if(BytesToRead == Size){
+				return -EAGAIN;
 			}
+
 			DelayThread(0, 100000);
 			Attempts -= 1;
 		}
@@ -640,20 +647,6 @@ int ReadFromSocket(TConnection *Connection, uint8 *Buffer, int Size){
 	return Size - BytesToRead;
 }
 
-bool DrainSocket(TConnection *Connection, int Size){
-	uint8 DiscardBuffer[KB(2)];
-	while(Size > 0){
-		int BytesToRead = std::min<int>(Size, sizeof(DiscardBuffer));
-		int BytesRead = ReadFromSocket(Connection, DiscardBuffer, BytesToRead);
-		if(BytesRead <= 0){
-			return false;
-		}
-		Size -= BytesRead;
-		NetLoad(PACKET_AVERAGE_SIZE_OVERHEAD + BytesRead, false);
-	}
-	return true;
-}
-
 bool CallGameThread(TConnection *Connection){
 	if(GameRunning()){
 		Connection->WaitingForACK = true;
@@ -1144,11 +1137,11 @@ bool ReceiveCommand(TConnection *Connection){
 			// more data to read.
 			return false;
 		}else if(BytesRead < 0){
-			// NOTE(fusion): There was either no data to be read or a connection
-			// error. Since we can't differ, let the connection be closed elsewhere
-			// in case of errors. This is the only path that will not cause the
-			// connection to be closed, aside from successfully reading a packet.
-			return true;
+			// NOTE(fusion): There was either no data, some data then a timeout,
+			// or a connection error. We only want to keep the connection open
+			// in case there was no data, to allow us to return to the connection
+			// loop in a consistent state.
+			return (BytesRead == -EAGAIN);
 		}
 
 		// NOTE(fusion): It doesn't make sense to continue if we couldn't read
@@ -1164,15 +1157,12 @@ bool ReceiveCommand(TConnection *Connection){
 		// have a few helper functions to assist with buffer reading.
 		int Size = ((uint16)Help[0] | ((uint16)Help[1] << 8));
 
+		// NOTE(fusion): It doesn't make sense to continue if the client didn't
+		// correctly size its packet.
 		if(Size == 0 || Size > (int)sizeof(Connection->InData)){
-			// TODO(fusion): We should definitely close the connection here.
-			// Nevertheless, the original handling of this edge case was just
-			// terrible, reading from the socket recklessly until at least `Size`
-			// bytes were discarded. I replaced it with a small helper function
-			// function `DrainSocket`.
 			print(3, "Paket an Socket %d zu groß oder leer, wird verworfen (%d Bytes)\n",
 					Connection->GetSocket(), Size);
-			return DrainSocket(Connection, Size);
+			return false;
 		}
 
 		BytesRead = ReadFromSocket(Connection, &Connection->InData[0], Size);
@@ -1269,38 +1259,45 @@ void CommunicationThread(int Socket){
 	Connection->Connect(Socket);
 	Connection->WaitingForACK = false;
 
-	struct f_owner_ex FOwnerEx = {};
-	FOwnerEx.type = F_OWNER_TID;
-	FOwnerEx.pid = Connection->ThreadID;
-	if(fcntl(Socket, F_SETOWN_EX, &FOwnerEx) == -1){
-		error("CommunicationThread: F_SETOWN_EX fehlgeschlagen für Socket %d.\n", Socket);
-		if(close(Socket) == -1){
-			error("CommunicationThread: Fehler %d beim Schließen der Socket (2).\n", errno);
+	{
+		struct f_owner_ex FOwnerEx = {};
+		FOwnerEx.type = F_OWNER_TID;
+		FOwnerEx.pid = Connection->ThreadID;
+		if(fcntl(Socket, F_SETOWN_EX, &FOwnerEx) == -1){
+			error("CommunicationThread: F_SETOWN_EX fehlgeschlagen für Socket %d.\n", Socket);
+			if(close(Socket) == -1){
+				error("CommunicationThread: Fehler %d beim Schließen der Socket (2).\n", errno);
+			}
+			Connection->Free();
+			return;
 		}
-		Connection->Free();
-		return;
 	}
 
-	if(fcntl(Socket, F_SETFL, (O_NONBLOCK | O_ASYNC)) == -1){
-		error("ConnectionThread: F_SETFL fehlgeschlagen für Socket %d.\n", Socket);
-		if(close(Socket) == -1){
-			error("CommunicationThread: Fehler %d beim Schließen der Socket (3).\n", errno);
+	{
+		int SocketFlags = fcntl(Socket, F_GETFL);
+		if(SocketFlags == -1 || fcntl(Socket, F_SETFL, (SocketFlags | O_NONBLOCK | O_ASYNC)) == -1){
+			error("ConnectionThread: F_SETFL fehlgeschlagen für Socket %d.\n", Socket);
+			if(close(Socket) == -1){
+				error("CommunicationThread: Fehler %d beim Schließen der Socket (3).\n", errno);
+			}
+			Connection->Free();
+			return;
 		}
-		Connection->Free();
-		return;
 	}
 
 	// NOTE(fusion): In some systems, the accepted socket will inherit TCP_NODELAY
 	// from the acceptor, making this next call redundant then. Nevertheless it is
 	// probably better to set it anyways to be sure.
-	int NoDelay = 1;
-	if(setsockopt(Socket, IPPROTO_TCP, TCP_NODELAY, &NoDelay, sizeof(NoDelay)) == -1){
-		error("ConnectionThread: Failed to set TCP_NODELAY=1 on socket %d.\n", Socket);
-		if(close(Socket) == -1){
-			error("CommunicationThread: Fehler %d beim Schließen der Socket (3.5).\n", errno);
+	{
+		int NoDelay = 1;
+		if(setsockopt(Socket, IPPROTO_TCP, TCP_NODELAY, &NoDelay, sizeof(NoDelay)) == -1){
+			error("ConnectionThread: Failed to set TCP_NODELAY=1 on socket %d.\n", Socket);
+			if(close(Socket) == -1){
+				error("CommunicationThread: Fehler %d beim Schließen der Socket (3.5).\n", errno);
+			}
+			Connection->Free();
+			return;
 		}
-		Connection->Free();
-		return;
 	}
 
 	sigset_t SignalSet;
@@ -1577,8 +1574,7 @@ void InitCommunication(void){
 		throw "cannot load RSA key";
 	}
 
-	OpenSocket();
-	if(TCPSocket == -1){
+	if(!OpenSocket()){
 		throw "cannot open socket";
 	}
 
diff --git a/src/query.cc b/src/query.cc
index 3b62ecd..57665fe 100644
--- a/src/query.cc
+++ b/src/query.cc
@@ -2,6 +2,7 @@
 #include "config.hh"
 
 #include <arpa/inet.h>
+#include <fcntl.h>
 #include <netdb.h>
 #include <netinet/in.h>
 #include <poll.h>
@@ -66,7 +67,7 @@ bool ResolveHostNameAddress(const char *HostName, in_addr_t *OutAddr){
 void TQueryManagerConnection::connect(void){
 	for(int i = 0; i < NumberOfQueryManagers; i += 1){
 		in_addr_t Addr = inet_addr(QUERY_MANAGER[i].Host);
-		if(Addr == 0xFFFFFFFF && !ResolveHostNameAddress(QUERY_MANAGER[i].Host, &Addr)){
+		if(Addr == INADDR_NONE && !ResolveHostNameAddress(QUERY_MANAGER[i].Host, &Addr)){
 			print(2, "TQueryManagerConnection::connect: Kann Rechnernamen nicht auflösen.\n");
 			continue;
 		}
@@ -82,10 +83,24 @@ void TQueryManagerConnection::connect(void){
 		QueryManagerAddress.sin_port = htons(QUERY_MANAGER[i].Port);
 		QueryManagerAddress.sin_addr.s_addr = Addr;
 		if(::connect(this->Socket, (struct sockaddr*)&QueryManagerAddress, sizeof(QueryManagerAddress)) == -1){
+			print(2, "TQueryManagerConnection::connect: Kann Verbindung nicht herstellen.\n");
 			this->disconnect();
 			continue;
 		}
 
+		// NOTE(fusion): Make socket non-blocking AFTER connecting. This is to
+		// better align the behaviour of both TQueryManagerConnection::read and
+		// TQueryManagerConnection::write.
+		{
+			int SocketFlags = fcntl(this->Socket, F_GETFL);
+			if(SocketFlags == -1 || fcntl(this->Socket, F_SETFL, (SocketFlags | O_NONBLOCK)) == -1){
+				error("TQueryManagerConnection::connect: Failed to set socket as non-blocking: (%d) %s\n",
+						errno, strerrordesc_np(errno));
+				this->disconnect();
+				continue;
+			}
+		}
+
 		this->prepareQuery(0);
 		this->sendByte((uint8)ApplicationType);
 		this->sendString(QUERY_MANAGER[i].Password);
@@ -128,11 +143,11 @@ int TQueryManagerConnection::write(const uint8 *Buffer, int Size){
 			WritePtr += BytesWritten;
 		}else if(BytesWritten == 0){
 			break;
-		}else{
-			// TODO(fusion): We don't set the socket as non blocking so I don't
-			// think we can even get `EAGAIN` here.
-			if(errno != EAGAIN || Attempts <= 0){
-				return -1;
+		}else if(errno != EINTR){
+			if(errno != EAGAIN){
+				return -errno;
+			}else if(Attempts <= 0){
+				return -ETIMEDOUT;
 			}
 
 			DelayThread(0, 100000);
@@ -143,17 +158,33 @@ int TQueryManagerConnection::write(const uint8 *Buffer, int Size){
 }
 
 int TQueryManagerConnection::read(uint8 *Buffer, int Size, int Timeout){
-	int Attempts = 50;
-	int BytesToRead = Size;
-	uint8 *ReadPtr = Buffer;
-	while(BytesToRead > 0){
+	// NOTE(fusion): Wait until there is any inbound data then read everything
+	// in one go. Data from the query manager is sent in a burst so we don't need
+	// to be polling every step along the way.
+	int TimeoutMS = Timeout * 1000;
+	int64 Deadline = GetClockMonotonicMS() + TimeoutMS;
+	while(true){
 		struct pollfd pollfd = {};
-		pollfd.fd = this->Socket;
+		pollfd.fd = Socket;
 		pollfd.events = POLLIN;
-		if(poll(&pollfd, 1, Timeout * 1000) != 1){
-			return -2;
+		pollfd.revents = 0;
+		int ret = poll(&pollfd, 1, TimeoutMS);
+		if(ret == 1) break;
+		if(ret == 0) return -ETIMEDOUT;
+		if(ret == -1 && errno != EINTR) return -errno;
+
+		// NOTE(fusion): We should get a timeout before this turns negative, but
+		// just in case... We don't want poll to block indefinitely...
+		TimeoutMS = (int)(Deadline - GetClockMonotonicMS());
+		if(TimeoutMS < 0){
+			return -ETIMEDOUT;
 		}
+	}
 
+	int Attempts = 50;
+	int BytesToRead = Size;
+	uint8 *ReadPtr = Buffer;
+	while(BytesToRead > 0){
 		int BytesRead = ::read(this->Socket, ReadPtr, BytesToRead);
 		if(BytesRead > 0){
 			BytesToRead -= BytesRead;
@@ -161,8 +192,10 @@ int TQueryManagerConnection::read(uint8 *Buffer, int Size, int Timeout){
 		}else if(BytesRead == 0){
 			break;
 		}else if(errno != EINTR){
-			if(errno != EAGAIN || BytesToRead == Size || Attempts <= 0){
-				return -1;
+			if(errno != EAGAIN){
+				return -errno;
+			}else if(Attempts <= 0){
+				return -ETIMEDOUT;
 			}
 
 			DelayThread(0, 100000);
@@ -347,7 +380,7 @@ int TQueryManagerConnection::executeQuery(int Timeout, bool AutoReconnect){
 		int BytesRead = this->read(Help, 2, Timeout);
 		if(BytesRead != 2){
 			this->disconnect();
-			if(BytesRead == -2 || Attempt >= MaxAttempts){
+			if(BytesRead == -ETIMEDOUT || Attempt >= MaxAttempts){
 				return QUERY_STATUS_FAILED;
 			}
 			continue;
diff --git a/src/time.cc b/src/time.cc
index b2ba545..c615462 100644
--- a/src/time.cc
+++ b/src/time.cc
@@ -17,6 +17,23 @@ struct tm GetLocalTimeTM(time_t t){
 	return result;
 }
 
+int64 GetClockMonotonicMS(void){
+#if OS_WINDOWS
+	LARGE_INTEGER Counter, Frequency;
+	QueryPerformanceCounter(&Counter);
+	QueryPerformanceFrequency(&Frequency);
+	return (int64)((Counter.QuadPart * 1000) / Frequency.QuadPart);
+#else
+	// NOTE(fusion): The coarse monotonic clock has a larger resolution but is
+	// supposed to be faster, even avoiding system calls in some cases. It should
+	// be fine for millisecond precision which is what we're using.
+	struct timespec Time;
+	clock_gettime(CLOCK_MONOTONIC_COARSE, &Time);
+	return ((int64)Time.tv_sec * 1000)
+		+ ((int64)Time.tv_nsec / 1000000);
+#endif
+}
+
 void GetRealTime(int *Hour, int *Minute){
 	struct tm LocalTime = GetLocalTimeTM(time(NULL));
 	*Hour = LocalTime.tm_hour;