From 71035a8706aa89d6cd12f25fea5191fb35787406 Mon Sep 17 00:00:00 2001 From: Tiger Wang Date: Sun, 21 Aug 2016 16:44:25 +0100 Subject: test --- src/ClientHandle.cpp | 72 +++++++++++++++++++++++++++++++++---------------- src/ClientHandle.h | 2 ++ src/Entities/Player.cpp | 6 ----- src/Root.cpp | 4 +-- src/World.cpp | 4 +++ 5 files changed, 57 insertions(+), 31 deletions(-) diff --git a/src/ClientHandle.cpp b/src/ClientHandle.cpp index 8ae81f645..f4aa3e0c5 100644 --- a/src/ClientHandle.cpp +++ b/src/ClientHandle.cpp @@ -104,24 +104,7 @@ cClientHandle::~cClientHandle() { ASSERT(m_State == eState::csDestroyed); // Has Destroy() been called? - if (m_Player != nullptr) - { - cWorld * World = m_Player->GetWorld(); - - // Upon clienthandle destruction, the world pointer of a valid player SHALL NOT be null. - // The only time where it can be null is after player construction but before cEntity::Initialize is called in the queued task in Authenticate. - // During this time, it is guaranteed that the clienthandle is not deleted. - ASSERT(World != nullptr); - - World->QueueTask( - [this](cWorld & a_World) - { - a_World.RemovePlayer(m_Player); - a_World.BroadcastPlayerListRemovePlayer(*m_Player); - m_Player->Destroy(); - } - ); - } + // Note: don't handle player destruction here because we don't own them, so problems arise during shutdown LOGD("Deletied client \"%s\" at %p", GetUsername().c_str(), static_cast(this)); } @@ -144,6 +127,38 @@ void cClientHandle::Destroy(void) m_Link.reset(); } + if (m_Player != nullptr) + { + cWorld * World = m_Player->GetWorld(); + + // Upon Destroy, the world pointer of a valid player SHALL NOT be null. + // Guaranteed by Destroy and Authenticate being called in the same thread + ASSERT(World != nullptr); + + // We do not leak a player object as Authenticate checks to see if Destroy was called before creating a player + World->QueueTask( + [this](cWorld & a_World) + { + if (m_Player->GetParentChunk() == nullptr) + { + // Player not yet initialised; calling Destroy is not valid + LOGWARN("ParentChunk null"); + return; + } + + if (!cRoot::Get()->GetPluginManager()->CallHookPlayerDestroyed(*m_Player)) + { + cRoot::Get()->BroadcastChatLeave(Printf("%s has left the game", m_Player->GetName().c_str())); + LOGINFO("Player %s has left the game", m_Player->GetName().c_str()); + } + + a_World.RemovePlayer(m_Player); + a_World.BroadcastPlayerListRemovePlayer(*m_Player); + m_Player->Destroy(); + } + ); + } + RemoveFromWorld(); LOGD("%s: client %p, \"%s\"", __FUNCTION__, static_cast(this), m_Username.c_str()); @@ -309,8 +324,11 @@ void cClientHandle::Kick(const AString & a_Reason) void cClientHandle::Authenticate(const AString & a_Name, const AString & a_UUID, const Json::Value & a_Properties) { + ASSERT(cRoot::Get()->GetServer()->IsInTickThread()); + if (m_State != eState::csAuthenticating) { + // TODO: is this necessary? return; } @@ -348,12 +366,22 @@ void cClientHandle::Authenticate(const AString & a_Name, const AString & a_UUID, } m_Player->SetIP(m_IPString); + // So Destroy always has a valid world for a valid player + // Additionally, plugins expect world to be set + m_Player->SetWorld(World); + cpp14::move_on_copy_wrapper PlayerPtr(std::move(Player)); World->QueueTask( [World, Player = m_Player, PlayerPtr, Client](cWorld & a_World) mutable { - // Plugins expect world to be set: - Player->SetWorld(World); + // We're in the task to create the player - any other QueueTask will execute in the next tick + // So, check to see cClientHandle::Destroy has not been called (State >= IsDestroying where State is monotonic) + if (Client->IsDestroying() || Client->IsDestroyed()) + { + // Destroy called - proceeding now means we may create a player object that's never deleted + LOGWARN("Destroyed"); + return; + } if (!cRoot::Get()->GetPluginManager()->CallHookPlayerJoined(*Player)) { @@ -386,9 +414,7 @@ void cClientHandle::Authenticate(const AString & a_Name, const AString & a_UUID, cRoot::Get()->BroadcastPlayerListsAddPlayer(*Player); cRoot::Get()->SendPlayerLists(Player); - // Note: cEnttiy::Initialize takes ownership of the player object, however: - // 1. It is guaranteed to exist whilst the server is running and this clienthandle is alive - // 2. In the case that the server is stopping, it is guaranteed that we are destroyed before the player object is destroyed + // Note: cEnttiy::Initialize takes ownership of the player object Player->Initialize(std::move(PlayerPtr.value), *World); World->AddPlayer(Player); diff --git a/src/ClientHandle.h b/src/ClientHandle.h index 0e60d1f0a..44261827d 100644 --- a/src/ClientHandle.h +++ b/src/ClientHandle.h @@ -455,6 +455,8 @@ private: /** Protects m_Link against multithreaded access. */ cCriticalSection m_CSLink; + /** Non-owning pointer to the player object we are associated with + It is guaranteed to exist whilst the server is running and this clienthandle is alive */ cPlayer * m_Player; /** Number of ticks since the last network packet was received (increased in Tick(), reset in OnReceivedData()) */ diff --git a/src/Entities/Player.cpp b/src/Entities/Player.cpp index babe31978..390e4ad52 100644 --- a/src/Entities/Player.cpp +++ b/src/Entities/Player.cpp @@ -158,12 +158,6 @@ cPlayer::cPlayer(std::weak_ptr a_Client, const AString & a_Player cPlayer::~cPlayer(void) { - if (!cRoot::Get()->GetPluginManager()->CallHookPlayerDestroyed(*this)) - { - cRoot::Get()->BroadcastChatLeave(Printf("%s has left the game", GetName().c_str())); - LOGINFO("Player %s has left the game", GetName().c_str()); - } - LOGD("Deleting cPlayer \"%s\" at %p, ID %d", GetName().c_str(), static_cast(this), GetUniqueID()); // Notify the server that the player is being destroyed diff --git a/src/Root.cpp b/src/Root.cpp index ec31e57a2..3749aaf64 100644 --- a/src/Root.cpp +++ b/src/Root.cpp @@ -572,7 +572,7 @@ bool cRoot::ForEachWorld(cWorldListCallback & a_Callback) { for (const auto & World : m_WorldsByName) { - if (a_Callback.Item(World.second.get())) + if (a_Callback.Item(World.second)) { return false; } @@ -960,7 +960,7 @@ void cRoot::LogChunkStats(cCommandOutputCallback & a_Output) int SumMem = 0; for (const auto & WorldEntry : m_WorldsByName) { - auto World = WorldEntry.second.get(); + auto World = WorldEntry.second; int NumInGenerator = World->GetGeneratorQueueLength(); int NumInSaveQueue = static_cast(World->GetStorageSaveQueueLength()); int NumInLoadQueue = static_cast(World->GetStorageLoadQueueLength()); diff --git a/src/World.cpp b/src/World.cpp index 242345f02..a9d6b2b13 100644 --- a/src/World.cpp +++ b/src/World.cpp @@ -253,6 +253,10 @@ cWorld::~cWorld() Serializer.Save(); m_MapManager.SaveMapData(); + + // Explicitly destroy the chunkmap, so that it's guaranteed to be destroyed before the other internals + // This fixes crashes on stopping the server, because chunk destructor deletes entities and those access the world. + m_ChunkMap.reset(); } -- cgit v1.2.3