From a14e015354611a50920a0861f80c9691ab8fa633 Mon Sep 17 00:00:00 2001 From: "madmaxoft@gmail.com" Date: Sat, 25 Feb 2012 23:48:28 +0000 Subject: Extended SocketThreads for writing support (unusable in cClientHandle due to too many deadlock possibilities) Extended the range of ignored packets in pre-game client states (fixes connection issues with some mods installed) git-svn-id: http://mc-server.googlecode.com/svn/trunk@327 0a769ca7-a7f5-676a-18bf-c427514a06d6 --- source/cClientHandle.cpp | 17 ++-- source/cServer.cpp | 2 +- source/cSocketThreads.cpp | 209 +++++++++++++++++++++++++++++++++++++++++----- source/cSocketThreads.h | 23 +++++ 4 files changed, 218 insertions(+), 33 deletions(-) diff --git a/source/cClientHandle.cpp b/source/cClientHandle.cpp index 78032611e..9eed31a3a 100644 --- a/source/cClientHandle.cpp +++ b/source/cClientHandle.cpp @@ -151,7 +151,7 @@ cClientHandle::~cClientHandle() { LOG("Deleting client \"%s\"", GetUsername().c_str()); - // Remove from cSocketThreads, just in case + // Remove from cSocketThreads, we're not to be called anymore: cRoot::Get()->GetServer()->ClientDestroying(this); m_LoadedChunks.clear(); @@ -225,14 +225,6 @@ void cClientHandle::Destroy() { RemoveFromAllChunks(); } - - if (m_Socket.IsValid()) - { - m_Socket.CloseSocket(); - } - - // Synchronize with the cSocketThreads (so that they don't call us anymore) - cRoot::Get()->GetServer()->ClientDestroying(this); } @@ -489,6 +481,7 @@ void cClientHandle::HandlePacket(cPacket * a_Packet) // Ignored packets: case E_PLAYERLOOK: + case E_CHAT: case E_PLAYERMOVELOOK: case E_PLAYERPOS: case E_KEEP_ALIVE: break; @@ -504,6 +497,8 @@ void cClientHandle::HandlePacket(cPacket * a_Packet) { // Ignored packets: case E_KEEP_ALIVE: + case E_CHAT: + case E_FLYING: case E_PLAYERLOOK: case E_PLAYERMOVELOOK: case E_PLAYERPOS: break; @@ -520,6 +515,8 @@ void cClientHandle::HandlePacket(cPacket * a_Packet) { // Ignored packets: case E_KEEP_ALIVE: + case E_CHAT: + case E_FLYING: case E_PLAYERLOOK: case E_PLAYERMOVELOOK: case E_PLAYERPOS: break; @@ -535,6 +532,8 @@ void cClientHandle::HandlePacket(cPacket * a_Packet) { // Ignored packets: case E_KEEP_ALIVE: + case E_CHAT: + case E_FLYING: case E_PLAYERLOOK: case E_PLAYERPOS: break; diff --git a/source/cServer.cpp b/source/cServer.cpp index 8462fa915..97bcffaae 100644 --- a/source/cServer.cpp +++ b/source/cServer.cpp @@ -102,7 +102,7 @@ void cServer::ServerListenThread( void *a_Args ) void cServer::ClientDestroying(const cClientHandle * a_Client) { - m_SocketThreads.RemoveClient(a_Client); + m_SocketThreads.StopReading(a_Client); } diff --git a/source/cSocketThreads.cpp b/source/cSocketThreads.cpp index aa0aba678..5e98398c4 100644 --- a/source/cSocketThreads.cpp +++ b/source/cSocketThreads.cpp @@ -85,7 +85,9 @@ void cSocketThreads::RemoveClient(const cSocket * a_Socket) { return; } - } + } // for itr - m_Threads[] + + ASSERT(!"Removing an unknown socket"); } @@ -103,7 +105,9 @@ void cSocketThreads::RemoveClient(const cCallback * a_Client) { return; } - } + } // for itr - m_Threads[] + + ASSERT(!"Removing an unknown client"); } @@ -121,7 +125,81 @@ void cSocketThreads::NotifyWrite(const cCallback * a_Client) { return; } + } // for itr - m_Threads[] + + // Cannot assert - this normally happens if a client disconnects and has pending packets, the cServer::cNotifyWriteThread will call this on invalid clients too + // ASSERT(!"Notifying write to an unknown client"); +} + + + + + +void cSocketThreads::Write(const cSocket * a_Socket, const AString & a_Data) +{ + // Puts a_Data into outgoing data queue for a_Socket + + if (!a_Socket->IsValid()) + { + // Socket already closed, ignore the request + return; + } + + cCSLock Lock(m_CS); + for (cSocketThreadList::iterator itr = m_Threads.begin(); itr != m_Threads.end(); ++itr) + { + if ((*itr)->Write(a_Socket, a_Data)) + { + return; + } + } // for itr - m_Threads[] + + ASSERT(!"Writing to an unknown socket"); +} + + + + + +/// Stops reading from the socket - when this call returns, no more calls to the callbacks are made +void cSocketThreads::StopReading(const cCallback * a_Client) +{ + cCSLock Lock(m_CS); + for (cSocketThreadList::iterator itr = m_Threads.begin(); itr != m_Threads.end(); ++itr) + { + if ((*itr)->StopReading(a_Client)) + { + return; + } + } // for itr - m_Threads[] + + // Cannot assert, this normally happens if the socket is closed before the client deinitializes + // ASSERT(!"Stopping reading on an unknown client"); +} + + + + + +/// Queues the socket for closing, as soon as its outgoing data is sent +void cSocketThreads::QueueClose(const cSocket * a_Socket) +{ + if (!a_Socket->IsValid()) + { + // Already closed, ignore the request + return; } + + cCSLock Lock(m_CS); + for (cSocketThreadList::iterator itr = m_Threads.begin(); itr != m_Threads.end(); ++itr) + { + if ((*itr)->QueueClose(a_Socket)) + { + return; + } + } // for itr - m_Threads[] + + ASSERT(!"Queueing close of an unknown socket"); } @@ -189,8 +267,7 @@ bool cSocketThreads::cSocketThread::RemoveClient(const cCallback * a_Client) } // Found, remove it: - m_Slots[i] = m_Slots[m_NumSlots - 1]; - m_NumSlots--; + m_Slots[i] = m_Slots[--m_NumSlots]; // Notify the thread of the change: ASSERT(m_ControlSocket2.IsValid()); @@ -210,11 +287,6 @@ bool cSocketThreads::cSocketThread::RemoveSocket(const cSocket * a_Socket) { // Returns true if removed, false if not found - if (m_NumSlots == 0) - { - return false; - } - for (int i = m_NumSlots - 1; i >= 0 ; --i) { if (m_Slots[i].m_Socket != a_Socket) @@ -223,8 +295,7 @@ bool cSocketThreads::cSocketThread::RemoveSocket(const cSocket * a_Socket) } // Found, remove it: - m_Slots[i] = m_Slots[m_NumSlots - 1]; - m_NumSlots--; + m_Slots[i] = m_Slots[--m_NumSlots]; // Notify the thread of the change: ASSERT(m_ControlSocket2.IsValid()); @@ -240,6 +311,38 @@ bool cSocketThreads::cSocketThread::RemoveSocket(const cSocket * a_Socket) +bool cSocketThreads::cSocketThread::HasClient(const cCallback * a_Client) const +{ + for (int i = m_NumSlots - 1; i >= 0; --i) + { + if (m_Slots[i].m_Client == a_Client) + { + return true; + } + } // for i - m_Slots[] + return false; +} + + + + + +bool cSocketThreads::cSocketThread::HasSocket(const cSocket * a_Socket) const +{ + for (int i = m_NumSlots - 1; i >= 0; --i) + { + if (m_Slots[i].m_Socket->GetSocket() == a_Socket->GetSocket()) + { + return true; + } + } // for i - m_Slots[] + return false; +} + + + + + bool cSocketThreads::cSocketThread::NotifyWrite(const cCallback * a_Client) { if (HasClient(a_Client)) @@ -256,12 +359,43 @@ bool cSocketThreads::cSocketThread::NotifyWrite(const cCallback * a_Client) -bool cSocketThreads::cSocketThread::HasClient(const cCallback * a_Client) const +bool cSocketThreads::cSocketThread::Write(const cSocket * a_Socket, const AString & a_Data) { + // Returns true if socket handled by this thread + for (int i = m_NumSlots - 1; i >= 0; --i) + { + if (m_Slots[i].m_Socket == a_Socket) + { + m_Slots[i].m_Outgoing.append(a_Data); + + // Notify the thread that there's data in the queue: + ASSERT(m_ControlSocket2.IsValid()); + m_ControlSocket2.Send("q", 1); + + return true; + } + } // for i - m_Slots[] + return false; +} + + + + + +bool cSocketThreads::cSocketThread::StopReading (const cCallback * a_Client) +{ + // Returns true if client handled by this thread for (int i = m_NumSlots - 1; i >= 0; --i) { if (m_Slots[i].m_Client == a_Client) { + m_Slots[i].m_Client = NULL; + m_Slots[i].m_ShouldClose = false; + + // Notify the thread that there's a stop reading request: + ASSERT(m_ControlSocket2.IsValid()); + m_ControlSocket2.Send("s", 1); + return true; } } // for i - m_Slots[] @@ -272,12 +406,20 @@ bool cSocketThreads::cSocketThread::HasClient(const cCallback * a_Client) const -bool cSocketThreads::cSocketThread::HasSocket(const cSocket * a_Socket) const +bool cSocketThreads::cSocketThread::QueueClose(const cSocket * a_Socket) { + // Returns true if socket handled by this thread for (int i = m_NumSlots - 1; i >= 0; --i) { - if (m_Slots[i].m_Socket->GetSocket() == a_Socket->GetSocket()) + if (m_Slots[i].m_Socket == a_Socket) { + ASSERT(m_Slots[i].m_Client == NULL); // Should have stopped reading first + m_Slots[i].m_ShouldClose = true; + + // Notify the thread that there's a close queued (in case its conditions are already met): + ASSERT(m_ControlSocket2.IsValid()); + m_ControlSocket2.Send("c", 1); + return true; } } // for i - m_Slots[] @@ -453,17 +595,26 @@ void cSocketThreads::cSocketThread::ReadFromSockets(fd_set * a_Read) { // The socket has been closed by the remote party, close our socket and let it be removed after we process all reading m_Slots[i].m_Socket->CloseSocket(); - m_Slots[i].m_Client->SocketClosed(); + if (m_Slots[i].m_Client != NULL) + { + m_Slots[i].m_Client->SocketClosed(); + } } else if (Received > 0) { - m_Slots[i].m_Client->DataReceived(Buffer, Received); + if (m_Slots[i].m_Client != NULL) + { + m_Slots[i].m_Client->DataReceived(Buffer, Received); + } } else { // The socket has encountered an error, close it and let it be removed after we process all reading m_Slots[i].m_Socket->CloseSocket(); - m_Slots[i].m_Client->SocketClosed(); + if (m_Slots[i].m_Client != NULL) + { + m_Slots[i].m_Client->SocketClosed(); + } } } // for i - m_Slots[] } @@ -485,10 +636,19 @@ void cSocketThreads::cSocketThread::WriteToSockets(fd_set * a_Write) if (m_Slots[i].m_Outgoing.empty()) { // Request another chunk of outgoing data: - m_Slots[i].m_Client->GetOutgoingData(m_Slots[i].m_Outgoing); + if (m_Slots[i].m_Client != NULL) + { + m_Slots[i].m_Client->GetOutgoingData(m_Slots[i].m_Outgoing); + } if (m_Slots[i].m_Outgoing.empty()) { - // Nothing ready yet + // Nothing ready + if ((m_Slots[i].m_Client == NULL) && m_Slots[i].m_ShouldClose) + { + // Socket was queued for closing and there's no more data to send, close it now: + m_Slots[i].m_Socket->CloseSocket(); + m_Slots[i] = m_Slots[--m_NumSlots]; + } continue; } } // if (outgoing data is empty) @@ -496,9 +656,13 @@ void cSocketThreads::cSocketThread::WriteToSockets(fd_set * a_Write) int Sent = m_Slots[i].m_Socket->Send(m_Slots[i].m_Outgoing.data(), m_Slots[i].m_Outgoing.size()); if (Sent < 0) { - LOGWARNING("Error while writing to client \"%s\", disconnecting", m_Slots[i].m_Socket->GetIPString().c_str()); + int Err = cSocket::GetLastError(); + LOGWARNING("Error %d while writing to client \"%s\", disconnecting. \"%s\"", Err, m_Slots[i].m_Socket->GetIPString().c_str(), cSocket::GetErrorString(Err).c_str()); m_Slots[i].m_Socket->CloseSocket(); - m_Slots[i].m_Client->SocketClosed(); + if (m_Slots[i].m_Client != NULL) + { + m_Slots[i].m_Client->SocketClosed(); + } return; } m_Slots[i].m_Outgoing.erase(0, Sent); @@ -531,8 +695,7 @@ void cSocketThreads::cSocketThread::RemoveClosedSockets(void) { continue; } - m_Slots[i] = m_Slots[m_NumSlots - 1]; - m_NumSlots--; + m_Slots[i] = m_Slots[--m_NumSlots]; } // for i - m_Slots[] } diff --git a/source/cSocketThreads.h b/source/cSocketThreads.h index a32808aeb..a8174a115 100644 --- a/source/cSocketThreads.h +++ b/source/cSocketThreads.h @@ -5,6 +5,16 @@ // This object takes care of network communication, groups sockets into threads and uses as little threads as possible for full read / write support // For more detail, see http://forum.mc-server.org/showthread.php?tid=327 +/* +Additional details: +When a client is terminating a connection: +- they call the StopReading() method to disable callbacks for the incoming data +- they call the Write() method to queue any outstanding outgoing data +- they call the QueueClose() method to queue the socket to close after outgoing data has been sent. +When a socket slot is marked as having no callback, it is kept alive until its outgoing data queue is empty and its m_ShouldClose flag is set. +This means that the socket can be written to several times before finally closing it via QueueClose() +*/ + @@ -76,6 +86,15 @@ public: /// Notify the thread responsible for a_Client that the client has something to write void NotifyWrite(const cCallback * a_Client); + + /// Puts a_Data into outgoing data queue for a_Socket + void Write(const cSocket * a_Socket, const AString & a_Data); + + /// Stops reading from the socket - when this call returns, no more calls to the callbacks are made + void StopReading(const cCallback * a_Client); + + /// Queues the socket for closing, as soon as its outgoing data is sent + void QueueClose(const cSocket * a_Socket); private: @@ -99,6 +118,9 @@ private: bool HasClient (const cCallback * a_Client) const; bool HasSocket (const cSocket * a_Socket) const; bool NotifyWrite (const cCallback * a_Client); // Returns true if client handled by this thread + bool Write (const cSocket * a_Socket, const AString & a_Data); // Returns true if socket handled by this thread + bool StopReading (const cCallback * a_Client); // Returns true if client handled by this thread + bool QueueClose (const cSocket * a_Socket); // Returns true if socket handled by this thread bool Start(void); // Hide the cIsThread's Start method, we need to provide our own startup to create the control socket @@ -119,6 +141,7 @@ private: cSocket * m_Socket; cCallback * m_Client; AString m_Outgoing; // If sending writes only partial data, the rest is stored here for another send + bool m_ShouldClose; // If true, the socket is to be closed after sending all outgoing data } ; sSlot m_Slots[MAX_SLOTS]; int m_NumSlots; // Number of slots actually used -- cgit v1.2.3