Memory leak when deleting root commands

[Mr-EmPee]

When you delete a root command by using #deleteRootCommand some references to it are kept.
I'm using cloud-paper and cloud-annotations 1.8.3 on a 1.19.3 server

[rgnter]

That's minecraft keeping a reference in the brigadier dispatcher. You can't delete that. It's going to be deleted on reload or restart.

[Mr-EmPee]

@rgnter Why isn't that deletable? Do you mean through API?

[rgnter]

When plugins are enabled, bukkit commands are merged with vanilla commands, and stored away in command dispatcher. This dispatcher is not available through the API. The wrapped command references bukkit command, which references cloud command.

[Mr-EmPee]

My question is why can't we just access the map with reflection and remove the command?

[rgnter]

Because using reflection is inefficient, burdensome(from maintenance point of view) and slow. Let them keep the reference, it's not that much trouble for us.
I can't see any real reason why to remove nodes. If there's something I can't see, enlighten me.

[Mr-EmPee]

Okay that's understandable but we should at least break the chain that links the command class (Where you define the command annotations) to the BukkitCommand, bc if the chain hasn't been broken that class can't be garbage collected and that could cause a big memory leak depending on how someone has structured their code.

[rgnter]

I like how you think, but we can't. It's impossible

• BukkitCommand is the only way to register commands in the API.
• API doesn't provide a direct layer to the Brigadier API.
  • All commands registered to Brigadier API are treated as vanilla commands (appending minecraft namespace in front of them)
  • We could to it hacky-way, but that's just irresponsible and could cause undefined behavior

And honestly, the "leak" is trivial. Nobody really removes nodes. And if, the memory footprint wouldn't be that huge to call it a "leak" (couple of hundreds bytes in HEAP). It is irresponsible, but we can't do much about it, unless we get direct access to the brigadier API.

[Mr-EmPee]

What I meant by breaking the chain was setting this field to null
https://github.com/Incendo/cloud/blob/a4cc749b91564af57bb7bba36dd8011b556c2b3a/cloud-annotations/src/main/java/cloud/commandframework/annotations/MethodCommandExecutionHandler.java#L52

[rgnter]

So, we can't make it null, because the node doesn't know when it's removed. And if it knew it's removed, how can we be sure that the node is not going to be added elsewhere.

[Mr-EmPee]

And honestly, the "leak" is trivial. Nobody really removes nodes. And if, the memory footprint wouldn't be that huge to call it a "leak" (couple of hundreds bytes in HEAP). It is irresponsible, but we can't do much about it, unless we get direct access to the brigadier API.

The leak could be big depending on how your code is structured. For example, you just need to have a cache that stores some data or a reference to an instance that holds and manages a cache.

I think that this problem shouldn't be overlooked or at least should be mentioned in the doc of the deleteRootCommand bc
I would expect (and probably other dev too) that if there are no references in my code to an instance and after doing a deleteRootCommand to be garbage collected.

So, we can't make it null, because the node doesn't know when it's removed. And if it knew it's removed, how can we be sure that the node is not going to be added elsewhere.

That's true this means that we should provide a way to "invalidate" the MethodCommandExecutionHandler.
I want to state that idk internally how cloud-annotations or cloud-core works and I may be missing some important concepts.

[rgnter]

I see your concern, but hacking our way through the API just to fix one small issue for Bukkit is kinda-sorta bad. The issue only occurs when deleting a node which is based on annotations (API for node removal is also experimental). Lesson learned: decouple your logic and data.

We probably can't do much other than document it.

[Mr-EmPee]

I don't think that hacking is the only way to fix this. There are other ways as I stated above

[rgnter]

I don't think that hacking is the only way to fix this. There are other ways as I stated above

By hacking I meant introducing new experimental API to release the method handle. 😀 There's really no way for us to tell when to safely release the method handle.

You're right that this situation is bad and irresponsible of us to ignore, but when user of the library - in this case Bukkit, is holding a reference we can't so much about it. Bukkit never intended for commands to be removed.

Imagine case where you provide a annotation based command, and another internal mechanism takes the reference to that command (not just bukkit) and you decide to release the method handle just because you removed the root node.
Okay, Bukkit is not going to use it, but what about that other internal mechanism? We could cause something worse.

solution: Disable the removal API for nodes on the Bukkit platform. Until direct interoperability with Brigadier is possible.

[rgnter]

Although I might have a simple solution to this. Once I'm available I'll try to craft something.

[Mr-EmPee]

I was thinking of wrapping the MethodHandle inside a WeakReference, this way everyone is made conscious that the reference may become null at any given PIT and thus should handle the case.

[rgnter]

Wrapping the methodHandle directly would cause the GC to remove the command almost immediately. As there would be no strong referent.

[Mr-EmPee]

I meant on the getter, internally it would still be used as a normal reference.

[Mr-EmPee]

Btw, I've just seen that the CommandMethodContext has references to the instance as well

[OOP-778]

Hello, has this been fixed since I just got to this.

My proposed fix is in the BukkitRegistrationHandler, it uses the player#syncCommands to sync commands to players, but that doesn't update dispatcher, to update the dispatcher you can use CraftServer#syncCommands() method that will update the dispatcher and players. Need some reflection, but that's how I got it to reload properly.

Also noticed that not all aliases are being removed, only the namespaced ones from the knownCommandMap
so if I have a command bp under a plugin neo, it removes neo:bp, but bp itself does not get removed if it's an aliase.