Player: Disconnects no longer nuke player internals, (mostly) fixes #1239

there are some problems that haven't been accounted for yet, but this fixes all the direct crashes.

src/pocketmine/Player.php

@@ -339,8 +339,8 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 
 		$ev = new PlayerLoginEvent($this, "Plugin reason");
 		$ev->call();
-		if($ev->isCancelled()){
+		if($ev->isCancelled() or !$this->isConnected()){
-			$this->close($this->getLeaveMessage(), $ev->getKickMessage());
+			$this->disconnect($ev->getKickMessage());
 
 			return;
 		}
@@ -758,7 +758,7 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	 * @return bool
 	 */
 	public function isConnected() : bool{
-		return $this->networkSession !== null;
+		return $this->networkSession !== null and $this->networkSession->isConnected();
 	}
 
 	/**
@@ -2479,7 +2479,7 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 					$message = "disconnectionScreen.noReason";
 				}
 			}
-			$this->close($ev->getQuitMessage(), $message);
+			$this->disconnect($message, $ev->getQuitMessage());
 
 			return true;
 		}
@@ -2488,81 +2488,92 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	}
 
 	/**
-	 * Note for plugin developers: use kick() with the isAdmin
+	 * Removes the player from the server. This cannot be cancelled.
-	 * flag set to kick without the "Kicked by admin" part instead of this method.
+	 * This is used for remote disconnects and for uninterruptible disconnects (for example, when the server shuts down).
 	 *
-	 * @param TextContainer|string $message Message to be broadcasted
+	 * Note for plugin developers: Prefer kick() with the isAdmin flag set to kick without the "Kicked by admin" part
-	 * @param string               $reason Reason showed in console
+	 * instead of this method. This way other plugins can have a say in whether the player is removed or not.
+	 *
+	 * @param string               $reason Shown to the player, usually this will appear on their disconnect screen.
+	 * @param TextContainer|string $quitMessage Message to broadcast to online players (null will use default)
 	 * @param bool                 $notify
 	 */
-	final public function close($message = "", string $reason = "generic reason", bool $notify = true) : void{
+	public function disconnect(string $reason, $quitMessage = null, bool $notify = true) : void{
-		if($this->isConnected() and !$this->closed){
+		if(!$this->isConnected()){
-			$ip = $this->networkSession->getIp();
+			return;
-			$port = $this->networkSession->getPort();
+		}
-			$this->networkSession->onPlayerDestroyed($reason, $notify);
+
+		$this->networkSession->onPlayerDestroyed($reason, $notify);
+
+		//prevent the player receiving their own disconnect message
+		PermissionManager::getInstance()->unsubscribeFromPermission(Server::BROADCAST_CHANNEL_USERS, $this);
+		PermissionManager::getInstance()->unsubscribeFromPermission(Server::BROADCAST_CHANNEL_ADMINISTRATIVE, $this);
+
+		$ev = new PlayerQuitEvent($this, $quitMessage ?? $this->getLeaveMessage(), $reason);
+		$ev->call();
+		if(!empty($ev->getQuitMessage())){
+			$this->server->broadcastMessage($ev->getQuitMessage());
+		}
+		$this->save();
+
+		$this->server->getLogger()->info($this->getServer()->getLanguage()->translateString("pocketmine.player.logOut", [
+			TextFormat::AQUA . $this->getName() . TextFormat::WHITE,
+			$this->networkSession->getIp(),
+			$this->networkSession->getPort(),
+			$this->getServer()->getLanguage()->translateString($reason)
+		]));
+
+		$this->spawned = false;
+
+		$this->stopSleep();
+		$this->despawnFromAll();
+
+		$this->server->removeOnlinePlayer($this);
+
+		foreach($this->server->getOnlinePlayers() as $player){
+			if(!$player->canSee($this)){
+				$player->showPlayer($this);
+			}
+		}
+		$this->hiddenPlayers = [];
+
+		if($this->isValid()){
+			foreach($this->usedChunks as $index => $d){
+				Level::getXZ($index, $chunkX, $chunkZ);
+				$this->level->unregisterChunkLoader($this, $chunkX, $chunkZ);
+				$this->level->unregisterChunkListener($this, $chunkX, $chunkZ);
+				foreach($this->level->getChunkEntities($chunkX, $chunkZ) as $entity){
+					$entity->despawnFrom($this);
+				}
+				unset($this->usedChunks[$index]);
+			}
+		}
+		$this->usedChunks = [];
+		$this->loadQueue = [];
+
+		$this->removeAllWindows(true);
+		$this->windows = [];
+		$this->windowIndex = [];
+
+		$this->perm->clearPermissions();
+
+		$this->flagForDespawn();
+	}
+
+	final public function close() : void{
+		if(!$this->closed){
+			$this->disconnect("Player destroyed");
 			$this->networkSession = null;
 
-			PermissionManager::getInstance()->unsubscribeFromPermission(Server::BROADCAST_CHANNEL_USERS, $this);
-			PermissionManager::getInstance()->unsubscribeFromPermission(Server::BROADCAST_CHANNEL_ADMINISTRATIVE, $this);
-
-			$this->stopSleep();
-
-			if($this->spawned){
-				$ev = new PlayerQuitEvent($this, $message, $reason);
-				$ev->call();
-				if($ev->getQuitMessage() != ""){
-					$this->server->broadcastMessage($ev->getQuitMessage());
-				}
-
-				$this->save();
-			}
-
-			if($this->isValid()){
-				foreach($this->usedChunks as $index => $d){
-					Level::getXZ($index, $chunkX, $chunkZ);
-					$this->level->unregisterChunkLoader($this, $chunkX, $chunkZ);
-					$this->level->unregisterChunkListener($this, $chunkX, $chunkZ);
-					foreach($this->level->getChunkEntities($chunkX, $chunkZ) as $entity){
-						$entity->despawnFrom($this);
-					}
-					unset($this->usedChunks[$index]);
-				}
-			}
-			$this->usedChunks = [];
-			$this->loadQueue = [];
-
-			foreach($this->server->getOnlinePlayers() as $player){
-				if(!$player->canSee($this)){
-					$player->showPlayer($this);
-				}
-			}
-			$this->hiddenPlayers = [];
-
-			$this->removeAllWindows(true);
-			$this->windows = [];
-			$this->windowIndex = [];
 			$this->cursorInventory = null;
 			$this->craftingGrid = null;
 
-			parent::close();
-
 			$this->spawned = false;
 
-			$this->server->removeOnlinePlayer($this);
-
-			$this->server->getLogger()->info($this->getServer()->getLanguage()->translateString("pocketmine.player.logOut", [
-				TextFormat::AQUA . $this->getName() . TextFormat::WHITE,
-				$ip,
-				$port,
-				$this->getServer()->getLanguage()->translateString($reason)
-			]));
-
 			$this->spawnPosition = null;
+			$this->perm = null;
 
-			if($this->perm !== null){
+			parent::close();
-				$this->perm->clearPermissions();
-				$this->perm = null;
-			}
 		}
 	}
 

src/pocketmine/level/LevelManager.php

@@ -167,7 +167,7 @@ class LevelManager{
 		$this->server->getLogger()->info($this->server->getLanguage()->translateString("pocketmine.level.unloading", [$level->getDisplayName()]));
 		foreach($level->getPlayers() as $player){
 			if($level === $this->levelDefault or $this->levelDefault === null){
-				$player->close($player->getLeaveMessage(), "Forced default world unload");
+				$player->disconnect("Forced default world unload");
 			}elseif($this->levelDefault instanceof Level){
 				$player->teleport($this->levelDefault->getSafeSpawn());
 			}

src/pocketmine/network/mcpe/NetworkSession.php

@@ -106,6 +106,8 @@ class NetworkSession{
 	/** @var bool */
 	private $connected = true;
 	/** @var bool */
+	private $disconnectGuard = false;
+	/** @var bool */
 	private $loggedIn = false;
 	/** @var bool */
 	private $authenticated = false;
@@ -395,13 +397,14 @@ class NetworkSession{
 		$this->interface->putPacket($this, $payload, $immediate);
 	}
 
-	private function checkDisconnect() : bool{
+	private function tryDisconnect(\Closure $func) : void{
-		if($this->connected){
+		if($this->connected and !$this->disconnectGuard){
+			$this->disconnectGuard = true;
+			$func();
+			$this->disconnectGuard = false;
 			$this->connected = false;
 			$this->manager->remove($this);
-			return true;
 		}
-		return false;
 	}
 
 	/**
@@ -411,12 +414,12 @@ class NetworkSession{
 	 * @param bool   $notify
 	 */
 	public function disconnect(string $reason, bool $notify = true) : void{
-		if($this->checkDisconnect()){
+		$this->tryDisconnect(function() use($reason, $notify){
 			if($this->player !== null){
-				$this->player->close($this->player->getLeaveMessage(), $reason);
+				$this->player->disconnect($reason, null, $notify);
 			}
 			$this->doServerDisconnect($reason, $notify);
-		}
+		});
 	}
 
 	/**
@@ -429,11 +432,17 @@ class NetworkSession{
 	 * @throws \UnsupportedOperationException
 	 */
 	public function transfer(string $ip, int $port, string $reason = "transfer") : void{
-		$pk = new TransferPacket();
+		$this->tryDisconnect(function() use($ip, $port, $reason){
-		$pk->address = $ip;
+			$pk = new TransferPacket();
-		$pk->port = $port;
+			$pk->address = $ip;
-		$this->sendDataPacket($pk, true);
+			$pk->port = $port;
-		$this->disconnect($reason);
+			$this->sendDataPacket($pk, true);
+			$this->disconnect($reason, false);
+			if($this->player !== null){
+				$this->player->disconnect($reason, null, false);
+			}
+			$this->doServerDisconnect($reason, false);
+		});
 	}
 
 	/**
@@ -443,9 +452,9 @@ class NetworkSession{
 	 * @param bool   $notify
 	 */
 	public function onPlayerDestroyed(string $reason, bool $notify = true) : void{
-		if($this->checkDisconnect()){
+		$this->tryDisconnect(function() use($reason, $notify){
 			$this->doServerDisconnect($reason, $notify);
-		}
+		});
 	}
 
 	/**
@@ -472,9 +481,11 @@ class NetworkSession{
 	 * @param string $reason
 	 */
 	public function onClientDisconnect(string $reason) : void{
-		if($this->checkDisconnect() and $this->player !== null){
+		$this->tryDisconnect(function() use($reason){
-			$this->player->close($this->player->getLeaveMessage(), $reason);
+			if($this->player !== null){
-		}
+				$this->player->disconnect($reason, null, false);
+			}
+		});
 	}
 
 	public function setAuthenticationStatus(bool $authenticated, bool $authRequired, ?string $error) : bool{