From 054a89dd9e5d6819adede9d7ba781b69f98ff2f4 Mon Sep 17 00:00:00 2001 From: Tiger Wang Date: Wed, 6 Jan 2021 00:35:42 +0000 Subject: Clarify cClientHandle, cPlayer ownership semantics + A cPlayer, once created, has a strong pointer to the cClientHandle. The player ticks the clienthandle. If he finds the handle destroyed, he destroys himself in turn. Nothing else can kill the player. * The client handle has a pointer to the player. Once a player is created, the client handle never outlasts the player, nor does it manage the player's lifetime. The pointer is always safe to use after FinishAuthenticate, which is also the point where cProtocol is put into the Game state that allows player manipulation. + Entities are once again never lost by constructing a chunk when they try to move into one that doesn't exist. * Fixed a forgotten Super invocation in cPlayer::OnRemoveFromWorld. * Fix SaveToDisk usage in destructor by only saving things cPlayer owns, instead of accessing cWorld. --- src/ChunkSender.cpp | 75 +++++++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 37 deletions(-) (limited to 'src/ChunkSender.cpp') diff --git a/src/ChunkSender.cpp b/src/ChunkSender.cpp index c93a764b2..0a7f58bc7 100644 --- a/src/ChunkSender.cpp +++ b/src/ChunkSender.cpp @@ -104,13 +104,13 @@ void cChunkSender::QueueSendChunkTo(int a_ChunkX, int a_ChunkZ, Priority a_Prior m_SendChunks.push(sChunkQueue{a_Priority, Chunk}); info.m_Priority = a_Priority; } - info.m_Clients.insert(a_Client); + info.m_Clients.insert(a_Client->shared_from_this()); } else { m_SendChunks.push(sChunkQueue{a_Priority, Chunk}); auto info = sSendChunk{Chunk, a_Priority}; - info.m_Clients.insert(a_Client); + info.m_Clients.insert(a_Client->shared_from_this()); m_ChunkInfo.emplace(Chunk, info); } } @@ -135,13 +135,19 @@ void cChunkSender::QueueSendChunkTo(int a_ChunkX, int a_ChunkZ, Priority a_Prior m_SendChunks.push(sChunkQueue{a_Priority, Chunk}); info.m_Priority = a_Priority; } - info.m_Clients.insert(a_Clients.begin(), a_Clients.end()); + for (const auto & Client : a_Clients) + { + info.m_Clients.insert(Client->shared_from_this()); + } } else { m_SendChunks.push(sChunkQueue{a_Priority, Chunk}); auto info = sSendChunk{Chunk, a_Priority}; - info.m_Clients.insert(a_Clients.begin(), a_Clients.end()); + for (const auto & Client : a_Clients) + { + info.m_Clients.insert(Client->shared_from_this()); + } m_ChunkInfo.emplace(Chunk, info); } } @@ -152,24 +158,6 @@ void cChunkSender::QueueSendChunkTo(int a_ChunkX, int a_ChunkZ, Priority a_Prior -void cChunkSender::RemoveClient(cClientHandle * a_Client) -{ - { - cCSLock Lock(m_CS); - for (auto && pair : m_ChunkInfo) - { - auto && clients = pair.second.m_Clients; - clients.erase(a_Client); // nop for sets that do not contain a_Client - } - } - m_evtQueue.Set(); - m_evtRemoved.Wait(); // Wait for all remaining instances of a_Client to be processed (Execute() makes a copy of m_ChunkInfo) -} - - - - - void cChunkSender::Execute(void) { while (!m_ShouldTerminate) @@ -189,16 +177,13 @@ void cChunkSender::Execute(void) continue; } - std::unordered_set clients; - std::swap(itr->second.m_Clients, clients); + auto clients = std::move(itr->second.m_Clients); m_ChunkInfo.erase(itr); cCSUnlock Unlock(Lock); SendChunk(Chunk.m_ChunkX, Chunk.m_ChunkZ, clients); } } - - m_evtRemoved.SetAll(); // Notify all waiting threads that all clients are processed and thus safe to destroy } // while (!m_ShouldTerminate) } @@ -206,21 +191,27 @@ void cChunkSender::Execute(void) -void cChunkSender::SendChunk(int a_ChunkX, int a_ChunkZ, std::unordered_set a_Clients) +void cChunkSender::SendChunk(int a_ChunkX, int a_ChunkZ, const WeakClients & a_Clients) { + // Contains strong pointers to clienthandles. + std::vector> Clients; + // Ask the client if it still wants the chunk: - for (auto itr = a_Clients.begin(); itr != a_Clients.end();) + for (const auto & WeakClient : a_Clients) { - if (!(*itr)->WantsSendChunk(a_ChunkX, a_ChunkZ)) + auto Client = WeakClient.lock(); + if ((Client != nullptr) && Client->WantsSendChunk(a_ChunkX, a_ChunkZ)) { - itr = a_Clients.erase(itr); - } - else - { - itr++; + Clients.push_back(std::move(Client)); } } + // Bail early if every requester disconnected: + if (Clients.empty()) + { + return; + } + // If the chunk has no clients, no need to packetize it: if (!m_World.HasChunkAnyClients(a_ChunkX, a_ChunkZ)) { @@ -247,9 +238,9 @@ void cChunkSender::SendChunk(int a_ChunkX, int a_ChunkZ, std::unordered_setGetUsername().c_str() ); */ - a_Entity.SpawnOn(*Client); + + /* This check looks highly suspect. + Its purpose is to check the client still has a valid player object associated, + since the player destroys itself when the client is destroyed. + It's done within the world lock to ensure correctness. + A better way involves fixing chunk sending (GH #3696) to obviate calling SpawnOn from this thread in the first place. */ + if (!Client->IsDestroyed()) + { + a_Entity.SpawnOn(*Client); + } + return true; }); } -- cgit v1.2.3