feat, fix: Improved logging, improved handling of sockets, fixed crashing issues on Linux/Unix. (#24)

* fix: send/recv now uses MSG_NOSIGNAL, improved server logging, changed disconnect logic from heartbeat

* fix: compile issue

* fix: added missing header

* feat, fix: logging improvements, improved handling of sockets on all platforms, improved disconnection logic

* refactor, fix: change of disconnect logic again, compile fix

* feat, fix: more rewrites and cleanup

* refactor: more cleanup

* fix: fixed potential race condition in server

* refactor: improved logging for server connections

* fix: No more deadlocking, unit tests now test multiple client connections to a server

* refactor: added socket to disconnect message

Author: Jaskowicz1

include/rconpp/client.h

@@ -34,11 +34,7 @@ class RCONPP_EXPORT rcon_client {
 	const std::string address{};
 	const int port{0};
 	const std::string password{};
-#ifdef _WIN32
-	SOCKET sock{INVALID_SOCKET};
-#else
-	int sock{0};
-#endif
+	SOCKET_TYPE sock{INVALID_SOCKET};
 
 	std::vector<queued_request> requests_queued{};
 
@@ -47,7 +43,7 @@ class RCONPP_EXPORT rcon_client {
 public:
 	std::atomic<bool> connected{false};
 
-	std::function<void(const std::string_view& log)> on_log;
+	std::function<void(const std::string_view& log)> on_log{};
 
 	std::condition_variable terminating;
 
@@ -61,7 +57,7 @@ class RCONPP_EXPORT rcon_client {
 	 * @note This is a blocking call (done on purpose). It needs to wait to connect to the RCON server before anything else happens.
 	 * It will timeout after 4 seconds if it can't connect.
 	 */
-	rcon_client(const std::string_view addr, const int _port, const std::string_view pass);
+	rcon_client(std::string_view addr, int _port, std::string_view pass);
 
 	~rcon_client();
 
@@ -93,7 +89,7 @@ class RCONPP_EXPORT rcon_client {
 	 *
 	 * @returns Data given by the server from the request.
 	 */
-	response send_data_sync(const std::string_view data, const int32_t id, data_type type, bool feedback = true);
+	response send_data_sync(std::string_view data, int32_t id, data_type type, bool feedback = true);
 
 private:
 

include/rconpp/server.h

@@ -24,7 +24,7 @@ namespace rconpp {
 
 struct connected_client {
 	sockaddr_in sock_info{};
-	int socket{0};
+	SOCKET_TYPE socket{0};
 	bool connected{false};
 
 	bool authenticated{false};
@@ -42,14 +42,12 @@ class RCONPP_EXPORT rcon_server {
 	int port{0};
 	std::string password{};
 
-#ifdef _WIN32
-	SOCKET sock{INVALID_SOCKET};
-#else
-	int sock{0};
-#endif
+	SOCKET_TYPE sock{INVALID_SOCKET};
 
 	std::thread accept_connections_runner;
+
 	std::mutex connected_clients_mutex;
+	std::mutex request_handlers_mutex;
 
 public:
 	bool online{false};
@@ -61,11 +59,14 @@ class RCONPP_EXPORT rcon_server {
 	std::condition_variable terminating;
 
 	/**
-	 * @brief A map of connected clients. The key is their socket to talk to.
+	 * @brief A map of connected clients. The key is their socket.
 	 */
-	std::unordered_map<int, connected_client> connected_clients{};
+	std::unordered_map<SOCKET_TYPE, connected_client> connected_clients{};
 
-	std::unordered_map<int, std::thread> request_handlers{};
+	/**
+	 * @brief A map of request_handlers (the thread for a client that handles all requests a client will send). The key is their socket.
+	 */
+	std::unordered_map<SOCKET_TYPE, std::thread> request_handlers{};
 
 	/**
 	 * @brief rcon_server constuctor. Initiates a connection to an RCON server with the parameters given.
@@ -77,7 +78,7 @@ class RCONPP_EXPORT rcon_server {
 	 * @note This is a blocking call (done on purpose). It needs to wait to connect to the RCON server before anything else happens.
 	 * It will timeout after 4 seconds if it can't connect.
 	 */
-	rcon_server(const std::string_view addr, const int _port, const std::string_view pass);
+	rcon_server(std::string_view addr, int _port, std::string_view pass);
 
 	~rcon_server();
 
@@ -89,7 +90,7 @@ class RCONPP_EXPORT rcon_server {
 	 * @param client_socket The socket of the client to disconnect.
 	 * @param remove_after Should remove client from connected_clients after?
 	 */
-	void disconnect_client(int client_socket, bool remove_after = true);
+	void disconnect_client(SOCKET_TYPE client_socket, bool remove_after = true);
 
 private:
 
@@ -117,6 +118,28 @@ class RCONPP_EXPORT rcon_server {
 	 * @returns bool, true is heartbeat was sent, otherwise false.
 	 */
 	bool send_heartbeat(connected_client& client);
+
+	void client_process_loop(connected_client& client);
+
+	void add_client(const SOCKET_TYPE client_socket, connected_client& client) {
+		while (!connected_clients_mutex.try_lock()) {
+			std::this_thread::sleep_for(std::chrono::milliseconds(100));
+		}
+
+		connected_clients.insert({ client_socket, client });
+
+		connected_clients_mutex.unlock();
+	}
+
+	void remove_client(const SOCKET_TYPE client_socket) {
+		while (!connected_clients_mutex.try_lock()) {
+			std::this_thread::sleep_for(std::chrono::milliseconds(100));
+		}
+
+		connected_clients.erase(client_socket);
+
+		connected_clients_mutex.unlock();
+	};
 };
 
 } // namespace rconpp

include/rconpp/utilities.h

@@ -1,20 +1,58 @@
 #pragma once
 
-#include <functional>
+#ifdef _WIN32
+#include <winsock2.h>
+#else
+#include <sys/types.h>
+#include <sys/socket.h>
+#endif
+
+
+#include <optional>
 
 #include "export.h"
 #include <string>
 #include <string_view>
 #include <vector>
-#include <iostream>
 
 namespace rconpp {
 
+// Connection constants.
 constexpr int DEFAULT_TIMEOUT = 4; // In Seconds.
+constexpr int MAX_RETRIES_TO_RECEIVE_INFO = 5;
+constexpr int HEARTBEAT_TIME = 30;
+
+// Packet constants.
 constexpr int MIN_PACKET_SIZE = 10;
 constexpr int MIN_PACKET_LENGTH = 14;
-constexpr int MAX_RETRIES_TO_RECEIVE_INFO = 500;
-constexpr int HEARTBEAT_TIME = 30;
+constexpr int MAX_PACKET_SIZE = 4096;
+constexpr int PACKET_SIZE_BYTES = 4; // The first x bytes of the packet to read for the packet size (usually the first 4 bytes)
+
+// Used for send/recv calls, as `signal(SIGPIPE, SIG_IGN);` seems to be ignored.
+#ifndef MSG_NOSIGNAL
+	#define MSG_NOSIGNAL 0
+#endif
+
+// INVALID_SOCKET doesn't exist on Linux/Unix (both platforms just return -1 on error), add this to avoid ifdef spam.
+#ifndef INVALID_SOCKET
+	#define INVALID_SOCKET -1
+#endif
+
+// Same as above.
+#ifndef SOCKET_ERROR
+	#define SOCKET_ERROR -1
+#endif
+
+// Windows uses uint64_t for sockets, whereas Linux/Unix uses int,
+// having this reduces the warnings and whatnot on Windows caused by converting them to int.
+#ifndef SOCKET_TYPE
+	#ifdef _WIN32
+		#define SOCKET_TYPE SOCKET
+	#else
+		#define SOCKET_TYPE int
+	#endif
+#endif
+
 
 
 enum data_type {
@@ -62,6 +100,17 @@ struct response {
 	bool server_responded{false};
 };
 
+enum error_type {
+	DISCONNECTED = 0,
+	BAD_FD = 1,
+	SHUTTING_DOWN = 2,
+};
+
+struct last_error {
+	error_type type_of_error{error_type::DISCONNECTED};
+	int error_code{0};
+};
+
 /**
  * @brief Form a valid RCON packet.
  *
@@ -71,7 +120,7 @@ struct response {
  *
  * @returns The packet data (as an array of chars) to send to a server.
  */
-RCONPP_EXPORT packet form_packet(const std::string_view data, int32_t id, int32_t type);
+RCONPP_EXPORT packet form_packet(std::string_view data, int32_t id, int32_t type);
 
 /**
  * @brief Turn the first 4 bytes of a buffer (which ideally a 32 bit int) into an integer.
@@ -92,15 +141,15 @@ RCONPP_EXPORT int bit32_to_int(const std::vector<char>& buffer);
 RCONPP_EXPORT int type_to_int(const std::vector<char>& buffer);
 
 /**
- * @brief Reports the recent socket error.
+ * @brief Converts the last error into a prev_error struct.
  */
-RCONPP_EXPORT void report_error();
+RCONPP_EXPORT last_error get_last_error();
 
 /**
  * @brief Reads the first 4 bytes of a packet to get the packet size (not to be mistaken with length).
  *
  * @return The size (not length) of the packet.
  */
-RCONPP_EXPORT int read_packet_size(int socket);
+RCONPP_EXPORT int read_packet_size(SOCKET_TYPE socket);
 
 } // namespace rconpp

src/rconpp/client.cpp

@@ -9,6 +9,7 @@ rconpp::rcon_client::~rcon_client() {
 	if (on_log) {
 		on_log("RCON client is shutting down.");
 	}
+
 	// Set connected to false, meaning no requests can be attempted during shutdown.
 	connected = false;
 
@@ -20,6 +21,7 @@ rconpp::rcon_client::~rcon_client() {
 #else
 	close(sock);
 #endif
+
 	// Join the queue runner (if allowed), meaning we await its end before killing this object, preventing any corruption.
 	if (queue_runner.joinable()) {
 		queue_runner.join();
@@ -34,9 +36,9 @@ rconpp::response rconpp::rcon_client::send_data_sync(const std::string_view data
 
 	packet formed_packet = form_packet(data, id, type);
 
-	if (send(sock, formed_packet.data.data(), formed_packet.length, 0) < 0) {
-		on_log("Sending failed!");
-		report_error();
+	if (send(sock, formed_packet.data.data(), formed_packet.length, MSG_NOSIGNAL) < 0) {
+		const last_error err = get_last_error();
+		on_log("Sending failed [Error code: " + std::to_string(err.error_code) + "]!");
 		return { "", false };
 	}
 
@@ -53,7 +55,7 @@ bool rconpp::rcon_client::connect_to_server() {
 #ifdef _WIN32
 	// Initialize Winsock
 	WSADATA wsa_data;
-	int result = WSAStartup(MAKEWORD(2, 2), &wsa_data);
+	const int result = WSAStartup(MAKEWORD(2, 2), &wsa_data);
 	if (result != 0) {
 		on_log("WSAStartup failed. Error: " + std::to_string(result));
 		return false;
@@ -63,13 +65,9 @@ bool rconpp::rcon_client::connect_to_server() {
 	// Create new TCP socket.
 	sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
 
-#ifdef _WIN32
 	if (sock == INVALID_SOCKET) {
-#else
-	if (sock == -1) {
-#endif
-		on_log("Failed to open socket.");
-		report_error();
+		const last_error err = get_last_error();
+		on_log("Failed to open socket [Error code: " + std::to_string(err.error_code) + "]!");
 		return false;
 	}
 
@@ -103,8 +101,7 @@ bool rconpp::rcon_client::connect_to_server() {
 	// Connect to the socket and set the status of the connection.
 	int status = connect(sock, (struct sockaddr*)&server, sizeof(server));
 
-	if (status == -1) {
-		report_error();
+	if (status == SOCKET_ERROR) {
 		return false;
 	}
 
@@ -158,7 +155,7 @@ rconpp::packet rconpp::rcon_client::read_packet() {
 	}
 
 	packet temp_packet{};
-	temp_packet.length = packet_size + 4;
+	temp_packet.length = packet_size + PACKET_SIZE_BYTES;
 
 	if (packet_size > 0) {
 		temp_packet.size = packet_size;
@@ -178,7 +175,7 @@ rconpp::packet rconpp::rcon_client::read_packet() {
 	 * Receiving by the length of the packet will give us 4 extra bytes, so, we do by size here.
 	 * This is because read_packet_size() reads the first 4 bytes and discards them.
 	 */
-	recv(sock, buffer.data(), temp_packet.size, 0);
+	recv(sock, buffer.data(), temp_packet.size, MSG_NOSIGNAL);
 
 	temp_packet.data = buffer;