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.
2021-01-06 00:35:42 +00:00
parent 9328afe65c
commit 054a89dd9e
21 changed files with 505 additions and 1126 deletions
--- a/src/ClientHandle.cpp
+++ b/src/ClientHandle.cpp
@@ -68,13 +68,12 @@ float cClientHandle::FASTBREAK_PERCENTAGE;
 // cClientHandle:

 cClientHandle::cClientHandle(const AString & a_IPString, int a_ViewDistance) :
-	m_LastSentDimension(dimNotSet),
 	m_ForgeHandshake(this),
 	m_CurrentViewDistance(a_ViewDistance),
 	m_RequestedViewDistance(a_ViewDistance),
 	m_IPString(a_IPString),
 	m_Player(nullptr),
-	m_CachedSentChunk(0, 0),
+	m_CachedSentChunk(0x7fffffff, 0x7fffffff),
 	m_HasSentDC(false),
 	m_LastStreamedChunkX(0x7fffffff),  // bogus chunk coords to force streaming upon login
 	m_LastStreamedChunkZ(0x7fffffff),
@@ -116,26 +115,6 @@ cClientHandle::~cClientHandle()

 	LOGD("Deleting client \"%s\" at %p", GetUsername().c_str(), static_cast<void *>(this));

-	{
-		cCSLock Lock(m_CSChunkLists);
-		m_LoadedChunks.clear();
-		m_ChunksToSend.clear();
-	}
-
-	if (m_Player != nullptr)
-	{
-		cWorld * World = m_Player->GetWorld();
-		if (World != nullptr)
-		{
-			m_Player->GetWorld()->RemoveClientFromChunkSender(this);
-		}
-		// Send the Offline PlayerList packet:
-		cRoot::Get()->BroadcastPlayerListsRemovePlayer(*m_Player);
-
-		m_PlayerPtr.reset();
-		m_Player = nullptr;
-	}
-
 	LOGD("ClientHandle at %p deleted", static_cast<void *>(this));
 }

@@ -145,12 +124,7 @@ cClientHandle::~cClientHandle()

 void cClientHandle::Destroy(void)
 {
-	{
-		cCSLock Lock(m_CSOutgoingData);
-		m_Link.reset();
-	}
-
-	if (!SetState(csDestroying))
+	if (!SetState(csDestroyed))
 	{
 		// Already called
 		LOGD("%s: client %p, \"%s\" already destroyed, bailing out", __FUNCTION__, static_cast<void *>(this), m_Username.c_str());
@@ -158,41 +132,12 @@ void cClientHandle::Destroy(void)
 	}

 	LOGD("%s: destroying client %p, \"%s\" @ %s", __FUNCTION__, static_cast<void *>(this), m_Username.c_str(), m_IPString.c_str());
-	auto player = m_Player;
-	auto Self = std::move(m_Self);  // Keep ourself alive for at least as long as this function
-	SetState(csDestroyed);

-	if (player == nullptr)
 	{
-		return;
+		cCSLock Lock(m_CSOutgoingData);
+		m_Link->Shutdown();  // Cleanly close the connection
+		m_Link.reset();  // Release the strong reference cTCPLink holds to ourself
 	}
-
-	// Atomically decrement player count (in world or server thread)
-	cRoot::Get()->GetServer()->PlayerDestroyed();
-
-	auto world = player->GetWorld();
-	if (world != nullptr)
-	{
-		player->StopEveryoneFromTargetingMe();
-		player->SetIsTicking(false);
-
-		if (!m_PlayerPtr)
-		{
-			// If our own smart pointer is unset, player has been transferred to world
-			ASSERT(world->IsPlayerReferencedInWorldOrChunk(*player));
-
-			m_PlayerPtr = world->RemovePlayer(*player);
-
-			// And RemovePlayer should have returned a valid smart pointer
-			ASSERT(m_PlayerPtr);
-		}
-		else
-		{
-			// If ownership was not transferred, our own smart pointer should be valid and RemovePlayer's should not
-			ASSERT(!world->IsPlayerReferencedInWorldOrChunk(*player));
-		}
-	}
-	player->RemoveClientHandle();
 }


@@ -301,9 +246,6 @@ void cClientHandle::Kick(const AString & a_Reason)

 void cClientHandle::Authenticate(const AString & a_Name, const cUUID & a_UUID, const Json::Value & a_Properties)
 {
-	// Atomically increment player count (in server thread)
-	cRoot::Get()->GetServer()->PlayerCreated();
-
 	{
 		cCSLock lock(m_CSState);
 		/*
@@ -351,95 +293,67 @@ void cClientHandle::Authenticate(const AString & a_Name, const cUUID & a_UUID, c

 void cClientHandle::FinishAuthenticate(const AString & a_Name, const cUUID & a_UUID, const Json::Value & a_Properties)
 {
-	cWorld * World;
+	// Serverside spawned player (so data are loaded).
+	auto Player = std::make_unique<cPlayer>(shared_from_this());
+	m_Player = Player.get();
+
+	/*
+	LOGD("Created a new cPlayer object at %p for client %s @ %s (%p)",
+		static_cast<void *>(m_Player),
+		m_Username.c_str(), m_IPString.c_str(), static_cast<void *>(this)
+	);
+	//*/
+
+	cWorld * World = cRoot::Get()->GetWorld(m_Player->GetLoadedWorldName());
+	if (World == nullptr)
 	{
-		// Spawn player (only serversided, so data is loaded)
-		m_PlayerPtr = std::make_unique<cPlayer>(m_Self, GetUsername());
-		m_Player = m_PlayerPtr.get();
-		/*
-		LOGD("Created a new cPlayer object at %p for client %s @ %s (%p)",
-			static_cast<void *>(m_Player),
-			m_Username.c_str(), m_IPString.c_str(), static_cast<void *>(this)
-		);
-		//*/
-		InvalidateCachedSentChunk();
-		m_Self.reset();
-
-
-		// New player use default world
-		// Player who can load from disk, use loaded world
-		if (m_Player->GetWorld() == nullptr)
-		{
-			World = cRoot::Get()->GetWorld(m_Player->GetLoadedWorldName());
-			if (World == nullptr)
-			{
-				World = cRoot::Get()->GetDefaultWorld();
-				m_Player->SetPosition(World->GetSpawnX(), World->GetSpawnY(), World->GetSpawnZ());
-			}
-			m_Player->SetWorld(World);
-		}
-		else
-		{
-			World = m_Player->GetWorld();
-		}
-
-		m_Player->SetIP (m_IPString);
-
-		if (!cRoot::Get()->GetPluginManager()->CallHookPlayerJoined(*m_Player))
-		{
-			cRoot::Get()->BroadcastChatJoin(Printf("%s has joined the game", GetUsername().c_str()));
-			LOGINFO("Player %s has joined the game", m_Username.c_str());
-		}
-
-		m_ConfirmPosition = m_Player->GetPosition();
-
-		// Return a server login packet
-		m_Protocol->SendLogin(*m_Player, *World);
-		m_LastSentDimension = World->GetDimension();
-
-		// Send Weather if raining:
-		if ((World->GetWeather() == 1) || (World->GetWeather() == 2))
-		{
-			m_Protocol->SendWeather(World->GetWeather());
-		}
-
-		// Send time:
-		m_Protocol->SendTimeUpdate(World->GetWorldAge(), World->GetTimeOfDay(), World->IsDaylightCycleEnabled());
-
-		// Send contents of the inventory window
-		m_Protocol->SendWholeInventory(*m_Player->GetWindow());
-
-		// Send health
-		m_Player->SendHealth();
-
-		// Send experience
-		m_Player->SendExperience();
-
-		// Send hotbar active slot
-		m_Player->SendHotbarActiveSlot();
-
-		// Send player list items
-		SendPlayerListAddPlayer(*m_Player);
-		cRoot::Get()->BroadcastPlayerListsAddPlayer(*m_Player);
-		cRoot::Get()->SendPlayerLists(m_Player);
-
-		SetState(csAuthenticated);
+		World = cRoot::Get()->GetDefaultWorld();
 	}

-	// Query player team
-	m_Player->UpdateTeam();
+	// Atomically increment player count (in server thread):
+	cRoot::Get()->GetServer()->PlayerCreated();

-	// Send scoreboard data
-	World->GetScoreBoard().SendTo(*this);
+	if (!cRoot::Get()->GetPluginManager()->CallHookPlayerJoined(*m_Player))
+	{
+		cRoot::Get()->BroadcastChatJoin(Printf("%s has joined the game", a_Name.c_str()));
+		LOGINFO("Player %s has joined the game", a_Name.c_str());
+	}

-	// Send statistics
+	// TODO: this accesses the world spawn from the authenticator thread
+	// World spawn should be sent in OnAddedToWorld.
+	// Return a server login packet:
+	m_Protocol->SendLogin(*m_Player, *World);
+
+	if (m_Player->GetKnownRecipes().empty())
+	{
+		SendInitRecipes(0);
+	}
+	else
+	{
+		for (const auto KnownRecipe : m_Player->GetKnownRecipes())
+		{
+			SendInitRecipes(KnownRecipe);
+		}
+	}
+
+	// Send player list items:
+	SendPlayerListAddPlayer(*m_Player);  // Add ourself
+	cRoot::Get()->BroadcastPlayerListsAddPlayer(*m_Player);  // Add ourself to everyone else
+	cRoot::Get()->SendPlayerLists(m_Player);  // Add everyone else to ourself
+
+	// Send statistics:
 	SendStatistics(m_Player->GetStatManager());

 	// Delay the first ping until the client "settles down"
 	// This should fix #889, "BadCast exception, cannot convert bit to fm" error in client
 	m_PingStartTime = std::chrono::steady_clock::now() + std::chrono::seconds(3);  // Send the first KeepAlive packet in 3 seconds

-	cRoot::Get()->GetPluginManager()->CallHookPlayerSpawned(*m_Player);
+	// Remove the client handle from the server, it will be ticked from its cPlayer object from now on:
+	cRoot::Get()->GetServer()->ClientMovedToWorld(this);
+
+	SetState(csDownloadingWorld);
+	m_Player->Initialize(std::move(Player), *World);
+
 	// LOGD("Client %s @ %s (%p) has been fully authenticated", m_Username.c_str(), m_IPString.c_str(), static_cast<void *>(this));
 }

@@ -449,10 +363,6 @@ void cClientHandle::FinishAuthenticate(const AString & a_Name, const cUUID & a_U

 bool cClientHandle::StreamNextChunk(void)
 {
-	if ((m_State < csAuthenticated) || (m_State >= csDestroying))
-	{
-		return true;
-	}
 	ASSERT(m_Player != nullptr);

 	int ChunkPosX = m_Player->GetChunkX();
@@ -609,12 +519,6 @@ void cClientHandle::UnloadOutOfRangeChunks(void)

 void cClientHandle::StreamChunk(int a_ChunkX, int a_ChunkZ, cChunkSender::Priority a_Priority)
 {
-	if (m_State >= csDestroying)
-	{
-		// Don't stream chunks to clients that are being destroyed
-		return;
-	}
-
 	cWorld * World = m_Player->GetWorld();
 	ASSERT(World != nullptr);

@@ -820,12 +724,6 @@ void cClientHandle::HandlePlayerAbilities(bool a_IsFlying, float FlyingSpeed, fl

 void cClientHandle::HandlePlayerPos(double a_PosX, double a_PosY, double a_PosZ, double a_Stance, bool a_IsOnGround)
 {
-	if ((m_Player == nullptr) || (m_State != csPlaying))
-	{
-		// The client hasn't been spawned yet and sends nonsense, we know better
-		return;
-	}
-
 	if (m_Player->IsFrozen())
 	{
 		// Ignore client-side updates if the player is frozen
@@ -1580,11 +1478,6 @@ void cClientHandle::HandleChat(const AString & a_Message)

 void cClientHandle::HandlePlayerLook(float a_Rotation, float a_Pitch, bool a_IsOnGround)
 {
-	if ((m_Player == nullptr) || (m_State != csPlaying))
-	{
-		return;
-	}
-
 	m_Player->SetYaw        (a_Rotation);
 	m_Player->SetHeadYaw    (a_Rotation);
 	m_Player->SetPitch      (a_Pitch);
@@ -1838,11 +1731,12 @@ void cClientHandle::HandleUseItem(eHand a_Hand)

 void cClientHandle::HandleRespawn(void)
 {
-	if (m_Player == nullptr)
+	if (m_Player->GetHealth() > 0)
 	{
-		Destroy();
+		Kick("What is not dead may not live again. Hacked client?");
 		return;
 	}
+
 	m_Player->Respawn();
 	cRoot::Get()->GetPluginManager()->CallHookPlayerSpawned(*m_Player);
 }
@@ -1961,10 +1855,6 @@ void cClientHandle::HandleEntitySprinting(UInt32 a_EntityID, bool a_IsSprinting)

 void cClientHandle::HandleUnmount(void)
 {
-	if (m_Player == nullptr)
-	{
-		return;
-	}
 	m_Player->Detach();
 }

@@ -2009,8 +1899,14 @@ void cClientHandle::SendData(const ContiguousByteBufferView a_Data)
 		return;
 	}

-	cCSLock Lock(m_CSOutgoingData);
-	m_OutgoingData += a_Data;
+	// Due to cTCPLink's design of holding a strong pointer to ourself, we need to explicitly reset m_Link.
+	// This means we need to check it's not nullptr before trying to send, but also capture the link,
+	// to prevent it being reset between the null check and the Send:
+	if (auto Link = m_Link; Link != nullptr)
+	{
+		cCSLock Lock(m_CSOutgoingData);
+		Link->Send(a_Data.data(), a_Data.size());
+	}
 }


@@ -2087,28 +1983,15 @@ void cClientHandle::Tick(float a_Dt)

 	try
 	{
-		ProcessProtocolInOut();
+		ProcessProtocolIn();
 	}
 	catch (const std::exception & Oops)
 	{
 		Kick(Oops.what());
-		return;  // Return early to give a chance to send the kick packet before link shutdown
 	}

-	// If player has been kicked, terminate the connection:
-	if (m_State == csKicked)
+	if (IsDestroyed())
 	{
-		m_Link->Shutdown();
-	}
-
-	// If destruction is queued, destroy now:
-	if (m_State == csQueuedForDestruction)
-	{
-		LOGD("Client %s @ %s (%p) has been queued for destruction, destroying now.",
-			m_Username.c_str(), m_IPString.c_str(), static_cast<void *>(this)
-		);
-		GetPlayer()->GetStatManager().AddValue(Statistic::LeaveGame);
-		Destroy();
 		return;
 	}

@@ -2119,12 +2002,6 @@ void cClientHandle::Tick(float a_Dt)
 		return;
 	}

-	// Only process further if the player object is valid:
-	if (m_Player == nullptr)
-	{
-		return;
-	}
-
 	// Freeze the player if they are standing in a chunk not yet sent to the client
 	m_HasSentPlayerChunk = false;
 	if (m_Player->GetParentChunk() != nullptr)
@@ -2160,6 +2037,12 @@ void cClientHandle::Tick(float a_Dt)
 		{
 			m_Protocol->SendPlayerMoveLook();
 			m_State = csPlaying;
+
+			// Send resource pack (after a MoveLook, because sending it before the initial MoveLook cancels the download screen):
+			if (const auto & ResourcePackUrl = cRoot::Get()->GetServer()->GetResourcePackUrl(); !ResourcePackUrl.empty())
+			{
+				SendResourcePack(ResourcePackUrl);
+			}
 		}
 	}  // lock(m_CSState)

@@ -2174,24 +2057,21 @@ void cClientHandle::Tick(float a_Dt)
 		}
 	}

-	if ((m_State >= csAuthenticated) && (m_State < csQueuedForDestruction))
+	// Stream 4 chunks per tick
+	for (int i = 0; i < 4; i++)
 	{
-		// Stream 4 chunks per tick
-		for (int i = 0; i < 4; i++)
+		// Stream the next chunk
+		if (StreamNextChunk())
 		{
-			// Stream the next chunk
-			if (StreamNextChunk())
-			{
-				// Streaming finished. All chunks are loaded.
-				break;
-			}
+			// Streaming finished. All chunks are loaded.
+			break;
 		}
+	}

-		// Unload all chunks that are out of the view distance (every 5 seconds)
-		if ((m_Player->GetWorld()->GetWorldAge() % 100) == 0)
-		{
-			UnloadOutOfRangeChunks();
-		}
+	// Unload all chunks that are out of the view distance (every 5 seconds)
+	if ((m_Player->GetWorld()->GetWorldAge() % 100) == 0)
+	{
+		UnloadOutOfRangeChunks();
 	}

 	// Handle block break animation:
@@ -2220,33 +2100,7 @@ void cClientHandle::Tick(float a_Dt)

 void cClientHandle::ServerTick(float a_Dt)
 {
-	ProcessProtocolInOut();
-
-	// If destruction is queued, destroy now:
-	if (m_State == csQueuedForDestruction)
-	{
-		LOGD("Client %s @ %s (%p) has been queued for destruction, destroying now.",
-			m_Username.c_str(), m_IPString.c_str(), static_cast<void *>(this)
-		);
-		Destroy();
-		return;
-	}
-
-	{
-		cCSLock lock(m_CSState);
-		if (m_State == csAuthenticated)
-		{
-			StreamNextChunk();
-
-			// Remove the client handle from the server, it will be ticked from its cPlayer object from now on
-			cRoot::Get()->GetServer()->ClientMovedToWorld(this);
-
-			// Add the player to the world (start ticking from there):
-			m_State = csDownloadingWorld;
-			m_Player->Initialize(std::move(m_PlayerPtr), *(m_Player->GetWorld()));
-			return;
-		}
-	}  // lock(m_CSState)
+	ProcessProtocolIn();

 	m_TicksSinceLastPacket += 1;
 	if (m_TicksSinceLastPacket > 600)  // 30 seconds
@@ -2538,10 +2392,7 @@ void cClientHandle::SendDisconnect(const AString & a_Reason)
 		LOGD("Sending a DC: \"%s\"", StripColorCodes(a_Reason).c_str());
 		m_Protocol.SendDisconnect(*this, a_Reason);
 		m_HasSentDC = true;
-		// csKicked means m_Link will be shut down on the next tick. The
-		// disconnect packet data is sent in the tick thread so the connection
-		// is closed there after the data is sent.
-		SetState(csKicked);
+		Destroy();
 	}
 }

@@ -2906,19 +2757,18 @@ void cClientHandle::SendResetTitle()

 void cClientHandle::SendRespawn(eDimension a_Dimension, bool a_ShouldIgnoreDimensionChecks)
 {
-	if ((!a_ShouldIgnoreDimensionChecks) && (a_Dimension == m_LastSentDimension))
+	if (!a_ShouldIgnoreDimensionChecks && (a_Dimension == m_Player->GetWorld()->GetDimension()))
 	{
 		// The client goes crazy if we send a respawn packet with the dimension of the current world
 		// So we send a temporary one first.
 		// This is not needed when the player dies, hence the a_ShouldIgnoreDimensionChecks flag.
-		// a_ShouldIgnoreDimensionChecks is true only at cPlayer::respawn, which is called after
+		// a_ShouldIgnoreDimensionChecks is true only at cPlayer::Respawn, which is called after
 		// the player dies.
 		eDimension TemporaryDimension = (a_Dimension == dimOverworld) ? dimNether : dimOverworld;
 		m_Protocol->SendRespawn(TemporaryDimension);
 	}
+
 	m_Protocol->SendRespawn(a_Dimension);
-	m_Protocol->SendExperience();
-	m_LastSentDimension = a_Dimension;
 }


@@ -3285,7 +3135,7 @@ bool cClientHandle::HasPluginChannel(const AString & a_PluginChannel)

 bool cClientHandle::WantsSendChunk(int a_ChunkX, int a_ChunkZ)
 {
-	if (m_State >= csQueuedForDestruction)
+	if (m_State >= csDestroyed)
 	{
 		return false;
 	}
@@ -3300,7 +3150,7 @@ bool cClientHandle::WantsSendChunk(int a_ChunkX, int a_ChunkZ)

 void cClientHandle::AddWantedChunk(int a_ChunkX, int a_ChunkZ)
 {
-	if (m_State >= csQueuedForDestruction)
+	if (m_State >= csDestroyed)
 	{
 		return;
 	}
@@ -3368,17 +3218,7 @@ void cClientHandle::SocketClosed(void)
 	}

 	// Queue self for destruction:
-	SetState(csQueuedForDestruction);
-}
-
-
-
-
-
-void cClientHandle::SetSelf(cClientHandlePtr a_Self)
-{
-	ASSERT(m_Self == nullptr);
-	m_Self = std::move(a_Self);
+	Destroy();
 }


@@ -3400,7 +3240,7 @@ bool cClientHandle::SetState(eState a_NewState)



-void cClientHandle::ProcessProtocolInOut(void)
+void cClientHandle::ProcessProtocolIn(void)
 {
 	// Process received network data:
 	AString IncomingData;
@@ -3413,21 +3253,6 @@ void cClientHandle::ProcessProtocolInOut(void)
 	{
 		m_Protocol.HandleIncomingData(*this, IncomingData);
 	}
-
-	// Send any queued outgoing data:
-	ContiguousByteBuffer OutgoingData;
-	{
-		cCSLock Lock(m_CSOutgoingData);
-		std::swap(OutgoingData, m_OutgoingData);
-	}
-
-	// Capture the link to prevent it being reset between the null check and the Send:
-	auto Link = m_Link;
-
-	if ((Link != nullptr) && !OutgoingData.empty())
-	{
-		Link->Send(OutgoingData.data(), OutgoingData.size());
-	}
 }


@@ -3464,10 +3289,6 @@ void cClientHandle::OnRemoteClosed(void)
 		m_Username.c_str(), m_IPString.c_str()
 	);
 	//*/
-	{
-		cCSLock Lock(m_CSOutgoingData);
-		m_Link.reset();
-	}
 	SocketClosed();
 }

@@ -3480,9 +3301,5 @@ void cClientHandle::OnError(int a_ErrorCode, const AString & a_ErrorMsg)
 	LOGD("An error has occurred on client link for %s @ %s: %d (%s). Client disconnected.",
 		m_Username.c_str(), m_IPString.c_str(), a_ErrorCode, a_ErrorMsg.c_str()
 	);
-	{
-		cCSLock Lock(m_CSOutgoingData);
-		m_Link.reset();
-	}
 	SocketClosed();
 }