From a403653805c6fd6022868c5f381e10107e1d2b20 Mon Sep 17 00:00:00 2001 From: Patric Stout Date: Thu, 13 May 2021 11:46:51 +0200 Subject: [PATCH] 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 | 13 +++++---- src/network/core/packet.cpp | 2 +- src/network/core/tcp.cpp | 41 ++++++++++++++++++++++------- src/network/core/tcp.h | 6 ++++- src/network/core/tcp_admin.cpp | 4 --- src/network/core/tcp_admin.h | 1 - src/network/core/tcp_content.cpp | 11 -------- src/network/core/tcp_content.h | 2 -- src/network/core/tcp_http.cpp | 15 ++++++----- src/network/core/tcp_http.h | 2 +- src/network/core/udp.cpp | 12 +++------ src/network/core/udp.h | 6 ++--- src/network/network_client.cpp | 4 +-- src/network/network_content.cpp | 18 +++++++------ src/network/network_content.h | 2 +- src/network/network_content_gui.cpp | 2 +- src/network/network_udp.cpp | 10 +++---- 17 files changed, 78 insertions(+), 73 deletions(-) diff --git a/src/network/core/core.h b/src/network/core/core.h index a16ed9f23b..3e470ef5f1 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 7369707910..883097dea1 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 5c436edf05..d5754f69a1 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(); - - if (this->sock != INVALID_SOCKET) closesocket(this->sock); - this->sock = INVALID_SOCKET; + this->EmptyPacketQueue(); + this->CloseSocket(); } -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; } diff --git a/src/network/core/tcp.h b/src/network/core/tcp.h index 44316bfca0..3b217cb2e1 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 8cc8b1efe6..0b48b419b6 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 e5bcefa86f..8b4a738bfa 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 3abf1c29c9..a53a352c28 100644 --- a/src/network/core/tcp_content.cpp +++ b/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. diff --git a/src/network/core/tcp_content.h b/src/network/core/tcp_content.h index b1bde48172..d99986ef26 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 08961e4029..f11722ae38 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 d7be0c327b..da7a04ac48 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 c8d7533640..312ac0f0a2 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 ab898eeee3..489e219856 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(); diff --git a/src/network/network_client.cpp b/src/network/network_client.cpp index 34746e935c..7852a6da9e 100644 --- a/src/network/network_client.cpp +++ b/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; diff --git a/src/network/network_content.cpp b/src/network/network_content.cpp index 26d220b6ae..2b90cf4153 100644 --- a/src/network/network_content.cpp +++ b/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; } diff --git a/src/network/network_content.h b/src/network/network_content.h index 13b93417c3..b74308a100 100644 --- a/src/network/network_content.h +++ b/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); diff --git a/src/network/network_content_gui.cpp b/src/network/network_content_gui.cpp index 0cd711877d..dd9590e99c 100644 --- a/src/network/network_content_gui.cpp +++ b/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 diff --git a/src/network/network_udp.cpp b/src/network/network_udp.cpp index 75bf4563d3..0da5a8b260 100644 --- a/src/network/network_udp.cpp +++ b/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 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_server.Close(); - _udp_master.Close(); + _udp_client.CloseSocket(); + _udp_server.CloseSocket(); + _udp_master.CloseSocket(); _network_udp_server = false; _network_udp_broadcast = 0;