Pocketmine-mp: checkTickUpdates on unloaded levels causes the server to crash

[thebigsmileXD]

Issue description

I am unloading worlds for getting reset after a round in minigames.
I've some day suddenly got crashes, and debugged the full plugin.

The way i am unloading the level:

        $server = Server::getInstance();
        if ($server->isLevelLoaded($levelname)){
            $server->unloadLevel($server->getLevelByName($levelname));
            Server::getInstance()->getLogger()->notice('Level ' . $levelname . ' successfully unloaded!');
//imagine some copy_r magic here to restore the old world
                            if (!Server::getInstance()->isLevelLoaded($levelname))
                                Server::getInstance()->getLogger()->notice('Level ' . $levelname . (Server::getInstance()->loadLevel($levelname) ? ' successfully' : ' NOT') . ' reloaded!');

For some reason, while all that, Server::checkTickUpdates($currentTick, $tickTime) is called

        foreach($this->getLevels() as $level){

Should not even contain the unloaded level anymore - but it is passed as null
After that, the server crashes on

                        #$this->getLogger()->debug("Raising level \"{$level->getName()}\" tick rate to {$level->getTickRate()} ticks");

in that function - null->getName() or Level->getProvider()->getName() where the provider is null

So either something is wrong with the unloadLevel function, or with getLevels.

• Expected result: What were you expecting to happen?
  The level being unloaded and not checked for ticks anymore
• Actual result: What actually happened?
  Server crashes due to debug based on null

Steps to reproduce the issue

1. execute the above code on a level which is not the default level
2. see the server beautifully crash

OS and versions

• PocketMine-MP: https://github.com/pmmp/PocketMine-MP/commit/733e61f8150287595ee567770b19b5a7897c4331
• PHP: 7.2 RC 3

Plugins

• Test on a clean server without plugins: is the issue reproducible without any plugins loaded?
  No - You need a plugin that executes the code.

If the issue is not reproducible without plugins:

• Have you asked for help on our forums before creating an issue?
  Why should i, to get messages by little kids "have you tried xxx" or "i can not get that"?
• Can you provide sample, minimal reproducing code for the issue? If so, paste it in the bottom section
  See above actually
• Paste your list of plugins here (use the 'plugins' command in PocketMine-MP)
  Whereas TNTRun is the plugin that contains the above code which crashes the server.

Crashdump, backtrace or other files

• Do not paste crashdumps into an issue - please use our Crash Archive at https://crash.pmmp.io for submitting crash reports to not spam the issue tracker. Add links to your reports in the Crash Archive here.

• Please use gist or anything else to add other files and add links here

• ...

[dktapps]

The context is important - are you running this code in an event? If so, which event?

I expect what is happening here is you're actually unloading the level DURING its tick, intentionally or not. Therefore when the tick function exits, the level has been closed. The tick might take too long as a result of synchronously copying files (or other factors), hence the crash.

This reminds me very much of a similar problem with entity deletion.

[thebigsmileXD]

I am running the re-loading in a delayed task, but after a series of events happened.
Take a look at https://github.com/thebigsmileXD/gameapi/blob/master/src/xenialdan/gameapi/API.php#L59

Eventlistener:

    public function onDamage(EntityDamageEvent $event){
        if ($event instanceof EntityDamageEvent){
            if (API::isArena(Loader::getInstance(), ($entity = $event->getEntity())->getLevel()) && API::isPlaying(Loader::getInstance(), $entity)){
                $arena = API::getArenaByLevel(Loader::getInstance(), $entity->getLevel());
                if ($arena->getState() !== Arena::INGAME) return;
                switch ($event->getCause()){
                    case EntityDamageEvent::CAUSE_VOID: {
                        $event->setCancelled();
                        Loader::getInstance()->removePlayer(API::getArenaByLevel(Loader::getInstance(), $entity->getLevel()), $entity);
                        break;
                    }
                    default:
                        $event->setCancelled();
                }
            }
        }
    }

Plugin's called removePlayer function, called inside onDamage

    public function removePlayer(Arena $arena, Player $player){
        if (count($arena->getPlayers()) > 1){
            $arena->removePlayer($player);
        }
        if(count($arena->getPlayers()) === 1){
            Server::getInstance()->broadcastMessage("If game does not stop, try /lobby", $arena->getPlayers());
            $winner = null;
            foreach ($arena->getPlayers() as $players){
                $arena->removePlayer($players);
                $winner = $players;
            }
            if (!is_null($winner)){
                Server::getInstance()->getPluginManager()->callEvent($ev = new WinEvent($this, $arena, $winner));
                $ev->announce();
            }

            API::stop($this);
            API::resetArena($arena);
        }
    }

[thebigsmileXD]

Neat - I now execute the copy in an async task so it does not block the main thread - this atleast calls no ticking.

Does not remove the core issue tho, that there is no check if level === null

[dktapps]

Well, the core issue is that levels are expected to be loaded and unloaded immediately, and subsequently crash when something later tries to use them. The same problem exists with entities - if they are closed midway through the tick, lots of undesirable behaviour will occur.

This could be fixed with hacks in checkTickUpdates(), but the issue will no doubt be able to occur elsewhere (such as Level->onUpdate(), if, for example, you happened to hit an autosave at the wrong moment).

It's not really reasonable to add closed checks everywhere, as previously discussed for entities. An alternative solution could be to have levels flagged to be unloaded at the end of the tick instead, but this would be a problem for reloading levels.

[dktapps]

For the meantime I would suggest using a scheduled task to unload the level, instead of doing it directly in the event.

[thebigsmileXD]

for example, you happened to hit an autosave at the wrong moment

Crashes 100% of the time though.

Yea, true. Maybe a solution will be found later, for now the async bit works well

EDIT:
Would you look at that, works perfectly now.

[dktapps]

@thebigsmileXD I don't mean that this bug is caused by autosave. I mean that because the core doesn't expect a level to be unloaded during a tick, it doesn't check if the level remains loaded after completion. Autosave was another example of where the bug might occur if this was hack-fixed.

[dktapps]

With the referenced commit this should no longer crash the server, although it will still trigger an exception. The server crash was occurring because it was crashing trying to log an exception.

[dktapps]

This problem was resolved by explicitly disallowing level unload during ticks.