Pocketmine-mp: Crash when closing players during some events

Created on 25 Jul 2017 · 32Comments · Source: pmmp/PocketMine-MP

Issue description

When I try to reconnect a player to a server, this happens:

I let players reconnect to the server if they are stuck or something.

Steps to reproduce the issue

Transfer a player to the same server they're already on using code below.

OS and versions

PocketMine-MP: ALPHA7 (is that enough info?)
PHP: 7.0
Server OS: ubuntu linux
Game version: irrelevant

Plugins

No it isn't reproducable without plugins. but this isnt a plugin error..
public function transfer(Player $player, string $identifier, $msg = ""){
$pk = new TransferPacket();
$pk->address = $this->getAddress(); //server ip
$pk->port = $this->getPortByIdentifier($identifier); //server port
$player->directDataPacket($pk);
if($msg != "") $this->addTransferMessage($player, $msg);
$player->close();
}

UPDATE: It doesn't do this anymore with Player->kick(), but I still think this is unexpected behavior..? Shouldn't crash.

Core Fixed

Source

mal0ne-23

👎1

Most helpful comment

Work is underway on the network-nuke branch to resolve this issue. The high-level overview is basically this:

close() has been abused by a lot of things for a long time, including for disconnecting players. A new disconnect() method is being introduced, which will do the light things like sending packets, and then schedule the player entity for close()ing at the end of the tick.
close() will no longer be an appropriate way to get rid of a player - instead things should use disconnect() for graceful cleanup and disconnection.

dktapps on 7 Jun 2018

👍4

All 32 comments

You should not be using $player->close() it is an internal function.

robske110 on 25 Jul 2017

👎1

..shouldn't it be private then? or somehow prevented to be used by plugins?

mal0ne-23 on 25 Jul 2017

👎1

@mal0ne-23 Private cannot prevent use by plugins. The correct way is to mark the method as @internal.

Actually, close() is not forbidden for plugins. It's just something different from kick(), but still meaningful if used correctly.

SOF3 on 25 Jul 2017

👎1

backtrace?

dktapps on 25 Jul 2017

👎1

I create portal with transfer (Check location player in PlayerMoveEvent)

[Server thread/INFO]: Ganselot[/192.168.1.81:1037] logged out due to transfer
[Server thread/CRITICAL]: Could not pass event 'pocketmine\event\player\PlayerMoveEvent' to 'RegionGuard v1.0.0': Call to a member function getName() on null on RegionGuard\EventListener
[Server thread/CRITICAL]: Error: "Call to a member function getName() on null" (EXCEPTION) in "plugins/Z_RegionGuard/src/RegionGuard/EventListener" at line 123
[Server thread/CRITICAL]: Could not pass event 'pocketmine\event\player\PlayerMoveEvent' to 'SkyWars v0.7dev': Call to a member function getName() on null on svile\sw\SWlistener
[Server thread/CRITICAL]: Error: "Call to a member function getName() on null" (EXCEPTION) in "plugins/Z_SkyWars/src/svile/sw/SWlistener" at line 257
[Server thread/CRITICAL]: Could not pass event 'pocketmine\event\player\PlayerMoveEvent' to 'ToolsPro v1.0.0': Trying to get permissions of closed player on ToolsPro\EventListener
[Server thread/CRITICAL]: InvalidStateException: "Trying to get permissions of closed player" (EXCEPTION) in "src/pocketmine/Player" at line 559
[Server thread/CRITICAL]: Could not tick level "lobby": Call to a member function addEntityMovement() on null
[Server thread/CRITICAL]: Error: "Call to a member function addEntityMovement() on null" (EXCEPTION) in "src/pocketmine/Player" at line 1567

zKoz210 on 25 Jul 2017

👎1

This is not a support forum for defective plugins.

dktapps on 25 Jul 2017

👎1

Funny, but such errors do not have even a poor fork. It only happens with PocketMine-MP

Example:

    public function onPlayerMoveEvent(PlayerMoveEvent $event)
    {
        $player = $event->getPlayer();
        $x = $player->x;
        $y = $player->y;
        $z = $player->z;
        foreach ($this->portals->getAll() as $name => $area) {
            if ($area['world'] == $player->getLevel()->getName()) {
                if (($area['pos1'][0] <= $x and $x <= $area['pos2'][0]) and ($area['pos1'][1] <= $y and $y <= $area['pos2'][1]) and ($area['pos1'][2] <= $z and $z <= $area['pos2'][2])) {
                    $addr = $area['ip'];
                    $addr = explode(":", $addr);
                    $ip = $addr[0];
                    $port = $addr[1];
                    $player->transfer($ip, $port);
                }
            }
        }
    }

zKoz210 on 25 Jul 2017

This is not a support forum for defective plugins. You fail to realize, like every other irritating person on this issue tracker who doesn't think, that PocketMine-MP's API is undergoing substantial changes. Therefore your experience on spoons is irrelevant.

dktapps on 25 Jul 2017

👍2

@zKoz210 Funny but without anyone supporting this repository, all spoons would just collapse _(oh wait, haven't most of them already collapsed and have a bunch of people fighting for the spoon ownership?)_.
The transfer bug you are referring is a client bug. If you try transferring a moving player, they will get "Disconnected" like as if they turned off their WiFi.

Muqsit on 25 Jul 2017

😄1

@Muqsit it can hardly be considered a client bug when the server is still processing it and crashing.

dktapps on 25 Jul 2017

Are you using just close() that makes it impossible to use the transfer in events.

Reproduce:
1) one plugin with PlayerInteractEvent (RegionGuard)
2) plugin with this event

    public function onPlayerInteractEvent(PlayerInteractEvent $event) {
        $event->getPlayer()->transfer('play.simplexland.ru', 19132);
    }

[12:48:37] [Server thread/CRITICAL]: Could not pass event 'pocketmine\event\player\PlayerInteractEvent' to 'RegionGuard v1.0.0': Call to a member function getItemInHand() on null on RegionGuard\EventListener
[12:48:37] [Server thread/CRITICAL]: Error: "Call to a member function getItemInHand() on null" (EXCEPTION) in "plugins/Z_RegionGuard/src/RegionGuard/EventListener" at line 33
[12:48:37] [Server thread/CRITICAL]: Error: "Call to a member function sendHeldItem() on null" (EXCEPTION) in "src/pocketmine/Player" at line 2387
[12:48:37] [Server thread/CRITICAL]: [Network] Stopped interface pocketmine\network\mcpe\RakLibInterface due to Undefined index: 192.168.1.81:32286

zKoz210 on 25 Jul 2017

@dktapps I'm some commits behind and it has happened to me ever since MCPE brought the transfer packet back. This bug isn't exactly related to the OP's issue, just a note for anyone who it's trying to transfer a moving player. I have a sign-tapping system to transfer players, and this bug happens even if the player slightly moves along the yaw-pitch axis. They have to stand stationery while tapping the sign to get successfully transferred.

Muqsit on 25 Jul 2017

The problem is that no additional checks are performed for the player being dead or closed if a plugin does something during PlayerMoveEvent. This problem extends to a wide range of other events:

PlayerLoginEvent
PlayerInteractEvent (for example clicking a sign)
many other events that involve players & entities

I've been trying to come up with a way to solve this issue for a while. My best solution is to throw an exception at the end of close() to interrupt any execution that might be taking place. This exception would then need to be caught by the packet handler or the onUpdate() function. However, this would be a radical API break, and lots of plugins would no doubt start crashing for no apparent reasons.

dktapps on 25 Jul 2017

👍4

This issue is also somewhat related to #1226 . The only clean way I can think of to solve this issue is to create an exception throw. It also doesn't help that Player and network are currently hopelessly tangled together.

Example of what I'm talking about

public function onUpdate($currentTick){
    if($this->closed){
        return false;
    }

    $this->someFunctionThatMightCloseMeWithoutNotifyingMe();
    $this->updateMovement(); //crash
    return true;
}

With exception throw, Entity->close() could throw an EntityClosedException:

public function onUpdate($currentTick){
    if($this->closed){
        return false;
    }

    try{
        $this->someFunctionThatMightCloseMeWithoutNotifyingMe();
        $this->updateMovement(); //never reached if the above call causes the entity to be closed
    }catch(EntityClosedException $e){
        return false;
        //ok, don't try and do anything else with garbage entity
    }
    return true;
}

dktapps on 25 Jul 2017

Temporary fix. If you don't care message (logged out due to transfer), you can simply remove $this->close("", $ev->getMessage(), false); or create your own function without it

    /**
     * Transfers a player to another server.
     *
     * @param string $address The IP address or hostname of the destination server
     * @param int    $port    The destination port, defaults to 19132
     * @param string $message Message to show in the console when closing the player
     *
     * @return bool if transfer was successful.
     */
    public function transfer(string $address, int $port = 19132, string $message = "transfer") : bool{
        $this->server->getPluginManager()->callEvent($ev = new PlayerTransferEvent($this, $address, $port, $message));

        if(!$ev->isCancelled()){
            $pk = new TransferPacket();
            $pk->address = $ev->getAddress();
            $pk->port = $ev->getPort();
            $this->directDataPacket($pk);
            $this->close("", $ev->getMessage(), false);

            return true;
        }

        return false;
    }

zKoz210 on 25 Jul 2017

how do i backtrace ..? i provided the code it happened with. directly send a transfer packet to the player and then close them right after

mal0ne-23 on 25 Jul 2017

same happening with me too:- [Server] Server thread/CRITICAL Stopped interface pocketmine\network\mcpe\RakLibInterface due to Undefined index: 208.188.113.205:41019
25.07 12:32:35 [Server] Server thread/CRITICAL Error: "Call to a member function addChunkPacket() on null" (EXCEPTION) in "src/pocketmine/Player" at line 2153
comming version :- db12657