reload clears Metadata before onDisable()

[ghost]

Description of issue:

when doing a /reload in the console, all Metadata is reset before it can be saved with onDisable(). this issue is not present in spigot.

Plugin list:

papertest.jar

bukkit.yml, spigot.yml, paper.yml, server.properties

{default settings}

Other helpful links

the already build jar file if you trust me. http://www.fast-files.com/getfile.aspx?file=141003

the source for reference or for if you don't trust me. https://pastebin.com/Bihepg2Q

Paper build number:

"This server is running Paper version git-Paper-1143 (MC: 1.12) (Implementing API version 1.12-R0.1-SNAPSHOT)"

What behaviour is expected:

I expected to see "[21:34:51 WARN]: [papertest] onDisable(): value of dodo_hacker: I do exist!" in the console.

What behaviour is observed:

I found an java.lang.IndexOutOfBoundsException in the log. this is because the list was empty.

Steps/models to reproduce:

1. install the test plugin.
2. join the server.
3. perform a /reload.
4. watch the logs.

[ghost]

In this zip is the .jar papertest.zip thought to better do it this way.

[zachbr]

This is caused by a patch intended to prevent leaks and breakage with objects. Unfortunately, it is now because of that patch that a plugin may not be able to save that metadata elsewhere before it is removed.

Due to the way it is implemented, it must be called before all plugins are actually removed from the server so it can loop through them to remove the data.

From my initial look through, this was placed before the disable event because plugin disabling (including calling onDisable) is handled in the API layer but all of the metadata lists that need to be cleared are in the implementation layer. Therefore, there is no easy path to handle this without restructuring the API or exposing some way to clear these lists to the API, which may not be ideal.

Further work is needed on this front to determine an acceptable solution that will allow plugins to properly save their associated metadata before it is cleared.

[ghost]

so it isnt possible to add pluginManager.disablePlugin(plugin); as the first statement to the for-loop? :/

[electronicboy]

Couldn't we just copy the array of plugins and then clear them afterward? https://github.com/electronicboy/Paper/blob/clean-meta-after-plugins/Spigot-Server-Patches/0070-Remove-Metadata-on-reload.patch#L17

I do not see any potential bad effects of this, nor do I see why this wasn't the original designed/implementation beyond what is below and ideal practices of plugins, however, I wasn't around when this was added so many there was something odd

However, I would say that making non-persistent storage act as persistent storage is bad practice and you should really be handling this in your plugin instead of leeching on the back of the metadata api which is somewhat flawed in implementation.

[MiniDigger]

and you should really be handling this in your plugin instead of leeching on the back of the metadata api which is somewhat flawed in implementation.

I think what he shows is a valid usecase for the metadata api tho. while server is running, mark stuff and when server is stopping, clean that stuff up.

[ghost]

@Zbob750 May I know what the reason is that "pluginManager.disablePlugin(plugin);" isnt used? (I am probably missing something important.) This change wouldnt require a copy of the plugin list. From phone.

[aikar]

@dodohacker it is used, by clearPlugins(), right above the change.

Before the code relied on manager.getPlugins(), and now that list will be empty at point of this call to clear metadata. so a clone before they are removed is needed to then iterate it post unload.