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

    Lakanate

    /signed.

    I've noticed this problem as well... Thank you Afforess for pointing it out.
     
  3. Offline

    OrtwinS

    /signed

    In fact, ever since 1.8 I haven't been able to use the /reload command at all, it causes some plugins (who are supposed to be up to date) to freak out.
    Results vary from lagging out half my players to a 20 second extreme lag spike that messes up everyones game.

    A build in restart would be nice.
    @Afforess , If we get a build-in restart command, would it be possible to let Spout servers put Spoutcraft clients 'on hold', displaying a 'Server is restarting, please be patient' message, and then auto-reconnecting those clients?
     
    rareshutzu likes this.
  4. Offline

    mindless728

    because even with a plugin being completely leak free, the way that the class loader leaks memory using /reload, but this is because of the way that java works and there really isn't anything you can do about it, this is another reason to get rid of /reload
     
  5. Offline

    rareshutzu

    It used to restart your server. I used it so I didn't had to kill and then start the process every time I made changes. Now I realise it's not working anymore how it should...

    /signed
     
  6. Offline

    Don Redhorse

    what I mean is that when somebody does an /reload bukkit calls the onReload method of the plugins... so a replacement of the original reload function.. are we talking about the same thing?
     
  7. Offline

    mindless728

    well then reload would have to be re-tooled, and btw what you said is the same as disabling then enabling them, realistically you want to reload them from the files so if there is an updated version sitting in the folder it uses that instead of the old version sitting in memory
     
  8. Offline

    bobbysmithyy

    /signed
    yesterday when I reloaded my server it completely deleted my whole entire permissions which took me so long to set up. DELETE IT PLEASE!
     
  9. Offline

    halley

    I know enough of the Java plugin mechanism to agree that there are several huge gaping problems with cycling classloaders.

    That said, why are we removing the feature from Bukkit? If you don't want it used on your server, don't give people the ability to run it. It's handy to use where appropriate, such as during plugin development or configuration experimentation.

    While administering my server, I test new plugins on a couple different staging servers. First is a clean server with almost nothing loaded. Second is a recent copy of the production server with all of the plugins I trust. A simple test I do on each is to load/reload/reload/stop/load a couple times, just to see what shakes loose, takes ages to do, or bloats badly. If I get exceptions on reload or stop, I don't trust the plugin on a production server. Only then do I bother to try client connections, stress simulations, etc.
     
  10. Offline

    mindless728

    @halley its not about letting certain users have commands (i have a small server, so only i have everything), its about trying to reduce the number of help threads as well since any exceptions or out of memory errors thrown because of this are a little harder to debug when the OP says nothing about it

    maybe a compromise is having it as opt-in in the config (say allow-reload) with a commented out warning about using can degrade performance and cause plugins that don't cleanup well to error out
     
  11. Offline

    Don Redhorse

    yes... and I don't want to reload the plugin class files as that will cause a higher memory consumption and leakage..

    to be honest I use reload for testing my plugins all the time.. and I see the threads grow every time in visualvm..

    It is convieniend but not necessary if you only have serveral plugins.. and if you have lots of plugins the reload memory leak will kill you quick..

    On the other side have a reload command for the plugin config would be nice, some developers are already doing it but not all... making onReload mandatory and changing the way reload works could solve it.

    perhaps even make the changes to the reload process configurable... with defaulting to the new way
     
  12. Offline

    mindless728

    might make developers life easy, though if not done right in onDisable() you could get garbage data from the last run so its a detriment there as well. For just this reason i do full shutdowns and restarts when i change the plugin for development just because i work on cleanly shutting down after the fact.

    as for just having them reload config files and such, there is no need to have onReload() since this can easily be done with onEnable() and onDisable()
     
  13. Offline

    tustin2121

    I would sign, but I am split on the issue.

    I, as a server admin, love to change settings on the fly. Now, most of the plugins I use have their own "reload", and I much prefer that because it's faster, but some do not. I don't like taking down the server, if possible, so reload is good for that. BUT, more often than I like, reloading a plugin does not clear all its settings, necessitating a hard reboot.

    I, as a plugin developer, try my best to clean up my plugins on shutdown, though sometimes that's not possible. A plugin I'm nearly done developing now, called LogCompactor, CANNOT be reloaded, because the plugin replaces the Logger package Formatter classes used by the server (so disabling the plugin would do... what? Pull out those Formatter classes and not be able to replace them with the original classes?). I get around this by making a static variable saying whether the plugin has already been enabled and NOT re-enabling it if it has. Hacky, yes, but it keeps objects from being needlessly duplicated. (I guess I should include a reload command to update the config options if they change them.... :/).

    EDIT:
    I'm all for an onReload() method being required!

    EDIT2:
    A lazy developer could just put onDisable() then onEnable() in the onReload() method, but a smarter developer could use the onReload() method to change settings to already existing objects, instead of clearing everything and remaking it all.
     
    DoctorDark, Feaelin and DeanDip like this.
  14. Offline

    Don Redhorse

    why do I have to register my PlayerListener again? at least I do that in onEnable... is that wrong?
     
  15. Offline

    tustin2121

    Also, (and sorry for double posting) I propose the /reload command be altered so that instead of simply typing "/reload", you had to supply an argument, and that would hook into the proposed onReload() method. So, typing "/reload Spout" would call the onReload() function of the Spout plugin, typing "/reload all" would call the onReload() function of every loaded plugin, and typing "/reload server" would ONLY reload the server.properties and server.yml file (which is the main reason for /reload on vanilla minecraft anyway).
     
    datwerd, DeanDip and Don Redhorse like this.
  16. Offline

    mindless728

    well either Bukkit or the plugin should be unhooking the listeners in onDisable(), so yeah i think so

    when i think reload, i am not thinking hey read you config file, i am thinking reset the plugin, as if you need to have a method of rereading the config, do it as a command
     
  17. Offline

    Zaros

    /Signed

    I do believe that something else should be required in its stead. I make a lot of small changes to configs/etc that require a restart/reload. Requiring plugins that have a config to also have a reload would be a very nice way to do it in my opinion.
     
  18. Offline

    cholo71796

    I think it would be best to have a plugin.yml node specifying whether the plugin should be reloaded in the would-be old way. Plugin.ymls lacking the node would continue using the would-be old way, so old plugins would still work just fine.

    Plugins that ignore the default reload system would still be able to tell when the player uses "/reload" or whatever command they want--if the developer so chooses.
     
  19. Offline

    bergerkiller

    I partly agree, since, you see, I am kinda hooked to /reload. I have to swap plugins up to 50 times a day, and being able to:
    - Right-click -> export -> Jar file -> plugin directory -> Export
    - Reload
    - Get some class exceptions (obviously, old file profile changed)
    - Continue testing without major drawbacks

    If /reload simply calls the disable and enable coding, it would make this impossible. (you end up with the class exceptions 24/7)
    (Re-)starting the server isn't an option either, since changes I do usually consist of a few lines of code. It would make debugging a pain.

    I'd say:
    - reload to call disable and enable of all plugins
    - reload http://dev.bukkit.org/bukkit-plugins/ to do the above only for a certain plugin (name) - refresh ([plugin]) to completely re-load the plugin (unsafe) Also, what use does it really have to call enable and disable for a plugin? Because of the 'save in ondisable' strategy it is not possible to change configuration most of the time... And, I agree, the Permgen space errors are a pain, resulting in damaged worlds and inventories out of sync, but the advantages of it should not be forgotten. (well, for plugin devs that is, not general server admins)/
     
  20. Offline

    andrewkm

    /signed
     
  21. Offline

    RazorFlint

  22. Offline

    gameswereus

  23. Offline

    MonsieurApple

    I agree with this.

    Note: Just to be clear I do not speak for the Bukkit team. This is just my opinion.
     
  24. Offline

    Celtic Minstrel

    First, you forgot to mention permissions.yml (and server.yml is actually bukkit.yml), and second, there's no such thing as /reload in vanilla minecraft.

    EDIT: Whoops, I posted without putting my opinion! I don't think /reload should be removed. Changed, maybe, but not removed. I do like the ability to "hot-swap" plugins, for example upgrading plugins without needing to restart the server, so I like the basic semantics of how it currently works, but obviously there are also some issues. Fine then, let's fix the issues instead of removing the command. And as for plugins not properly cleaning up, that's not the problem of /reload; you need to get after the plugin developer in that case.
     
  25. Offline

    Afforess

    You did read the OP, right? Hotswapping will always cause a memory leak. Period.
     
  26. Offline

    RoofToilet1107

    This could DEFINATELY ruin a server............. /signed /highfive Afforess
     
  27. Offline

    mbaxter ʇıʞʞnq ɐ sɐɥ ı Retired Staff

    5mb could definitely ruin a server?
     
  28. Offline

    mbsuperstar1

  29. Offline

    Jaker232

    /signed
     
  30. Offline

    zhuowei

    Didn't even know the risks of /reload before reading this. /signed
     
  31. Offline

    mindless728

    it accumulates, ie 3 reloads = 15mb, so it adds up over time. Keep in mind that this is the base cost, add in plugins that have reference leaks and this cost could easily hit 30mb/reload
     
Thread Status:
Not open for further replies.

Share This Page