Pocketmine-mp: Level object memory leak on world unload

[dktapps]

Issue description

When a world is unloaded, the Level object is not destroyed due to Position objects holding references to it. This causes myriad bugs, the most famous ones being examples like getName() on null (https://github.com/PocketMine/PocketMine-MP/pull/4205), Level::getProvider returning null (https://github.com/iTXTech/Genisys/issues/1908).

The memory leak is very minor, barely worthy of the name "leak" at all. However, the effects are widespread.

Why this happens

Players with spawn points in worlds that are later unloaded will, for example, hold Position objects pointing to their spawn point in the unloaded world. These Position objects hold strong references to the Level object, which is subsequently not destroyed even after everything related to it is closed, most notably the LevelProvider.

The getName() on null bug occurs when a player with a spawn point in an unloaded world either attempts to teleport back to their spawn point, or when they quit from the server and their data is saved.

Steps to reproduce the issue

1. Set a player's spawn point in world x
2. Unload world x
3. Kill the player, or otherwise attempt to teleport them to their spawn point.

Possible solutions

Use Level ID

Instead of holding a direct Level reference, Position objects could instead keep the level _ID_ of the level that it points to. Then, when getLevel() is called on a Position object, obtain a Level reference from the server directly. If the world is an unloaded world, this will return null.

Problems

• Every Position object would either need to hold a Server reference, or obtain one on-demand using Server::getInstance().
• Position currently has a _public_ Level property. This is used directly by many things in the core, and removal of this property would require either a magic __get() method or all access to the level property to be done with Position::getLevel(), which would have to get a Level reference from the Server ondemand. Both of these solutions have potential to impact on performance due to how widespread the use of Position is.
  

  Use WeakRef

Use WeakRef to hold a Level weak reference instead of a strong one. This would eliminate the problem, but would bring back some performance issues which are the reason Position weak references were removed before now (they previously did use weak references).

Problems

• WeakRef doesn't play nice on PHP7, especially when used with pthreads.
• Performance/memory impacts.
  

  OS and versions

• PocketMine-MP: https://github.com/pmmp/PocketMine-MP/commit/2fba1073ddf4965eb5d2bbbbbfb4a5c5f9377db3
• PHP: doesn't matter
• OS: doesn't matter

[legoboy0215]

You finally found the reason 👍

I think the first solution is more viable.

[dktapps]

@legoboy0215 I've known for a while what the cause was, but spent a while trying to work out ways to fix it. I couldn't come up with a good way to do it, so I opened this.

[archie426]

I have noticed this on my server as well

[SOF3]

If you have to choose between the two solutions, use the former one. The impact of having a WeakRef in every position is really expensive.

I would like to enquire, however, if there is actually a big problem with the use of Server::getInstance()? Really, it isn't even laggy.

[dktapps]

Anyone fancy testing https://github.com/pmmp/PocketMine-MP/commit/87ba3264cfdce5c4c6d08bac6b9400e7d082b36e ?