summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPatric Stout <truebrain@openttd.org>2021-05-13 11:46:51 +0200
committerGitHub <noreply@github.com>2021-05-13 11:46:51 +0200
commita403653805c6fd6022868c5f381e10107e1d2b20 (patch)
treeb44e4df40d65cf1fecdadab090badb73d78e0a6d
parent86741ad489c3ee2d519eeb071be846721b90412c (diff)
downloadopenttd-a403653805c6fd6022868c5f381e10107e1d2b20.tar.xz
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.
-rw-r--r--src/network/core/core.h13
-rw-r--r--src/network/core/packet.cpp2
-rw-r--r--src/network/core/tcp.cpp39
-rw-r--r--src/network/core/tcp.h6
-rw-r--r--src/network/core/tcp_admin.cpp4
-rw-r--r--src/network/core/tcp_admin.h1
-rw-r--r--src/network/core/tcp_content.cpp11
-rw-r--r--src/network/core/tcp_content.h2
-rw-r--r--src/network/core/tcp_http.cpp15
-rw-r--r--src/network/core/tcp_http.h2
-rw-r--r--src/network/core/udp.cpp12
-rw-r--r--src/network/core/udp.h6
-rw-r--r--src/network/network_client.cpp4
-rw-r--r--src/network/network_content.cpp18
-rw-r--r--src/network/network_content.h2
-rw-r--r--src/network/network_content_gui.cpp2
-rw-r--r--src/network/network_udp.cpp10
17 files changed, 77 insertions, 72 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();
diff --git a/src/network/network_client.cpp b/src/network/network_client.cpp
index 34746e935..7852a6da9 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 26d220b6a..2b90cf415 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();
- this->CloseConnection();
- this->CloseSocket();
+ if (this->sock == INVALID_SOCKET) return NETWORK_RECV_STATUS_OKAY;
+ 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 13b93417c..b74308a10 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 0cd711877..dd9590e99 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 75bf4563d..0da5a8b26 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<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_server.Close();
- _udp_master.Close();
+ _udp_client.CloseSocket();
+ _udp_server.CloseSocket();
+ _udp_master.CloseSocket();
_network_udp_server = false;
_network_udp_broadcast = 0;