Pocketmine-mp: Excessive use of new Config leads into memory leak

[thebigsmileXD]

Issue description

I found several badly written plugins that use new Config on every tick (task).
When running an 20tps task that open a config via new Config, writes to it and then closes it, you can see the memory increasing quite fast.
My question: Config.php or plugin fault?
On my server this leads into a crash, attached below

Steps to reproduce the issue

1. create task
2. new Config()

OS and versions

• PocketMine-MP: https://jenkins.pmmp.io/job/PocketMine-MP/60/artifact/PocketMine-MP_1.6.2dev-60_a22306d4_API-3.0.0-ALPHA5.phar
• PHP: 7.0.1
• Server OS: Windows Server 2012

Crashdump, backtrace or other files

• https://gist.github.com/thebigsmileXD/592826a1271885f39a1983ed67b552ce

[dktapps]

Can you share source code to an example plugin which causes this?

[ghost]

Confirmed with a simple YAML config type. The leak occurred for me when I used $config->set("key", "data");

I never saved any reference to the config object, seems like something within the class is holding on to the config object reference causing GC to never collect it.

[thebigsmileXD]

@dktapps I'll send it to you in private, due to the code being closed source

[thebigsmileXD]

Somehow makes sense. file_get_contents every time. A deconstructor with fclose would be useful

[dktapps]

file_get_contents() handles that automatically...

[dktapps]

Based on comments above, I created this test plugin: https://gist.github.com/dktapps/4c6790bd80ccd40dfe47362886eb3241

This does not reproduce the issue.

[SOF3]

Closing since this cannot be reproduced.

I have tested with this plugin:

<?php

/**
 * @main TestConfig
 * @name TestConfig
 * @version 1.0.0
 * @api 3.0.0-ALPHA5
 */

class TestConfig extends pocketmine\plugin\PluginBase{
    public function onEnable(){
        @mkdir($this->getDataFolder());
        $this->getServer()->getScheduler()->scheduleRepeatingTask(new class($this) extends pocketmine\scheduler\PluginTask{
            public function onRun($t){
                if(!isset($this->tmpKey)) $this->tmpKey = bin2hex(random_bytes(8));
                $config = new pocketmine\utils\Config($this->getOwner()->getDataFolder() . "config.yml");
                $config->setDefaults([$this->tmpKey => base64_encode(Random_bytes(8))]);
                $config->set($this->tmpKey, base64_encode(random_bytes(8)));
                $config->save();
            }
        }, 1);
    }
}

And it fails to reproduce your problem — executing more than 20000 ticks, the main thread memory only increased by less than 0.01 MB, which I consider as tolerable fluctuation. Your memory leak probably comes from somewhere else, or the config is probably saved asynchronously (see #922)

You are getting the crash line because, coincidentally, this line in Config indeed allocates more memory in reading a file, but it automatically disposes of the memory, as @dktapps has said. I don't think PHP has this bug.

You can imagine this as a frog jumping in the bottom of a well on a floating plank. Every time it jumps, it rises 50 cm and drops 50cm. Slowly, the water level in the well rises, but the frog first jumps out of the well, not by floating out with the plank. This does not mean that the frog's height is accumulating due to its jumps, but simply that the jumps are the topmost layer to be observed.

[thebigsmileXD]

Interesting thing to add: i added a deconstructor for Config.php, that fcloses the files.
This somehow fixed it, but sounds a bit hacky.

[dktapps]

@thebigsmileXD you're deluding yourself. Config doesn't hold or use any resource handles, there's nothing to fclose. I already said that further up.

http://php.net/manual/en/function.fclose.php

[EinBexiii]

I have the same crash.

[jasonwynn10]

32 bit issue?

[dktapps]

I don't know why everyone suddenly seems to think that unexplained crashes are to do with 32-bit platforms. "Oh, it's 32-bit, that's got to be why".

Whether you have the same crash or not is irrelevant. As @SOF3 described above, the reason you're seeing config crashes is due to heavy use of configs (in bad plugins usually) which potentially allocates a lot of memory. What you are seeing is that at some point there is no longer enough memory to allocate because something _else_ is slowly leaking and filling up memory. You could just as easily see crashes due to BatchPacket creation and/or chunk sending, which _also_ allocates a large amount of memory.

[thebigsmileXD]

Plugins _SHOULD_ be allowed to use configs like they want, as excessive as possible. The problem is that new Config always caches. maybe add a non-cache mode.

[thebigsmileXD]

You can reproduce it with all plugins by @Bluplayz

[dktapps]

where is that facepalm gif when I need it...

The cache makes zero difference whatsoever _because nothing holds a reference to the Config_, which means it is garbage-collected as soon as the thing goes out of scope. The crashdump you posted doesn't even have anything to do with the cache anyway.

For the final time: this issue is the _RESULT_ of a leak, _not the cause_. Remove the plugins which are overusing configs and you may then observe more accurate possibilities of a leak.