Codechange: [Network] split CloseSocket and CloseConnection more clearly (#9261)

* Codechange: [Network] split CloseSocket and CloseConnection more clearly

- CloseSocket now closes the actual OS socket.
- CloseConnection frees up the resources to just before CloseSocket.
- dtors call CloseSocket / CloseConnection where needed.

src/network/core/core.h

@@ -40,7 +40,9 @@ struct Packet;
  * SocketHandler for all network sockets in OpenTTD.
  */
 class NetworkSocketHandler {
+private:
 	bool has_quit; ///< Whether the current client has quit/send a bad packet
+
 public:
 	/** Create a new unbound socket */
 	NetworkSocketHandler() { this->has_quit = false; }
@@ -49,12 +51,13 @@ public:
 	virtual ~NetworkSocketHandler() {}
 
 	/**
-	 * Close the current connection; for TCP this will be mostly equivalent
+	 * Mark the connection as closed.
-	 * to Close(), but for UDP it just means the packet has to be dropped.
+	 *
-	 * @param error Whether we quit under an error condition or not.
+	 * This doesn't mean the actual connection is closed, but just that we
-	 * @return new status of the connection.
+	 * act like it is. This is useful for UDP, which doesn't normally close
+	 * a socket, but its handler might need to pretend it does.
 	 */
-	virtual NetworkRecvStatus CloseConnection(bool error = true) { this->has_quit = true; return NETWORK_RECV_STATUS_OKAY; }
+	void MarkClosed() { this->has_quit = true; }
 
 	/**
 	 * Whether the current client connected to the socket has quit.

src/network/core/packet.cpp

@@ -222,7 +222,7 @@ bool Packet::CanReadFromPacket(size_t bytes_to_read, bool close_connection)
 
 	/* Check if variable is within packet-size */
 	if (this->pos + bytes_to_read > this->Size()) {
-		if (close_connection) this->cs->NetworkSocketHandler::CloseConnection();
+		if (close_connection) this->cs->NetworkSocketHandler::MarkClosed();
 		return false;
 	}
 

src/network/core/tcp.cpp

@@ -29,24 +29,45 @@ NetworkTCPSocketHandler::NetworkTCPSocketHandler(SOCKET s) :
 
 NetworkTCPSocketHandler::~NetworkTCPSocketHandler()
 {
-	/* Virtual functions get called statically in destructors, so make it explicit to remove any confusion. */
+	this->EmptyPacketQueue();
-	this->NetworkTCPSocketHandler::CloseConnection();
+	this->CloseSocket();
-
-	if (this->sock != INVALID_SOCKET) closesocket(this->sock);
-	this->sock = INVALID_SOCKET;
 }
 
-NetworkRecvStatus NetworkTCPSocketHandler::CloseConnection(bool error)
+/**
+ * Free all pending and partially received packets.
+ */
+void NetworkTCPSocketHandler::EmptyPacketQueue()
 {
-	this->writable = false;
-	NetworkSocketHandler::CloseConnection(error);
-
-	/* Free all pending and partially received packets */
 	while (this->packet_queue != nullptr) {
 		delete Packet::PopFromQueue(&this->packet_queue);
 	}
 	delete this->packet_recv;
 	this->packet_recv = nullptr;
+}
+
+/**
+ * Close the actual socket of the connection.
+ * Please make sure CloseConnection is called before CloseSocket, as
+ * otherwise not all resources might be released.
+ */
+void NetworkTCPSocketHandler::CloseSocket()
+{
+	if (this->sock != INVALID_SOCKET) closesocket(this->sock);
+	this->sock = INVALID_SOCKET;
+}
+
+/**
+ * This will put this socket handler in a close state. It will not
+ * actually close the OS socket; use CloseSocket for this.
+ * @param error Whether we quit under an error condition or not.
+ * @return new status of the connection.
+ */
+NetworkRecvStatus NetworkTCPSocketHandler::CloseConnection(bool error)
+{
+	this->MarkClosed();
+	this->writable = false;
+
+	this->EmptyPacketQueue();
 
 	return NETWORK_RECV_STATUS_OKAY;
 }

src/network/core/tcp.h

@@ -33,6 +33,8 @@ class NetworkTCPSocketHandler : public NetworkSocketHandler {
 private:
 	Packet *packet_queue;     ///< Packets that are awaiting delivery
 	Packet *packet_recv;      ///< Partially received packet
+
+	void EmptyPacketQueue();
 public:
 	SOCKET sock;              ///< The socket currently connected to
 	bool writable;            ///< Can we write to this socket?
@@ -43,7 +45,9 @@ public:
 	 */
 	bool IsConnected() const { return this->sock != INVALID_SOCKET; }
 
-	NetworkRecvStatus CloseConnection(bool error = true) override;
+	virtual NetworkRecvStatus CloseConnection(bool error = true);
+	void CloseSocket();
+
 	virtual void SendPacket(Packet *packet);
 	SendPacketsState SendPackets(bool closing_down = false);
 

src/network/core/tcp_admin.cpp

@@ -34,10 +34,6 @@ NetworkAdminSocketHandler::NetworkAdminSocketHandler(SOCKET s) : status(ADMIN_ST
 	this->admin_version[0] = '\0';
 }
 
-NetworkAdminSocketHandler::~NetworkAdminSocketHandler()
-{
-}
-
 NetworkRecvStatus NetworkAdminSocketHandler::CloseConnection(bool error)
 {
 	delete this;

src/network/core/tcp_admin.h

@@ -482,7 +482,6 @@ public:
 	NetworkRecvStatus CloseConnection(bool error = true) override;
 
 	NetworkAdminSocketHandler(SOCKET s);
-	~NetworkAdminSocketHandler();
 
 	NetworkRecvStatus ReceivePackets();
 

src/network/core/tcp_content.cpp

@@ -137,17 +137,6 @@ const char *ContentInfo::GetTextfile(TextfileType type) const
 	return ::GetTextfile(type, GetContentInfoSubDir(this->type), tmp);
 }
 
-/**
- * Close the actual socket.
- */
-void NetworkContentSocketHandler::CloseSocket()
-{
-	if (this->sock == INVALID_SOCKET) return;
-
-	closesocket(this->sock);
-	this->sock = INVALID_SOCKET;
-}
-
 /**
  * Handle the given packet, i.e. pass it to the right
  * parser receive command.

src/network/core/tcp_content.h

@@ -21,8 +21,6 @@
 /** Base socket handler for all Content TCP sockets */
 class NetworkContentSocketHandler : public NetworkTCPSocketHandler {
 protected:
-	void CloseSocket();
-
 	bool ReceiveInvalidPacket(PacketContentType type);
 
 	/**

src/network/core/tcp_http.cpp

@@ -68,17 +68,18 @@ NetworkHTTPSocketHandler::NetworkHTTPSocketHandler(SOCKET s,
 /** Free whatever needs to be freed. */
 NetworkHTTPSocketHandler::~NetworkHTTPSocketHandler()
 {
-	this->CloseConnection();
+	this->CloseSocket();
 
-	if (this->sock != INVALID_SOCKET) closesocket(this->sock);
-	this->sock = INVALID_SOCKET;
 	free(this->data);
 }
 
-NetworkRecvStatus NetworkHTTPSocketHandler::CloseConnection(bool error)
+/**
+ * Close the actual socket of the connection.
+ */
+void NetworkHTTPSocketHandler::CloseSocket()
 {
-	NetworkSocketHandler::CloseConnection(error);
+	if (this->sock != INVALID_SOCKET) closesocket(this->sock);
-	return NETWORK_RECV_STATUS_OKAY;
+	this->sock = INVALID_SOCKET;
 }
 
 /**
@@ -313,7 +314,7 @@ int NetworkHTTPSocketHandler::Receive()
 			if (ret < 0) cur->callback->OnFailure();
 			if (ret <= 0) {
 				/* Then... the connection can be closed */
-				cur->CloseConnection();
+				cur->CloseSocket();
 				iter = _http_connections.erase(iter);
 				delete cur;
 				continue;

src/network/core/tcp_http.h

@@ -58,7 +58,7 @@ public:
 		return this->sock != INVALID_SOCKET;
 	}
 
-	NetworkRecvStatus CloseConnection(bool error = true) override;
+	void CloseSocket();
 
 	NetworkHTTPSocketHandler(SOCKET sock, HTTPCallback *callback,
 			const char *host, const char *url, const char *data, int depth);

src/network/core/udp.cpp

@@ -44,7 +44,7 @@ NetworkUDPSocketHandler::NetworkUDPSocketHandler(NetworkAddressList *bind)
 bool NetworkUDPSocketHandler::Listen()
 {
 	/* Make sure socket is closed */
-	this->Close();
+	this->CloseSocket();
 
 	for (NetworkAddress &addr : this->bind) {
 		addr.Listen(SOCK_DGRAM, &this->sockets);
@@ -54,9 +54,9 @@ bool NetworkUDPSocketHandler::Listen()
 }
 
 /**
- * Close the given UDP socket
+ * Close the actual UDP socket.
  */
-void NetworkUDPSocketHandler::Close()
+void NetworkUDPSocketHandler::CloseSocket()
 {
 	for (auto &s : this->sockets) {
 		closesocket(s.second);
@@ -64,12 +64,6 @@ void NetworkUDPSocketHandler::Close()
 	this->sockets.clear();
 }
 
-NetworkRecvStatus NetworkUDPSocketHandler::CloseConnection(bool error)
-{
-	NetworkSocketHandler::CloseConnection(error);
-	return NETWORK_RECV_STATUS_OKAY;
-}
-
 /**
  * Send a packet over UDP
  * @param p    the packet to send

src/network/core/udp.h

@@ -49,8 +49,6 @@ protected:
 	/** The opened sockets. */
 	SocketList sockets;
 
-	NetworkRecvStatus CloseConnection(bool error = true) override;
-
 	void ReceiveInvalidPacket(PacketUDPType, NetworkAddress *client_addr);
 
 	/**
@@ -187,10 +185,10 @@ public:
 	NetworkUDPSocketHandler(NetworkAddressList *bind = nullptr);
 
 	/** On destructing of this class, the socket needs to be closed */
-	virtual ~NetworkUDPSocketHandler() { this->Close(); }
+	virtual ~NetworkUDPSocketHandler() { this->CloseSocket(); }
 
 	bool Listen();
-	void Close();
+	void CloseSocket();
 
 	void SendPacket(Packet *p, NetworkAddress *recv, bool all = false, bool broadcast = false);
 	void ReceivePackets();

src/network/network_client.cpp

@@ -158,6 +158,7 @@ ClientNetworkGameSocketHandler::~ClientNetworkGameSocketHandler()
 	ClientNetworkGameSocketHandler::my_client = nullptr;
 
 	delete this->savegame;
+	delete this->GetInfo();
 }
 
 NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvStatus status)
@@ -182,7 +183,6 @@ NetworkRecvStatus ClientNetworkGameSocketHandler::CloseConnection(NetworkRecvSta
 	 * which would trigger the server to close the connection as well. */
 	CSleep(3 * MILLISECONDS_PER_TICK);
 
-	delete this->GetInfo();
 	delete this;
 
 	return status;
@@ -200,7 +200,7 @@ void ClientNetworkGameSocketHandler::ClientError(NetworkRecvStatus res)
 
 	/* We just want to close the connection.. */
 	if (res == NETWORK_RECV_STATUS_CLOSE_QUERY) {
-		this->NetworkSocketHandler::CloseConnection();
+		this->NetworkSocketHandler::MarkClosed();
 		this->CloseConnection(res);
 		_networking = false;
 

src/network/network_content.cpp

@@ -76,7 +76,7 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_INFO(Packet *p)
 
 	if (!ci->IsValid()) {
 		delete ci;
-		this->Close();
+		this->CloseConnection();
 		return false;
 	}
 
@@ -488,7 +488,7 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_CONTENT(Packet *p)
 		p->Recv_string(this->curInfo->filename, lengthof(this->curInfo->filename));
 
 		if (!this->BeforeDownload()) {
-			this->Close();
+			this->CloseConnection();
 			return false;
 		}
 	} else {
@@ -497,7 +497,7 @@ bool ClientNetworkContentSocketHandler::Receive_SERVER_CONTENT(Packet *p)
 		if (toRead != 0 && (size_t)p->TransferOut(TransferOutFWrite, this->curFile) != toRead) {
 			DeleteWindowById(WC_NETWORK_STATUS_WINDOW, WN_NETWORK_STATUS_WINDOW_CONTENT_DOWNLOAD);
 			ShowErrorMessage(STR_CONTENT_ERROR_COULD_NOT_DOWNLOAD, STR_CONTENT_ERROR_COULD_NOT_DOWNLOAD_FILE_NOT_WRITABLE, WL_ERROR);
-			this->Close();
+			this->CloseConnection();
 			fclose(this->curFile);
 			this->curFile = nullptr;
 
@@ -781,14 +781,16 @@ void ClientNetworkContentSocketHandler::Connect()
 /**
  * Disconnect from the content server.
  */
-void ClientNetworkContentSocketHandler::Close()
+NetworkRecvStatus ClientNetworkContentSocketHandler::CloseConnection(bool error)
 {
-	if (this->sock == INVALID_SOCKET) return;
+	NetworkContentSocketHandler::CloseConnection();
+
+	if (this->sock == INVALID_SOCKET) return NETWORK_RECV_STATUS_OKAY;
 
-	this->CloseConnection();
 	this->CloseSocket();
-
 	this->OnDisconnect();
+
+	return NETWORK_RECV_STATUS_OKAY;
 }
 
 /**
@@ -800,7 +802,7 @@ void ClientNetworkContentSocketHandler::SendReceive()
 	if (this->sock == INVALID_SOCKET || this->isConnecting) return;
 
 	if (std::chrono::steady_clock::now() > this->lastActivity + IDLE_TIMEOUT) {
-		this->Close();
+		this->CloseConnection();
 		return;
 	}
 

src/network/network_content.h

@@ -107,7 +107,7 @@ public:
 
 	void Connect();
 	void SendReceive();
-	void Close();
+	NetworkRecvStatus CloseConnection(bool error = true) override;
 
 	void RequestContentList(ContentType type);
 	void RequestContentList(uint count, const ContentID *content_ids);

src/network/network_content_gui.cpp

@@ -260,7 +260,7 @@ public:
 	{
 		if (widget == WID_NCDS_CANCELOK) {
 			if (this->downloaded_bytes != this->total_bytes) {
-				_network_content_client.Close();
+				_network_content_client.CloseConnection();
 				delete this;
 			} else {
 				/* If downloading succeeded, close the online content window. This will close

src/network/network_udp.cpp

@@ -54,10 +54,10 @@ struct UDPSocket {
 
 	UDPSocket(const std::string &name_) : name(name_), socket(nullptr) {}
 
-	void Close()
+	void CloseSocket()
 	{
 		std::lock_guard<std::mutex> lock(mutex);
-		socket->Close();
+		socket->CloseSocket();
 		delete socket;
 		socket = nullptr;
 	}
@@ -619,9 +619,9 @@ void NetworkUDPServerListen()
 /** Close all UDP related stuff. */
 void NetworkUDPClose()
 {
-	_udp_client.Close();
+	_udp_client.CloseSocket();
-	_udp_server.Close();
+	_udp_server.CloseSocket();
-	_udp_master.Close();
+	_udp_master.CloseSocket();
 
 	_network_udp_server = false;
 	_network_udp_broadcast = 0;