Codechange: another attempt at making thread safer AsString

src/network/core/address.cpp

@@ -317,7 +317,7 @@ static SOCKET ConnectLoopProc(addrinfo *runp)
 
 	SOCKET sock = socket(runp->ai_family, runp->ai_socktype, runp->ai_protocol);
 	if (sock == INVALID_SOCKET) {
-		DEBUG(net, 1, "[%s] could not create %s socket: %s", type, family, NetworkError::GetLastAsString().c_str());
+		DEBUG(net, 1, "[%s] could not create %s socket: %s", type, family, NetworkError::GetLast().AsString());
 		return INVALID_SOCKET;
 	}
 
@@ -327,7 +327,7 @@ static SOCKET ConnectLoopProc(addrinfo *runp)
 
 	int err = connect(sock, runp->ai_addr, (int)runp->ai_addrlen);
 	if (err != 0 && !NetworkError::GetLast().IsConnectInProgress()) {
-		DEBUG(net, 1, "[%s] could not connect to %s over %s: %s", type, address.c_str(), family, NetworkError::GetLastAsString().c_str());
+		DEBUG(net, 1, "[%s] could not connect to %s over %s: %s", type, address.c_str(), family, NetworkError::GetLast().AsString());
 		closesocket(sock);
 		return INVALID_SOCKET;
 	}
@@ -343,7 +343,7 @@ static SOCKET ConnectLoopProc(addrinfo *runp)
 	tv.tv_sec = DEFAULT_CONNECT_TIMEOUT_SECONDS;
 	int n = select(FD_SETSIZE, NULL, &write_fd, NULL, &tv);
 	if (n < 0) {
-		DEBUG(net, 1, "[%s] could not connect to %s: %s", type, address.c_str(), NetworkError::GetLastAsString().c_str());
+		DEBUG(net, 1, "[%s] could not connect to %s: %s", type, address.c_str(), NetworkError::GetLast().AsString());
 		closesocket(sock);
 		return INVALID_SOCKET;
 	}
@@ -358,7 +358,7 @@ static SOCKET ConnectLoopProc(addrinfo *runp)
 	/* Retrieve last error, if any, on the socket. */
 	NetworkError socket_error = GetSocketError(sock);
 	if (socket_error.HasError()) {
-		DEBUG(net, 1, "[%s] could not connect to %s: %s", type, address.c_str(), socket_error.AsString().c_str());
+		DEBUG(net, 1, "[%s] could not connect to %s: %s", type, address.c_str(), socket_error.AsString());
 		closesocket(sock);
 		return INVALID_SOCKET;
 	}
@@ -393,7 +393,7 @@ static SOCKET ListenLoopProc(addrinfo *runp)
 
 	SOCKET sock = socket(runp->ai_family, runp->ai_socktype, runp->ai_protocol);
 	if (sock == INVALID_SOCKET) {
-		DEBUG(net, 0, "[%s] could not create %s socket on port %s: %s", type, family, address.c_str(), NetworkError::GetLastAsString().c_str());
+		DEBUG(net, 0, "[%s] could not create %s socket on port %s: %s", type, family, address.c_str(), NetworkError::GetLast().AsString());
 		return INVALID_SOCKET;
 	}
 
@@ -404,24 +404,24 @@ static SOCKET ListenLoopProc(addrinfo *runp)
 	int on = 1;
 	/* The (const char*) cast is needed for windows!! */
 	if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, (const char*)&on, sizeof(on)) == -1) {
-		DEBUG(net, 3, "[%s] could not set reusable %s sockets for port %s: %s", type, family, address.c_str(), NetworkError::GetLastAsString().c_str());
+		DEBUG(net, 3, "[%s] could not set reusable %s sockets for port %s: %s", type, family, address.c_str(), NetworkError::GetLast().AsString());
 	}
 
 #ifndef __OS2__
 	if (runp->ai_family == AF_INET6 &&
 			setsockopt(sock, IPPROTO_IPV6, IPV6_V6ONLY, (const char*)&on, sizeof(on)) == -1) {
-		DEBUG(net, 3, "[%s] could not disable IPv4 over IPv6 on port %s: %s", type, address.c_str(), NetworkError::GetLastAsString().c_str());
+		DEBUG(net, 3, "[%s] could not disable IPv4 over IPv6 on port %s: %s", type, address.c_str(), NetworkError::GetLast().AsString());
 	}
 #endif
 
 	if (bind(sock, runp->ai_addr, (int)runp->ai_addrlen) != 0) {
-		DEBUG(net, 1, "[%s] could not bind on %s port %s: %s", type, family, address.c_str(), NetworkError::GetLastAsString().c_str());
+		DEBUG(net, 1, "[%s] could not bind on %s port %s: %s", type, family, address.c_str(), NetworkError::GetLast().AsString());
 		closesocket(sock);
 		return INVALID_SOCKET;
 	}
 
 	if (runp->ai_socktype != SOCK_DGRAM && listen(sock, 1) != 0) {
-		DEBUG(net, 1, "[%s] could not listen at %s port %s: %s", type, family, address.c_str(), NetworkError::GetLastAsString().c_str());
+		DEBUG(net, 1, "[%s] could not listen at %s port %s: %s", type, family, address.c_str(), NetworkError::GetLast().AsString());
 		closesocket(sock);
 		return INVALID_SOCKET;
 	}

src/network/core/os_abstraction.cpp

@@ -76,25 +76,28 @@ bool NetworkError::IsConnectInProgress() const
  * Get the string representation of the error message.
  * @return The string representation that will get overwritten by next calls.
  */
-std::string NetworkError::AsString() const
+const char *NetworkError::AsString() const
 {
+	if (this->message.get() == nullptr) {
 #if defined(_WIN32)
-	char buffer[512];
-	if (FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, NULL, this->error,
-		MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), buffer, sizeof(buffer), NULL) == 0) {
-		seprintf(buffer, lastof(buffer), "Unknown error %d", this->error);
-	}
-	return buffer;
+		char buffer[512];
+		if (FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, NULL, this->error,
+			MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), buffer, sizeof(buffer), NULL) == 0) {
+			seprintf(buffer, lastof(buffer), "Unknown error %d", this->error);
+		}
+		this->message.reset(new std::string(buffer));
 #else
-	/* Make strerror thread safe by locking access to it. There is a thread safe strerror_r, however
-	 * the non-POSIX variant is available due to defining _GNU_SOURCE meaning it is not portable.
-	 * The problem with the non-POSIX variant is that it does not necessarily fill the buffer with
-	 * the error message but can also return a pointer to a static bit of memory, whereas the POSIX
-	 * variant always fills the buffer. This makes the behaviour too erratic to work with. */
-	static std::mutex mutex;
-	std::lock_guard<std::mutex> guard(mutex);
-	return strerror(this->error);
+		/* Make strerror thread safe by locking access to it. There is a thread safe strerror_r, however
+		 * the non-POSIX variant is available due to defining _GNU_SOURCE meaning it is not portable.
+		 * The problem with the non-POSIX variant is that it does not necessarily fill the buffer with
+		 * the error message but can also return a pointer to a static bit of memory, whereas the POSIX
+		 * variant always fills the buffer. This makes the behaviour too erratic to work with. */
+		static std::mutex mutex;
+		std::lock_guard<std::mutex> guard(mutex);
+		this->message.reset(new std::string(strerror(this->error)));
 #endif
+	}
+	return this->message.get()->c_str();
 }
 
 /**
@@ -121,15 +124,6 @@ bool NetworkError::HasError() const
 #endif
 }
 
-/**
- * Convenience methods to the get string represenation of the last error.
- * @return The string representation that will get overwritten by next calls.
- */
-/* static */ std::string NetworkError::GetLastAsString()
-{
-	return GetLast().AsString();
-}
-
 
 /**
  * Try to set the socket into non-blocking mode.

src/network/core/os_abstraction.h

@@ -20,18 +20,18 @@
  */
 class NetworkError {
 private:
-	int error; ///< The underlying error number from errno or WSAGetLastError.
+	int error;                                    ///< The underlying error number from errno or WSAGetLastError.
+	mutable std::unique_ptr<std::string> message; ///< The string representation of the error.
 public:
 	NetworkError(int error);
 
 	bool HasError() const;
 	bool WouldBlock() const;
 	bool IsConnectionReset() const;
 	bool IsConnectInProgress() const;
-	std::string AsString() const;
+	const char *AsString() const;
 
 	static NetworkError GetLast();
-	static std::string GetLastAsString();
 };
 
 /* Include standard stuff per OS */

src/network/core/tcp.cpp

@@ -90,7 +90,7 @@ SendPacketsState NetworkTCPSocketHandler::SendPackets(bool closing_down)
 			if (!err.WouldBlock()) {
 				/* Something went wrong.. close client! */
 				if (!closing_down) {
-					DEBUG(net, 0, "send failed with error %s", err.AsString().c_str());
+					DEBUG(net, 0, "send failed with error %s", err.AsString());
 					this->CloseConnection();
 				}
 				return SPS_CLOSED;
@@ -139,7 +139,7 @@ Packet *NetworkTCPSocketHandler::ReceivePacket()
 				NetworkError err = NetworkError::GetLast();
 				if (!err.WouldBlock()) {
 					/* Something went wrong... */
-					if (!err.IsConnectionReset()) DEBUG(net, 0, "recv failed with error %s", err.AsString().c_str());
+					if (!err.IsConnectionReset()) DEBUG(net, 0, "recv failed with error %s", err.AsString());
 					this->CloseConnection();
 					return nullptr;
 				}
@@ -167,7 +167,7 @@ Packet *NetworkTCPSocketHandler::ReceivePacket()
 			NetworkError err = NetworkError::GetLast();
 			if (!err.WouldBlock()) {
 				/* Something went wrong... */
-				if (!err.IsConnectionReset()) DEBUG(net, 0, "recv failed with error %s", err.AsString().c_str());
+				if (!err.IsConnectionReset()) DEBUG(net, 0, "recv failed with error %s", err.AsString());
 				this->CloseConnection();
 				return nullptr;
 			}

src/network/core/tcp_http.cpp

@@ -228,7 +228,7 @@ int NetworkHTTPSocketHandler::Receive()
 			NetworkError err = NetworkError::GetLast();
 			if (!err.WouldBlock()) {
 				/* Something went wrong... */
-				if (!err.IsConnectionReset()) DEBUG(net, 0, "recv failed with error %s", err.AsString().c_str());
+				if (!err.IsConnectionReset()) DEBUG(net, 0, "recv failed with error %s", err.AsString());
 				return -1;
 			}
 			/* Connection would block, so stop for now */

src/network/core/tcp_listen.h

@@ -64,7 +64,7 @@ class TCPListenHandler {
 					DEBUG(net, 1, "[%s] Banned ip tried to join (%s), refused", Tsocket::GetName(), entry.c_str());
 
 					if (p.TransferOut<int>(send, s, 0) < 0) {
-						DEBUG(net, 0, "send failed with error %s", NetworkError::GetLastAsString().c_str());
+						DEBUG(net, 0, "send failed with error %s", NetworkError::GetLast().AsString());
 					}
 					closesocket(s);
 					break;
@@ -81,7 +81,7 @@ class TCPListenHandler {
 				p.PrepareToSend();
 
 				if (p.TransferOut<int>(send, s, 0) < 0) {
-					DEBUG(net, 0, "send failed with error %s", NetworkError::GetLastAsString().c_str());
+					DEBUG(net, 0, "send failed with error %s", NetworkError::GetLast().AsString());
 				}
 				closesocket(s);
 

src/network/core/udp.cpp

@@ -95,7 +95,7 @@ void NetworkUDPSocketHandler::SendPacket(Packet *p, NetworkAddress *recv, bool a
 			/* Enable broadcast */
 			unsigned long val = 1;
 			if (setsockopt(s.second, SOL_SOCKET, SO_BROADCAST, (char *) &val, sizeof(val)) < 0) {
-				DEBUG(net, 1, "[udp] setting broadcast failed with: %s", NetworkError::GetLastAsString().c_str());
+				DEBUG(net, 1, "[udp] setting broadcast failed with: %s", NetworkError::GetLast().AsString());
 			}
 		}
 
@@ -104,7 +104,7 @@ void NetworkUDPSocketHandler::SendPacket(Packet *p, NetworkAddress *recv, bool a
 		DEBUG(net, 7, "[udp] sendto(%s)", send.GetAddressAsString().c_str());
 
 		/* Check for any errors, but ignore it otherwise */
-		if (res == -1) DEBUG(net, 1, "[udp] sendto(%s) failed with: %s", send.GetAddressAsString().c_str(), NetworkError::GetLastAsString().c_str());
+		if (res == -1) DEBUG(net, 1, "[udp] sendto(%s) failed with: %s", send.GetAddressAsString().c_str(), NetworkError::GetLast().AsString());
 
 		if (!all) break;
 	}