BaseCommand Memory leak

[cijaaimee]

BaseCommand holds the operation context of the last time the command was executed. I suspect this may be the cause of a memory leak. This includes the command issuer, which, in Bukkit, holds a reference to the CommandSender. In the case of a player executing a command, this causes the Player object to not be collected by the GC until somebody else runs the same command. On top of that, if the player was on a world no longer loaded on the server when they disconnected, a reference to the entire World object is kept.

[CyberFlameGO]

Oh dang yeah that’s pretty bad

[embeddedt]

Perhaps this can be fixed by using a WeakReference here?

[cijaaimee]

Yeah. That's pretty much what I did as a bandaid fix for myself. I hoped maybe someone with deeper knowledge of the framework would be able to make a better fix for it.

[Akiranya]

I believe this has caused memory leak on my server.

I dumped the memory, and looked into the GC roots of pig zombie entity instances, many of which show that CommandIssuer is the cause of memory leak. There were 20000+ leaked monster entity instances in the memory snapshot. This would happen if some player runs a pig zombie farm and keeps farming all the day.

So I guess the reason why acf would accumulate the pig zombie objects in memory:

Pig zombies have anger and Minecraft stores the anger in the EntityPlayer object which is stored by CraftPlayer which is stored by CommandIssuer. So the BukkitCommandIssuer makes all the pig zombie entity objects, which ever had anger at the player, retained in the memory. Restarting server wouldn't help much because it's just leaking too fast :\ imagine if the farm spawns 15 pig zombies per sec

Here is the evidence:

This was one of the many phantom pig zombies. There were 20000+ phantom leaked pig zombie entity instances while there were just ~200 real pig zombies in the world.

The GC roots are merged GC roots.

[kyngs]

Is there any update on this? This could potentially be pretty serious.

[aikar]

well 'could be' is def going to really vary on lots of variables. In the screenshot above it was due to the player having entity damage as last damage source holding refs to the world chain.

But the proper fix is relating to https://github.com/aikar/commands/blob/master/core/src/main/java/co/aikar/commands/BaseCommand.java#L544

in postCommandOperation above this need to set this back to null.

[aikar]

Also id like to note this issue is absolutely not 'major', it's been like this for many years with no one noticing. While leaking the player object isn't good like this, what this shows is that it's identified an issue in Spigot/Paper too, in that the last damage cause can leak an entity reference which can reference no longer valid entities/worlds.

An issue should be filed for Paper too, that the entity ref on the damage cause should likely be a weakref and also include an .isValid() check.

If this entity wasn't leaked on the damage cause, it would not have been such a heavy impact here.

I am unable to fix this issue myself as I don't have java dev environment set back up on new PC, but I have asked chicken if he can verify my fix doesn't break things when he has time.

A weak ref here isn't the solution, we ultimately just need to be clearing that thread local as the context object doesn't really serve any value post execution.

[chickeneer]

I have pushed up that fix. I tried to modify the javadoc description for the accessor method for the last context to reflect that it can be null outside of command execution altogether. Instead of just before it has run the first time.