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.

12 files changed, 58 insertions, 55 deletions

diff --git a/src/network/core/core.h b/src/network/core/core.h
index a16ed9f23..3e470ef5f 100644
--- a/src/network/core/core.h
+++ b/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
-	 * 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.
-	 * @return new status of the connection.
+	 * Mark the connection as closed.
+	 *
+	 * This doesn't mean the actual connection is closed, but just that we
+	 * 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.
diff --git a/src/network/core/packet.cpp b/src/network/core/packet.cpp
index 736970791..883097dea 100644
--- a/src/network/core/packet.cpp
+++ b/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;
 	}
 
diff --git a/src/network/core/tcp.cpp b/src/network/core/tcp.cpp
index 5c436edf0..d5754f69a 100644
--- a/src/network/core/tcp.cpp
+++ b/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->NetworkTCPSocketHandler::CloseConnection();
+	this->EmptyPacketQueue();
+	this->CloseSocket();
+}
+
+/**
+ * Free all pending and partially received packets.
+ */
+void NetworkTCPSocketHandler::EmptyPacketQueue()
+{
+	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;
-	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;
+	this->EmptyPacketQueue();
 
 	return NETWORK_RECV_STATUS_OKAY;
 }
diff --git a/src/network/core/tcp.h b/src/network/core/tcp.h
index 44316bfca..3b217cb2e 100644
--- a/src/network/core/tcp.h
+++ b/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);
 
diff --git a/src/network/core/tcp_admin.cpp b/src/network/core/tcp_admin.cpp
index 8cc8b1efe..0b48b419b 100644
--- a/src/network/core/tcp_admin.cpp
+++ b/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;
diff --git a/src/network/core/tcp_admin.h b/src/network/core/tcp_admin.h
index e5bcefa86..8b4a738bf 100644
--- a/src/network/core/tcp_admin.h
+++ b/src/network/core/tcp_admin.h
@@ -482,7 +482,6 @@ public:
 	NetworkRecvStatus CloseConnection(bool error = true) override;
 
 	NetworkAdminSocketHandler(SOCKET s);
-	~NetworkAdminSocketHandler();
 
 	NetworkRecvStatus ReceivePackets();
 
diff --git a/src/network/core/tcp_content.cpp b/src/network/core/tcp_content.cpp
index 3abf1c29c..a53a352c2 100644
--- a/src/network/core/tcp_content.cpp
+++ b/src/network/core/tcp_content.cpp
@@ -138,17 +138,6 @@ const char *ContentInfo::GetTextfile(TextfileType type) const
 }
 
 /**
- * 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.
  * @param p the packet to handle
diff --git a/src/network/core/tcp_content.h b/src/network/core/tcp_content.h
index b1bde4817..d99986ef2 100644
--- a/src/network/core/tcp_content.h
+++ b/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);
 
 	/**
diff --git a/src/network/core/tcp_http.cpp b/src/network/core/tcp_http.cpp
index 08961e402..f11722ae3 100644
--- a/src/network/core/tcp_http.cpp
+++ b/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);
-	return NETWORK_RECV_STATUS_OKAY;
+	if (this->sock != INVALID_SOCKET) closesocket(this->sock);
+	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;
diff --git a/src/network/core/tcp_http.h b/src/network/core/tcp_http.h
index d7be0c327..da7a04ac4 100644
--- a/src/network/core/tcp_http.h
+++ b/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);
diff --git a/src/network/core/udp.cpp b/src/network/core/udp.cpp
index c8d753364..312ac0f0a 100644
--- a/src/network/core/udp.cpp
+++ b/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
diff --git a/src/network/core/udp.h b/src/network/core/udp.h
index ab898eeee..489e21985 100644
--- a/src/network/core/udp.h
+++ b/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();