Petition to remove the /reload command

Discussion in 'Bukkit Discussion' started by Afforess, Oct 27, 2011.

Thread Status:
Not open for further replies.
  1. Offline

    Afforess

    The reload command in bukkit is diabolical, and should be removed. I've stated this many times in private before, but I'm often surprised by the number of people who are unaware of the problems with reloading.

    Memory Leaks:
    To understand why the reload command is terrible, you have to understand how it works. /reload disables all active plugins, and then, creates a new instance of every plugin in the plugins folder. It does not reactivate the old, disabled plugins. It creates a new copy. Due to the way Java works, and how Bukkit implements it's classloaders, this causes a memory leak. This is easy to confirm with most java profilers. The memory leak is not super significant (I'd guess at most, 5mb per /reload), but it does exist. Unfortunately, Java's eccentricities make it pretty much impossible to fix this memory leak.

    The effects of the leak can be viewed by my profiling some months back. Nothing has changed with plugin loading since these screenshots were taken.

    Server running BukkitContrib without any reloads.

    Server running BukkitContrib 4 reloads later. (Plugins loaded 5 times total, 4 reloads + 1 startup)

    All of the classes and any static variables remain in memory.

    Poor Plugin Design:
    Memory leaks are not the worst part about the reload command. Many, MANY plugins handle reloads poorly, or not at all. I'm certain most server admins are painfully aware that when they use /reload, the console will spit out at least one error, usually more. Exceptions are extremely common with /reload. And before you begin to berate plugin developers for simple mistakes, know that it is often extremely difficult to handle reload situations perfectly, especially the more complex the plugin. The most complex case I am aware of is Spout, where we have literally, thousands of lines of code simply to handle the /reload command. And Spout still doesn't handle reloads perfectly. Plugins that use multiple threads to achieve their tasks (usually plugins using some form of SQL, or persistent storage....) will also fail to end their threads after the plugin shuts down. This means your server could be wasting CPU cycles for extra threads that are not needed or used.

    Lag, Lag, Lag:
    Assuming that your server has only selected the highest tier plugins, of exceeding quality that handle reload flawlessly, and has gigabytes of extra memory to handle any leaks from bukkit, reload will still lag for several seconds after you issue the command. It is not uncommon for players to be kicked for fly hacking, moving to quickly, or just to read time out from lag. You might as well have used the 15 seconds it takes to RESTART the server instead of reloading it.

    There are several plugins made specifically to offer a way to safely RESTART your server instead of reloading it, and I encourage admins to use them:

    http://forums.bukkit.org/threads/ad...w-v0-3_2-full-server-restarts-818-1060.20039/
    http://forums.bukkit.org/threads/ad...-automatic-rebooting-and-commands-1000.24652/
    http://forums.bukkit.org/threads/ad...t-down-at-the-same-time-every-day-1248.26101/
    http://forums.bukkit.org/threads/ad...detection-auto-saves-remote-console-1317.674/

    I'm personally inclined to simply block the reload command from any of my plugins, but I expect that would make admins unhappy. So instead, I'm trying to raise awareness of why the command is bad, what can happen when you use it, and alternatives for it.
     
  2. Offline

    MCTSS

    /signed
     
  3. Offline

    ks07

    At the same time, you could have a server owner who also doesn't know squat, and doesn't read the documentation for their plugins (you mean these words were there for a reason? ;)), and then ends up complaining to the dev that their plugin is horribly flawed... Ending up just wasting time. I think a way to override the plugin dev's decision would be in order, just in case that does happen. Perhaps the setting could be stored in the plugin.yml, at least then it's easy for non-devs to change, yet not so easy that people will just change it without thinking.
     
  4. Offline

    mindless728

    and now i would be back at the premise, get rid of it completely because everyone is retarded unless they can PROVE otherwise
     
  5. Offline

    ks07

    Problem with that is it's removing functionality that isn't always broken and that is useful in some situations. Assuming that everyone is retarded and can't be trusted to make their own decisions isn't a very fair summation of the Bukkit community, is it?
     
  6. Offline

    mindless728

    no, it is ALWAYS broken due to the reference leaking that occurs and most plugins don't do anything to clean up after themselves resulting in a more broken /reload

    and that idea is for people in general which includes the bukkit community
     
  7. I agree with what some others sayed before: Change the /reload command to call something like onReload() in the plugins, so they can reload config files, for example. Every dev should be able to use this wisely as many plugins still have their own reload method and if not, well, then they should learn how to fix it when users start to cry that their plugin freaks out on server reload.

    Another thing is: Integrate something like the server restart plugins in the OP directly into bukkit with a new command: /restart. Somebody wants to add in a plugin or update it? /restart. He wants to update bukkit? /restart. He wants to reload bukkits and/or the plugins configuration files? /reload. And, last but not least: He wants to reload the configuration of one plugin? /reload <pluginName>.

    This would even avoid lazy admins crying: "I cannot reload my server anymore! What's wrong?!?" :D

    The only bad thing is that I'll bet the memory leaks will count for /restart, too, even if bukkit gets a new startup class which then starts a instance of bukkit and handles the restart, because there will still be static variables left, right? So either we have to make a non-java bukkit loader who restarts the whole java process or just give a s*it on plugin hotswapping (and don't integrate /restart).
     
  8. Offline

    ks07

    @V10lator The separate reload command, with it's own hook, sounds like a good idea that shouldn't be too hard to implement. A lot of plugins definitely already have commands along the line of /myplugin reload, so a bit of refactoring and voila - new reload command.
     
  9. Offline

    Rednax_

    Removing all memory leaks IS possible, just not within ANY reasonable amount of effort.
    Therefor you have my vote
    /signed

    Adding a /restart with support for spout would be a very good replacement and generelly a good new command.
    Even if you decide not te remove the /reload command a /restart command would still be good.

    Also the /load commands sounds very usefull. An admin can set the plugin up on a test server, and than place it on
    the real server without restarting it.

    At least add a warning msg to the users of the /reload command that it might not work properly and give memory leaks.

    ps.
    How ironic is it that the stystem that is supposed to counter memory leaks (aka GC) is causing memory leaks :p.
     
  10. Offline

    jasvecht

    /unsigned

    Instead of removing what is a great idea (who enjoys restarting the server for every change? Anyone?),

    Why not replace it by a trigger send to all plugins and then let the command handled separately by all plugins to whipe old data and/or reload all config files?


    Also, a /load command would be epic.
     
  11. Offline

    ZachBora

    I would add a command to load a single plugin (for when you just added a new one).
    And I would make a server property to enable/disable the /reload. I've told my server admins to stop using it but sometimes they do it for some reason. And at some point we're forced to really reboot it because everything became slow.
    I still want /reload for when I test my plugins locally. It is a quicker way than stopping and restarting my test server.
     
  12. Offline

    ks07

    This is beginning to sound more like the hMod plugin command structure, which wasn't half bad for server ops imo. Wasn't there a reason these commands weren't added in the first place? Something to do with the new plugin architecture iirc, but I have no idea.
     
  13. Offline

    ZachBora

    Add a function to the JavaPlugin interface called Reload that plugins can override to do proper reloading hmm?
     
  14. Offline

    matejdro

    /reload is useful when developing plugins. Instead of restarting whole server, I often just /reload after updating jar file, because It's easier and I do not have to reconnect. But still I have to reboot my sever from time to time, because I eventually get out of memory error :D. But other than that, I don't think that it should be used anywhere else, since reboot is far more stable solution.

    Thanks for pointing that out, I guess I will use /restart plugin from now.

    /signed
     
  15. Offline

    emericask8ur

    /Signed , But it is usefull but we shoud leave it to the Developers with the plugins to do the /Reload stuff
     
  16. Offline

    ZachBora

    Something I just thought of, can we do a /reload command that every plugin catches then sends back up? like /reload could be used by multiple times.
     
  17. Offline

    DDoS

    /signed

    I've stopped using /reload because I was having problems with async tasks for some plugins.
     
  18. Offline

    md_5

    /notsigned

    I do occasionally have async issues, and maybe I should take a look at Bukkits reload code, but it is a very useful command, and the best approach would be to redo it to make it do everything a /stop would do, but plugins only, while still cleaning all of them up.
     
  19. Offline

    phrstbrn

    I disagree. While I agree that a sane admin should NEVER use /reload, the functionality should be there, for no other reason than to force plugin owners to cleanup their plugins properly. I sometimes will hot load/reload individual plugins (never the whole sever, that's insane), and without proper onDisable handling those plugins probably wouldn't hot reload as well as they do.

    Leaving /reload in gives awareness to plugin developers, hey, this use case should be handled properly. As an server admin, your job is to know what commands you should use, and when it's appropriate. If you don't understand the repercussions of using a /reload, and can't use it properly, then you really shouldn't be complaining when things go downhill.

    I feel that removing the /reload command would remove some awareness of this important use case, and that plugin developers should be aware of it.

    This plugin I've found helpful for loading/unloading individual plugins:

    http://forums.bukkit.org/threads/ad...d-unload-reload-individual-plugins-1240.32853

    Not everything works with it, and the ones that don't generally fall in the same category as the ones who don't work on /reload, but at least for the plugin devs who are aware the /reload is a valid use case and code around it, server owners can still hot load/unload plugins.

    I understand there can be a memory leaks reloading plugins, but when given a choice between 5MB of memory and a server restart, I'll take the 5MB leak almost any day. My server will restart later, and "losing" 5MB of memory until the next restart is much better than taking the whole server offline and kicking the 50+ online players to update some silly plugin once.
     
    Celtic Minstrel and obnoxint like this.
  20. Offline

    ZachBora

    This thread made me think I might not be disabling my plugins correctly. Is there help somewhere that shows what should be done in a ondisable other than making null some variables?
     
  21. Offline

    XtenD

    /signed
     
  22. Offline

    Lolmewn

    Meh, I use it quite often to reload test plugins.
    Screw RAM, I got loads!
     
    PatrickFreed likes this.
  23. Offline

    undeadmach1ne

    this. i am a noob and this thread is making me all paranoid that im making a huge mistake somewhere. some further info would be greatly appreciated.
     
  24. Make sure to set all static variables to null in onDisable(); :)
     
    undeadmach1ne likes this.
  25. Offline

    undeadmach1ne

    thanks :)
     
  26. Offline

    Celtic Minstrel

    I dunno... there are a lot of them. <_<
    So fix it. :p Don't remove it because it's broken.


    I agree with much of what phrstbrn said. In particular I would note that the /reload command should probably be extended to be able to load individual plugins rather than the whole server. Perhaps to the point that you should need "/reload all" to reload all plugins, and a plain "/reload" only reloads the server data (server.properties, bukkit.yml, permissions.yml, recipes, maybe other stuff, I dunno).
     
    AegisZephyr likes this.
  27. Offline

    Sayshal

    /signed
     
  28. Offline

    AegisZephyr

    I agree with Afforess on this. I have found my server to be very laggy and using excessive amounts of RAM after a /reload. The other admin (pausekuk) and myself know that using /reload is bad so we have to restart after a plugin change (Thank you plugin devs that include some sort of /plugin reload command in your plugin). I think all plugin devs that have configuration files should work some sort of plugin reload command as well into their own plugins. Perhaps the Bukkit team could develop a sort of universal hook or something that plugin devs can use for their own plugins would be handy. I kind of miss the old HMod "/plugin enable/disable/reload/load [nameofplugin]" command. Perhaps that could be the new /reload command for Bukkit?


    /signed
    AegisZephyr
     
  29. Offline

    Afforess

    @Celtic Minstrel I'm not sure you are reading the same thread as I am. Hotswapping plugins doesn't work because it's a Java limitation. It's not something that can be fixed.
     
  30. Offline

    Hesse77

    So you suggest we purely restart?

    Is /stop okay?
     
  31. Offline

    LEOcab

    Spout uses thousands of lines to handle a reaload? Y u no just do function onReload() { unload(); load(); }?

    Just kidding, I don't know what I'm talking about. I agree with OP, I used to do /reload all to install new plugins and I always got errors. One time my server crashed with a weird Java error. Kinda like the one it throws when it's out of memory. Now I just restart server with /etc/rc.d/rc.bukkit restart. Go Linux. :D

    /signed
     
Thread Status:
Not open for further replies.

Share This Page