Merge pull request #989 from UnknownShadow200/SocketsRewrite

Rewrite sockets API to be a bit simpler

src/Platform.h

@@ -233,25 +233,23 @@ CC_API void  Waitable_WaitFor(void* handle, cc_uint32 milliseconds);
 /* Calls SysFonts_Register on each font that is available on this platform. */
 void Platform_LoadSysFonts(void);
 
-/* Returns how much data is available to be read from the given socket. */
+/* Checks if the given socket is currently readable (i.e. has data available to read) */
-CC_API cc_result Socket_Available(cc_socket s, int* available);
+/* NOTE: A closed socket is also considered readable */
-/* Returns (and resets) the last error generated by the given socket. */
+cc_result Socket_CheckReadable(cc_socket s, cc_bool* readable);
-CC_API cc_result Socket_GetError(cc_socket s, cc_result* result);
+/* Checks if the given socket is currently writable (i.e. has finished connecting) */
+cc_result Socket_CheckWritable(cc_socket s, cc_bool* writable);
 /* Returns non-zero if the given address is valid for a socket to connect to */
-CC_API int Socket_ValidAddress(const cc_string* address);
+int Socket_ValidAddress(const cc_string* address);
 
 /* Allocates a new non-blocking socket and then begins connecting to the given address:port. */
-CC_API cc_result Socket_Connect(cc_socket* s, const cc_string* address, int port);
+cc_result Socket_Connect(cc_socket* s, const cc_string* address, int port);
 /* Attempts to read data from the given socket. */
-CC_API cc_result Socket_Read(cc_socket s, cc_uint8* data, cc_uint32 count, cc_uint32* modified);
+/* NOTE: A closed socket may set modified to 0, but still return 'success' (i.e. 0) */
+cc_result Socket_Read(cc_socket s, cc_uint8* data, cc_uint32 count, cc_uint32* modified);
 /* Attempts to write data to the given socket. */
-CC_API cc_result Socket_Write(cc_socket s, const cc_uint8* data, cc_uint32 count, cc_uint32* modified);
+cc_result Socket_Write(cc_socket s, const cc_uint8* data, cc_uint32 count, cc_uint32* modified);
 /* Attempts to close the given socket. */
-CC_API void Socket_Close(cc_socket s);
+void Socket_Close(cc_socket s);
-/* Attempts to poll the given socket for readability or writability. */
-/* NOTE: A closed socket is still considered readable. */
-/* NOTE: A socket is considered writable once it has finished connecting. */
-CC_API cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success);
 
 #ifdef CC_BUILD_MOBILE
 void Platform_ShareScreenshot(const cc_string* filename);

src/Platform_Posix.c

@@ -483,15 +483,6 @@ union SocketAddress {
 	struct sockaddr_in6 v6;
 };
 
-cc_result Socket_Available(cc_socket s, int* available) {
-	return ioctl(s, FIONREAD, available);
-}
-
-cc_result Socket_GetError(cc_socket s, cc_result* result) {
-	socklen_t resultSize = sizeof(cc_result);
-	return getsockopt(s, SOL_SOCKET, SO_ERROR, result, &resultSize);
-}
-
 static int ParseHost(union SocketAddress* addr, const char* host) {
 	struct addrinfo hints = { 0 };
 	struct addrinfo* result;
@@ -577,7 +568,7 @@ void Socket_Close(cc_socket s) {
 
 #if defined CC_BUILD_DARWIN
 /* poll is broken on old OSX apparently https://daniel.haxx.se/docs/poll-vs-select.html */
-cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
+static cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
 	fd_set set;
 	struct timeval time = { 0 };
 	int selectCount;
@@ -596,7 +587,7 @@ cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
 }
 #else
 #include <poll.h>
-cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
+static cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
 	struct pollfd pfd;
 	int flags;
 
@@ -611,6 +602,20 @@ cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
 }
 #endif
 
+cc_result Socket_CheckReadable(cc_socket s, cc_bool* readable) {
+	return Socket_Poll(s, SOCKET_POLL_READ, readable);
+}
+
+cc_result Socket_CheckWritable(cc_socket s, cc_bool* writable) {
+	socklen_t resultSize;
+	cc_result res = Socket_Poll(s, SOCKET_POLL_WRITE, writable);
+	if (res || *writable) return res;
+
+	/* https://stackoverflow.com/questions/29479953/so-error-value-after-successful-socket-operation */
+	getsockopt(s, SOL_SOCKET, SO_ERROR, &res, &resultSize);
+	return res;
+}
+
 
 /*########################################################################################################################*
 *-----------------------------------------------------Process/Module------------------------------------------------------*

src/Platform_Web.c

@@ -254,30 +254,6 @@ void Platform_LoadSysFonts(void) { }
 extern void interop_InitSockets(void);
 int Socket_ValidAddress(const cc_string* address) { return true; }
 
-extern int interop_SocketGetPending(int sock);
-cc_result Socket_Available(cc_socket s, int* available) {
-	int res = interop_SocketGetPending(s);
-	/* returned result is negative for error */
-
-	if (res >= 0) {
-		*available = res; return 0;
-	} else {
-		*available = 0; return -res;
-	}
-}
-
-extern int interop_SocketGetError(int sock);
-cc_result Socket_GetError(cc_socket s, cc_result* result) {
-	int res = interop_SocketGetError(s);
-	/* returned result is negative for error */
-
-	if (res >= 0) {
-		*result = res; return 0;
-	} else {
-		*result = 0; return -res;
-	}
-}
-
 extern int interop_SocketCreate(void);
 extern int interop_SocketConnect(int sock, const char* addr, int port);
 cc_result Socket_Connect(cc_socket* s, const cc_string* address, int port) {
@@ -295,20 +271,22 @@ cc_result Socket_Connect(cc_socket* s, const cc_string* address, int port) {
 }
 
 extern int interop_SocketRecv(int sock, void* data, int len);
-cc_result Socket_Read(cc_socket s, cc_uint8* data, cc_uint32 count, cc_uint32* modified) {
+cc_result Socket_Read(cc_socket s, cc_uint8* data, cc_uint32 count, cc_uint32* read) {
-	/* recv only reads one WebSocket frame at most, hence call it multiple times */
+	int res; 
-	int res; *modified = 0;
+	*read = 0;
 
-	while (count && interop_SocketGetPending(s) > 0) {
+	/* interop_SocketRecv only reads one WebSocket frame at most, hence call it multiple times */
+	while (count) {
 		/* returned result is negative for error */
 		res = interop_SocketRecv(s, data, count);
 
 		if (res >= 0) {
-			*modified += res;
+			*read += res;
 			data  += res; count -= res;
 		} else {
-			/* EAGAIN when no data available */
+			/* EAGAIN when no more data available */
-			if (res == -_EAGAIN) break;
+			if (res == -_EAGAIN) return *read == 0 ? _EAGAIN : 0;
+
 			return -res;
 		}
 	}
@@ -332,18 +310,10 @@ void Socket_Close(cc_socket s) {
 	interop_SocketClose(s);
 }
 
-extern int interop_SocketPoll(int sock);
+extern int interop_SocketWritable(int sock, cc_bool* writable);
-cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
+cc_result Socket_CheckWritable(cc_socket s, cc_bool* writable) {
 	/* returned result is negative for error */
-	int res = interop_SocketPoll(s), flags;
+	return -interop_SocketWritable(s, writable);
-
-	if (res >= 0) {
-		flags    = mode == SOCKET_POLL_READ ? 0x01 : 0x02;
-		*success = (res & flags) != 0;
-		return 0;
-	} else {
-		*success = false; return -res;
-	}
 }
 
 

src/Platform_WinApi.c

@@ -453,15 +453,6 @@ static void LoadWinsockFuncs(void) {
 	if (!_WSAStringToAddressW) _WSAStringToAddressW = FallbackParseAddress;
 }
 
-cc_result Socket_Available(cc_socket s, int* available) {
-	return _ioctlsocket(s, FIONREAD, available);
-}
-
-cc_result Socket_GetError(cc_socket s, cc_result* result) {
-	int resultSize = sizeof(cc_result);
-	return _getsockopt(s, SOL_SOCKET, SO_ERROR, (char*)result, &resultSize);
-}
-
 static int ParseHost(void* dst, WCHAR* host, int port) {
 	SOCKADDR_IN* addr4 = (SOCKADDR_IN*)dst;
 	struct hostent* res;
@@ -542,7 +533,7 @@ void Socket_Close(cc_socket s) {
 	_closesocket(s);
 }
 
-cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
+static cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
 	fd_set set;
 	struct timeval time = { 0 };
 	int selectCount;
@@ -561,6 +552,21 @@ cc_result Socket_Poll(cc_socket s, int mode, cc_bool* success) {
 	*success = set.fd_count != 0; return 0;
 }
 
+cc_result Socket_CheckReadable(cc_socket s, cc_bool* readable) {
+	return Socket_Poll(s, SOCKET_POLL_READ, readable);
+}
+
+cc_result Socket_CheckWritable(cc_socket s, cc_bool* writable) {
+	int resultSize;
+	cc_result res = Socket_Poll(s, SOCKET_POLL_WRITE, writable);
+	if (res || *writable) return res;
+
+	/* https://stackoverflow.com/questions/29479953/so-error-value-after-successful-socket-operation */
+	resultSize = sizeof(cc_result);
+	_getsockopt(s, SOL_SOCKET, SO_ERROR, (char*)&res, &resultSize);
+	return res;
+}
+
 
 /*########################################################################################################################*
 *-----------------------------------------------------Process/Module------------------------------------------------------*

src/Protocol.c

@@ -369,7 +369,6 @@ static cc_result MapState_Read(struct MapState* m) {
 /*########################################################################################################################*
 *----------------------------------------------------Classic protocol-----------------------------------------------------*
 *#########################################################################################################################*/
-#define Classic_HandshakeSize() (Game_Version.Protocol > PROTOCOL_0019 ? 131 : 130)
 void Classic_SendLogin(void) {
 	cc_uint8 data[131];
 	data[0] = OPCODE_HANDSHAKE;
@@ -377,9 +376,19 @@ void Classic_SendLogin(void) {
 		data[1]   = Game_Version.Protocol;
 		WriteString(&data[2],  &Game_Username);
 		WriteString(&data[66], &Game_Mppass);
-		data[130] = Game_UseCPE ? 0x42 : 0x00;
+
+		/* The 'user type' field's behaviour depends on protocol version: */
+		/*   version 7 - 0x42 specifies CPE client, any other value is ignored? */
+		/*   version 6 - any value ignored? */
+		/*   version 5 - field doesn't exist */
+		/* Theroetically, this means packet size is 131 bytes for 6/7, 130 bytes for 5 and below. */
+		/* In practice, some version 7 server software always expects to read 131 bytes for handshake packet, */
+		/*  and will get stuck waiting for data if client connects using version 5 and only sends 130 bytes */
+		/* To workaround this, include a 'ping packet' after 'version 5 handshake packet' - version 5 server software */
+		/*  will do nothing with the ping packet, and the aforementioned server software will be happy with 131 bytes */
+		data[130] = Game_UseCPE ? 0x42 : (Game_Version.Protocol <= PROTOCOL_0019 ? OPCODE_PING : 0x00);
 	}
-	Server.SendData(data, Classic_HandshakeSize());
+	Server.SendData(data, 131);
 }
 
 void Classic_SendChat(const cc_string* text, cc_bool partial) {
@@ -718,6 +727,7 @@ static void Classic_ReadAbsoluteLocation(cc_uint8* data, EntityID id, cc_uint8 f
 	UpdateLocation(id, &update);
 }
 
+#define Classic_HandshakeSize() (Game_Version.Protocol > PROTOCOL_0019 ? 131 : 130)
 static void Classic_Reset(void) {
 	Stream_ReadonlyMemory(&map_part, NULL, 0);
 	map_begunLoading = false;

src/Server.c

@@ -116,7 +116,6 @@ cc_string SP_AutoloadMap = String_FromArray(autoloadBuffer);
 
 static void SPConnection_BeginConnect(void) {
 	static const cc_string logName = String_FromConst("Singleplayer");
-	cc_string path;
 	RNGState rnd;
 	Chat_SetLogName(&logName);
 	Game_UseCPEBlocks = Game_UseCPE;
@@ -214,7 +213,7 @@ static cc_uint8  net_readBuffer[4096 * 5];
 static cc_uint8* net_readCurrent;
 
 static cc_result net_writeFailure;
-static double lastPacket;
+static double net_lastPacket;
 static cc_uint8 lastOpcode;
 
 static cc_bool net_connecting;
@@ -228,8 +227,8 @@ static void MPConnection_FinishConnect(void) {
 	Event_RaiseFloat(&WorldEvents.Loading, 0.0f);
 
 	net_readCurrent = net_readBuffer;
+	net_lastPacket  = Game.Time;
 	Classic_SendLogin();
-	lastPacket = Game.Time;
 }
 
 static void MPConnection_Fail(const cc_string* reason) {
@@ -255,17 +254,13 @@ static void MPConnection_FailConnect(cc_result result) {
 }
 
 static void MPConnection_TickConnect(void) {
-	cc_result res = 0;
+	cc_bool writable;
-	cc_bool poll_write;
+	double now    = Game.Time;
-	double now;
+	cc_result res = Socket_CheckWritable(net_socket, &writable);
 
-	Socket_GetError(net_socket, &res);
+	if (res) {
-	if (res) { MPConnection_FailConnect(res); return; }
+		MPConnection_FailConnect(res);
-
+	} else if (writable) {
-	Socket_Poll(net_socket, SOCKET_POLL_WRITE, &poll_write);
-	now = Game.Time;
-
-	if (poll_write) {
 		MPConnection_FinishConnect();
 	} else if (now > net_connectTimeout) {
 		MPConnection_FailConnect(0);
@@ -331,18 +326,13 @@ static void MPConnection_Disconnect(void) {
 	Game_Disconnect(&title, &reason);
 }
 
-static void MPConnection_CheckDisconnection(void) {
+static void DisconnectReadFailed(cc_result res) {
-	cc_result availRes, selectRes;
+	cc_string msg; char msgBuffer[STRING_SIZE * 2];
-	int pending = 0;
+	String_InitArray(msg, msgBuffer);
-	cc_bool poll_read;
+	String_Format3(&msg, "Error reading from %s:%i: %i" _NL, &Server.Address, &Server.Port, &res);
 
-	availRes  = Socket_Available(net_socket, &pending);
+	Logger_Log(&msg);
-	/* poll read returns true when socket is closed */
-	selectRes = Socket_Poll(net_socket, SOCKET_POLL_READ, &poll_read);
-
-	if (net_writeFailure || availRes || selectRes || (pending == 0 && poll_read)) {
 	MPConnection_Disconnect();
-	}
 }
 
 static void DisconnectInvalidOpcode(cc_uint8 opcode) {
@@ -355,47 +345,34 @@ static void DisconnectInvalidOpcode(cc_uint8 opcode) {
 }
 
 static void MPConnection_Tick(struct ScheduledTask* task) {
-	static const cc_string title_lost  = String_FromConst("&eLost connection to the server");
-	static const cc_string reason_err  = String_FromConst("I/O error when reading packets");
-	cc_string msg; char msgBuffer[STRING_SIZE * 2];
-	cc_uint32 pending;
 	cc_uint8* readEnd;
 	Net_Handler handler;
+	cc_uint32 read;
 	int i, remaining;
 	cc_result res;
 
 	if (Server.Disconnected) return;
 	if (net_connecting) { MPConnection_TickConnect(); return; }
 
-	/* Over 30 seconds since last packet, connection likely dropped */
+	/* NOTE: using a read call that is a multiple of 4096 (appears to?) improve read performance */	
-	if (lastPacket + 30 < Game.Time) MPConnection_CheckDisconnection();
+	res = Socket_Read(net_socket, net_readCurrent, 4096 * 4, &read);
-	if (Server.Disconnected) return;
-
-	pending = 0;
-	res     = Socket_Available(net_socket, &pending);
-	readEnd = net_readCurrent; /* todo change to int remaining instead */
-
-	if (!res && pending) {
-		/* NOTE: Always using a read call that is a multiple of 4096 (appears to?) improve read performance */	
-		res = Socket_Read(net_socket, net_readCurrent, 4096 * 4, &pending);
-		/* Ignore errors for 'no data available for non-blocking read' */
-		if (res) {
-			if (res == ReturnCode_SocketInProgess)  return;
-			if (res == ReturnCode_SocketWouldBlock) return;
-		}
-		readEnd += pending;
-	}
 	
 	if (res) {
-		String_InitArray(msg, msgBuffer);
+		/* 'no data available for non-blocking read' is an expected error */
-		String_Format3(&msg, "Error reading from %s:%i: %i" _NL, &Server.Address, &Server.Port, &res);
+		if (res == ReturnCode_SocketInProgess)  res = 0;
-
+		if (res == ReturnCode_SocketWouldBlock) res = 0;
-		Logger_Log(&msg);
-		Game_Disconnect(&title_lost, &reason_err);
-		return;
-	}
 
+		if (res) { DisconnectReadFailed(res); return; }
+	} else if (read == 0) {
+		/* recv only returns 0 read when socket is closed.. probably? */
+		/* Over 30 seconds since last packet, connection probably dropped */
+		/* TODO: Should this be checked unconditonally instead of just when read = 0 ? */
+		if (net_lastPacket + 30 < Game.Time) { MPConnection_Disconnect(); return; }
+	} else {
+		readEnd         = net_readCurrent + read;
+		net_lastPacket  = Game.Time;
 		net_readCurrent = net_readBuffer;
+
 		while (net_readCurrent < readEnd) {
 			cc_uint8 opcode = net_readCurrent[0];
 
@@ -412,7 +389,6 @@ static void MPConnection_Tick(struct ScheduledTask* task) {
 			if (!handler) { DisconnectInvalidOpcode(opcode); return; }
 
 			lastOpcode = opcode;
-		lastPacket = Game.Time;
 			handler(net_readCurrent + 1); /* skip opcode */
 			net_readCurrent += Protocol.Sizes[opcode];
 		}
@@ -425,10 +401,11 @@ static void MPConnection_Tick(struct ScheduledTask* task) {
 			net_readBuffer[i] = net_readCurrent[i];
 		}
 		net_readCurrent = net_readBuffer + remaining;
+	}
 
 	if (net_writeFailure) {
 		Platform_Log1("Error from send: %i", &net_writeFailure);
-		MPConnection_Disconnect();
+		MPConnection_Disconnect(); return;
 	}
 
 	/* Network is ticked 60 times a second. We only send position updates 20 times a second */

src/interop_web.js

@@ -537,7 +537,7 @@ mergeInto(LibraryManager.library, {
   },
   interop_SocketCreate: function() {
     var sock = {
-      error: null, // Used by interop_SocketGetError
+      error: null, // Used by interop_SocketWritable
       recv_queue: [],
       socket: null,
     };
@@ -593,7 +593,7 @@ mergeInto(LibraryManager.library, {
     ws.onerror = function(error) {
       // The WebSocket spec only allows a 'simple event' to be thrown on error,
       // so we only really know as much as ECONNREFUSED.
-      sock.error = -SOCKETS.ECONNREFUSED; // Used by interop_SocketGetError
+      sock.error = SOCKETS.ECONNREFUSED; // Used by interop_SocketWritable
     };
     // always "fail" in non-blocking mode
     return SOCKETS.EINPROGRESS;
@@ -667,29 +667,15 @@ mergeInto(LibraryManager.library, {
     HEAPU8.set(msg, dst);
     return msg.byteLength;
   },
-  interop_SocketGetPending: function(sockFD) {
+  interop_SocketWritable: function(sockFD, writable) {
-    var sock = SOCKETS.sockets[sockFD];
+    HEAPU8[writable|0] = 0;
-    if (!sock) return SOCKETS.EBADF;
-
-    return sock.recv_queue.length;
-  },
-  interop_SocketGetError: function(sockFD) {
-    var sock = SOCKETS.sockets[sockFD];
-    if (!sock) return SOCKETS.EBADF;
-
-    return sock.error || 0;
-  },
-  interop_SocketPoll: function(sockFD) {
     var sock = SOCKETS.sockets[sockFD];
     if (!sock) return SOCKETS.EBADF;
 
     var ws = sock.socket;
-    if (!ws) return 0;
+    if (!ws) return SOCKETS.ENOTCONN;
-    var mask = 0;
+    if (ws.readyState === ws.OPEN) HEAPU8[writable|0] = 1;
-
+    return sock.error || 0;
-    if (sock.recv_queue.length || (ws.readyState === ws.CLOSING || ws.readyState === ws.CLOSED)) mask |= 1;
-    if (ws.readyState === ws.OPEN) mask |= 2;
-    return mask;
   },