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

    V10lator

    Can I have a look at the script, please? :)
     
  3. Offline

    Spl1tz

    /signed
    IMO all plugins should include their own reload command.
     
  4. Offline

    fsking541

    I noticed this too. My minecraft usually runs at 500-700k on my computer and the server at 300k on good days but i reloaded once and my server got up to 900-1000k. I was amazed. I didnt realize that this is what was causing it to lag so much.

    /signed
     
  5. Offline

    xcanner

    Never used /reload on my server or test server. DonĀ“t know why, but i've never trusted it :)
     
  6. Offline

    Evangon

    /sign
    I'm going to say redesign it to be more effiencent. Last resort is removing it if they can't redesign it.
     
  7. Offline

    Bradley Hilton

  8. Offline

    dockter

    /Signed.
     
  9. Offline

    Borch

    This should be up to the plugin authors, they should be able to set a flag whether their plugin supports reloading or not. If not, the plugin will be skipped. If yes, a reload happens. Then you're "just" left with the memleak issue, which should be acceptable. Maybe add a console output in bukkit that will warn the player about the impact every time he uses /reload.
     
  10. Offline

    NinjaZidane

    I agree to this petition as well...this command causing way to much harm to occur and should have never been used this way.
     
  11. Offline

    BooGaLoo90

    /signed
    i read the first post, BUT

    the forum thread: tl;dr

    is there any way to fix what the /reload command has caused?
     
  12. Offline

    efstajas

    Yeah, my server always totally messes up after a reload. All warps get deleted and the server lags for around 5 minutes.

    /signed
     
  13. Offline

    iffa

    Have I signed this yet?

    /signed
     
  14. Offline

    Tarheels

    /signed
     
  15. Offline

    NuclearW

    Because apparently you all could not be simply asked to stop arguing, I've cleaned up this thread and removed all the posts in the argument, and those related directly to them.

    I understand some valid points were made, if you feel those are directly relevant to the discussion at large you are free to re-post them out of the context they were originally posted in.
     
  16. Offline

    JakebIngram

    I see no reason... people can just not use it.
     
  17. Offline

    obnoxint

    Hmpf. Better than nothing. Even if I don't understand why you also deleted my posts before you made your statement. There where no posts of mine after it.

    Where have I been?... Yes
    - clean up your mess (as an plugin author)
    - reload-command should stay (for the people who know about the risks)
    - upcoming developers and server owners must be aware of the problems and it's our responsibility to make them aware.

    I think this outlines the statements I made earlier.
     
  18. Offline

    Afforess

    I appreciate all the comments and feedback on this thread.

    For Spout, we've decided to drop support of the /reload command.

    Any user that reloads the command will see this message appear:

    Spout does not support the /reload command.
    Unexpected behavior may occur.
    We recommend using /stop and restarting.
    Or you can use /spout reload to reload the config.
    If you want to use /reload anyway, use the command again.

    As the message mentions, I also added '/spout reload' to reload the configuration file for Spout, because I understand a lot of people used it for that purpose. I would urge other plugin developers to add a similar option.
     
  19. Offline

    NinjaZidane

    By far the best path to take with Spout or any plugin in general.
     
  20. Offline

    Tadas159

    /signed
     
  21. Offline

    Lunar Delta

    Bravo. I was hoping this would happen.
     
  22. Offline

    mutiny

  23. Offline

    Joager

    This thread:

    " STOP USING WHAT I DON'T WANT YOU TO USE! "
     
  24. Offline

    mradr

    yea... it does seem poorly design... most plugin just seem to recopy it self without really reloading a fresh copy of it self... so I am /sign this one also
     
  25. Offline

    obnoxint

    No, Joager, you got it wrong. Afforess opinion is, the command should be removed or redesigned, because it causes a lot of problems. But there are also people (like me) who don't want this command to be removed, but instead warn the users about the problems. And I also think, a plugin author should clean up his mess, when his plugin is going to be unloaded. But there are some "monsters" of plugins (like Spout) which are too complex to clean up. And there also are dependencies between plugins that cause problems because of an unpredictible unload order. I recommend you to reread the pages, I think you missed some very important statements.
     
  26. Offline

    Celtic Minstrel

    You shouldn't say things are impossible, unless you like being proven wrong. :p

    Unfortunately, I don't have the knowledge to prove you wrong. I wouldn't be surprised if fixing it requires C code with a JNI interface, or something crazy like that, but I doubt it's truly unfixable. It's probably not worth the trouble it would take to fix it, but that doesn't mean it can't be fixed.
     
  27. Offline

    dvdbrander

    And why do we all want to remove it? Fixing it would be way better. Or some sort of /restart instead of /reload
     
  28. Offline

    jasvecht

    How dare thee steal the avatar I used for over 300 internet years! (5 real life ones)


    That said, as I stated before (and will now expand on), while the current function of the command fails pretty badly, I truly can't see the harm of, instead, doing the following:

    Search for any unloaded plugins - > Attempt to load them
    Send a little request to every plugin, allowing each individual plugin to handle it as they wish. Usually, I suspect this'l mean the plugin will perform a little clean up and reload the configuration.


    Good plan, doc?
     
  29. Offline

    Raphfrk

    The reason you aren't allowed do individual reloads is partly because of the issue inter-plugin communication. With hmod, you could reload individual plugins.

    However, with Bukkit a plugin might have an Object from another plugin. If you reload the other plugin, then Object is out of date.

    It might be possible to get around it with dependencies.
     
  30. Offline

    phrstbrn

    Not every plugin is a dependency. Plugins that are not dependencies for other plugins can generally be reloaded indivudually.

    Spout is an instance of a plugin that will generally never reload properly, since for most servers, it's going to be a dependency for a lot of plugins. But something that is either standalone, or is not depended on by other plugins can be hot-reloaded without any issues most of the time.

    I understand what issues Afforess has with the /reload command, mainly since he develops Spout, which IS a large dependency, and something like Spout should never be reloaded, however disabling a feature that should exist is not the correct solution.

    I'd be all for an option in the plugin.yml which allows authors to prevent their plugin from being reloaded during a /reload. Plugins like Spout, iConomy are good examples of plugins which should probably never be reloaded. However, MySimpleCombatPlugin or MySimpleRPG plugin can usually be reloaded without any issues, since they aren't going to be a dependency.

    As I stated before in this thread, it's generally a really bad idea to reload every single plugin on your server, and executing a /reload is probably a bad idea. But reloading individual plugins can be useful, and just removing this feature entirely is the wrong way of solving this problem.
     
  31. Offline

    Raphfrk

    I agree that a /reload <name> would be useful. However, care needs to be made that none of the objects are used outside the class.

    Something like

    /reload <name> -> This reloads that particular plugin (or gives an error if the plugin has it blocked)
    /reload -> This reloads all plugins that allow it, but keeps the old versions for any that block it

    /restart -> This restarts the server (but can't really be done by java internals)

    Ofc, this doesn't fix the inherent memory leak, but reload is useful for coding plugin, so you don't have to restart. /reload name would be even more useful.

    Apparently, the problem is that ClassLoaders have a strong references to all Objects of that class and all Objects have references back to their class loader. To nuke a class, you need to

    - remove all references to any objects of that class
    - remove all references to the class loader of that class

    I think each plugin gets its own class loader, so maybe it would be possible. I know that there are various Class caches all over the place :).

    Assuming that is correct, then reloads could in theory be made safe. Each plugin could be given an onReload(Plugin plugin) method. It would then be required to clear all references to classes from that plugin.

    However, getting plugins to actually do that would be pretty hard. Ideally, there would be some kind of auto check. For example, when you reload, there is a check performed to see if you have queued any async tasks.

    Speaking of which, in theory, CraftBukkit could prevent the creation of new threads by plugins. For example, there could be a BukkitThread extends Thread class that plugins are forced to use. It would work exactly like a Thread object, but Bukkit would record what plugin started what thread. If you disable a plugin, then Bukkit could scan all threads started by the plugin and see if any are still alive.

    Error messages would guilt trip the plugin author into using threads properly. It could be 2 stage

    - first have Bukkit give a warning message if a plugin uses Thread instead of BukkitThread
    - second block the use of native Threads by plugins
     
Thread Status:
Not open for further replies.

Share This Page