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

    mbaxter ʇıʞʞnq ɐ sɐɥ ı

    But if you feel that 5mb is a big threat to your server, you don't demand the feature be removed, you just don't use the feature and instead restart the server :)
     
  3. Offline

    Mike724

    Totally agree with everything you have said. I'm developing my own plugin, and the /reload command is KILLING me.

    /signed
     
  4. Offline

    Celtic Minstrel

    I cannot believe that it's absolutely impossible to programmatically unload a class... after all, unloading a class is a pretty fundamental operation. Still, if that's too much work, I could live without it; I usually restart the server when I'm changing plugins, anyway. However, I don't think the /reload command should be removed.
     
  5. /signed
    @Afforess can you remove RestartNow as a link to restarting the server though? Its highly inactive :)
     
  6. Offline

    Afforess

    This is Java we are talking about here, remember? :p
     
  7. Offline

    Don Redhorse

    btw: is there any reason why to NOT use a singleton listener with bukkit?

    I think all of the examples use normal classes or a form of singleton I don't know (which is possible as I'm a freshman in java coding)
     
  8. Offline

    losdamianos

    U dont like it ? DONT USE IT !!! simple
     
  9. Offline

    desht

    Fully on board with removing /reload.

    As a minor convenience, however, it would be nice to have a command which loaded any new plugins which weren't previously loaded, so a new JAR could be dropped in and enabled without needing a cold restart.
     
  10. Offline

    tustin2121

    I had to use /reload on my server 5 times the other night. The 5th time, it took so long to reload that all my players lost connection due to endOfStream timeout. :confused:

    I still stand by my earlier suggestion, though, about changing /reload to make the plugins handle it, instead of reloading all the classes like it does now. Also, it would be nice if you could /disable and /enable plugins individually, without /reload. :)
     
  11. Offline

    MCManager

    if you dont like it, dont use it
    i still wanna use it, memory leak or not
     
  12. Offline

    Afforess

    Are you giving me the thumbs up to block the command from my plugins then? :p
     
  13. Offline

    AgentKid

    /signed

    Many of my friends who also own servers come to me complaining about problems, then I open the log file and, oh look, they used /reload and the SQL connection didn't close properly. /reload is just a pain in the ass and it's much better for the server and the plugins to just have a reboot. So what if you're down for 30 seconds? It's better than having lost plugin data due to the reload.
     
  14. Offline

    MCManager

    nope, i just want to keep it in, why remove if you dont like it, some other people DO like it.
     
  15. Offline

    Afforess

    Did you just miss this entire paragraph of the OP?

     
  16. Offline

    gameswereus

    Question, so will the reload just take RAM until the next restart? Or will you have less and less RAM as you go?
     
  17. Offline

    losdamianos

    I cant really find the valid reason why you want so badly to remove that,
    I hope you consider that most of the server admins are capable of logical thinking and they will know that this is BAD command and they shouldn't really use it. I use it just to update some plugin configs in the evening where there is just too many people on the server to restart it and I can sacrifice that 5mb leak, late night or in the morning I can do a proper restart.
     
  18. Offline

    Kaikz

    That's the thing. Most server "admins" are not capable of logical thinking.
     
  19. Offline

    NinjaGrinch

    I honestly feel it should be removed. It causes nothing but issues when you have a fair bit of plugins since most of them just break.

    I stopped using it after the first few reloads back when I was still learning Bukkit. Realized it was causing many of the issues I had. :(
     
  20. Offline

    Daniel Heppner

    I think reload should be changed then toggleable. On my plugin test servers, I couldn't care less about ram usage, so I use the reload command a lot while testing my plugins. However for production servers, you wouldn't want to use it, so I think that the reload command should be changed to work better (Without leaking as much ram) then it should be toggleable for the server admins. (with a warning)
     
  21. Offline

    Nathan C

    Forget that.

    I would rather have memory leaks than having to shut down then restart my server when there is 20+ online. Not to mention I have to make TINY tweaks to the config files and I would have to restart the server just for 1 line change?

    Nah..
     
  22. Offline

    Acrobot

    I'd go with changing how plugins react.
    If in your plugin.yml you'll place "customReload: true", it should INSTEAD of disabling/enabling plugin execute onReload() method.
     
  23. Offline

    mindless728

    the reloads are at BEST a little reference leak, at worst you could have hundreds of megs of references leaking and/or plugins that are not being able to stop and start properly

    this is why it should be shut down, to avoid the crap that is most code that is done for plugins, there are some rare occasions where devs actually cleanup after themselves, but mostly its going to be leaky and broken
     
  24. Offline

    alta189

    /signed
     
  25. Offline

    ks07

    Glossing over things slightly a minute - but would it not be a good idea for someone (who knows what they're doing and talking about), to write some kind of reference in the development resources or on the wiki as to how to develop plugins that play nicely with /reload, resulting in no/as little as possible memory leak?
     
  26. Offline

    obnoxint

    It's not an allround-solution, but the best we can do at the moment.
     
  27. Offline

    mindless728

    cleaning up after yourself can be involved but as a developer you should know how to do it before even attempting to create minecraft plugins, there are plenty of tips in programming tutorials on how to clean your references so they don't become leaked
     
  28. Offline

    Celtic Minstrel

    The concept of what you need to do is pretty simple; make sure you acquire all your resources (opening a file, connecting to a database, etc) in onEnable(), and release them (closing the file, database connection, etc) in onDisable(). In practice it may not be that easy to implement, but as long as you try to follow those basic rules I think you would be able to minimize the leak. (Some people have been mentioning things to the effect that you should set class variables to null in onDisable(); this is just another way of releasing resources, though a less immediate way. I don't know if it's necessary; I haven't really researched how Java class loaders work.)

    I could handle the removal of the /reload command if there were tools for a developer to add it back in without ugly hacks, for example a Server.reload() method or a JavaPlugin.reload() method. I still would not really like it, though; I'd rather see the command's issues fixed than see it excised altogether. I don't feel that the complexity of cleaning up after yourself is a very good argument, either.
     
  29. Offline

    codename_B

    Even the ability to make plugins exempt from reloading would be something to think about.
     
  30. Offline

    ks07

    That sounds like a good compromise - /reload is far from useless, and any plugins that have good reason to not support reload could opt-out. A best of both worlds approach.
     
  31. Offline

    mindless728

    i feel as though it shouldn't be up to the programmer but up to the server owner, that way that person can disable reloading plugins that obviously don't work correctly

    as you could have a developer who doesn't know squat make a plugin that allows reloading that is completely broken undermining making it opt-in
     
Thread Status:
Not open for further replies.

Share This Page