Problem/Bug Common mistakes thread in plugin developmemt extremely outdated

Discussion in 'Bukkit Help' started by RcExtract, Jun 11, 2018.

  1. Offline

    RcExtract

    https://bukkit.org/threads/common-mistakes.100544/

    In the thread it is calling us to code a plugin based on Java 6. However, even Java 7 reaches eol (end of life) and Java 10 is released for awhile already. Someone please update it to recommending programmers to code on Java 8+.

    EDIT: or can i make a new one?
     
  2. Offline

    timtower Ninja on the waves Moderator

    @RcExtract Might need a new one instead.
    And not every server runs java 9, so 8 is probably the highest you can go right now.
     
  3. Offline

    RcExtract

    Post it right in plugin development forum?
     
  4. Offline

    timtower Ninja on the waves Moderator

    Might be better to post it as staff so we have less issues when editing it.
    And maybe post it here first to check if it is correct etc.
     
  5. Offline

    RcExtract

    (Since the old one is archived, a new one must be created.)
    ____________________________________________________

    Common Mistakes while coding a Bukkit plugin

    In this article I hope to cover a bunch of mistakes that I see frequently either in the Bukkit Plugin Development forum, the #bukkitdev IRC channel, in plugins that mbaxter help folks write in other places on the net. All of these issues or mistakes can occur on a plugin that compiles fine.

    Building with latest java version - 2
    While technically not a problem for your own computer, you need to keep in mind that plenty of Bukkit servers are running Java 7. However, Java 7 has reached EOL (end of life). There will be no more public updates, which is a very bad idea. Therefore, the server owners will update their Java, and you should also compile your code on Java 8 or higher versions. Note that the information above is probably only applicable to 2018. For anytime measures, you should compile your code with the latest version minus 2 of Java.

    Denying Console from using commands that it could otherwise use
    I see a lot of plugins that, for commands not affecting the sender, deny anyone that isn’t a Player from using them. If your command can operate with console using it, you should allow it for normal situation. Checking for instanceof Player prevents execution from console as well as other situations such as IRC plugins executing commands remotely.

    Not registering events or commands
    You’ve created your amazing event listener full of methods for catching all sorts of events. But none of them are firing! You created an onCommand method or a couple CommandExecutors for your commands but they aren’t triggering! Looks like you forgot to register your events and commands. Event registration is documented here, while command registration is documented here. Also, don't forget to register commands in your plugin.yml.

    Putting other plugins classes (or entire plugins) in your jar
    I’ve seen several developers, in the hope of ensuring their users also get the plugin theirs uses, include plugins or pieces of plugins in their code. First, this is often a license violation. More importantly this can breakthat other plugin and is a bad practice. Only include your own code (or code used with permission under license) in your plugins. If you are using maven, make sure to set the scope of all dependencies to "provided" if you are not sure whether you can add it into ur jar.

    Including CraftBukkit in your plugin
    This is certainly an impressive feat, but I’ve seen it in probably half a dozen submissions. Don’t include CraftBukkit (or Bukkit) in your plugin, the Bukkit server jar already has that.

    Importing individual permissions or chat plugins for chat formatting (prefix/suffix)
    You create your awesome chat plugin that needs to get prefix/suffix data from other plugins. You release it and it works fine for 5 months. In those 5 months a brand new awesome prefix plugin comes out and you have to now edit in support, or one of the plugins you supported completely changes its internals. Your plugin needs constant updates to keep track of all the other plugins you’re checking just for some simple data they all provide. The plugin Vault was created precisely to avoid this scenario. Vault handles keeping track of all these systems for you and is an easy dependency to have your users install. They just drop it in and go. Done! On your end, you now have a single plugin to support whose API isn’t going to change.

    Using Vault to check if a Player has a permission
    On the opposite side of the Vault issue, is people who believe that using Vault to check a permission is the ideal way to go. I’ve seen plugins require Vault just for checking has(Player,permission). Fun fact: Vault’s permission check just calls Bukkit’s methods. There is zero benefit in using Vault to check a permission. Just call player.hasPermission(permission) and your life is far easier!

    Using non-threadsafe methods in Async events (AsyncPlayerPreloginEvent, AsyncPlayerChatEvent) or Async tasks (scheduler)
    Async events are not running as part of the minecraft thread. A good way of judging if you should be calling a method from an async event is asking yourself “does this method change or query something in-game?” If the answer is yes, you probably shouldn’t be doing it. Permission checks are not safe, as are many other methods. Pretty much, if you’re not certain, don’t.
    Examples of valid ways to deal with various async features:
    • AsyncChat: Format the chat with your formatter. Use cached information about the player to format the chat.
    • AsyncPrelogin: Query an external database (async is perfect for this, the delay in reply won’t lag the server) to determine if that username or IP can join.
    • Async task: Update a MySQL database with new data.

    IO operations (writing non-config files, MySQL queries, metrics, update checks, etc) on the main thread
    IO calls can take a long time. Running your MySQL query or update check from the main thread can very quickly slow down the server in a highly noticeable way. Instead, schedule these actions to run in an async task. Need to do something that isn't thread-safe (see above) as a result of an IO operation? Schedule a sync task from that async task when it's time to react!

    Utilizing static variables for information sharing
    There are many reasons this is generally a poor decision. Here is a great description of some reasons: http://stackoverflow.com/questions/7026507/why-are-static-variables-considered-evil/7084473#7084473
    Storing constants statically is great. Storing temporary information is less so.

    For main plugin class, you should also keep the fields non-static, and if exists, call the API users to obtain your main plugin class with Bukkit.getPluginManager().getPlugin(String) or provide a static method which returns the main plugin class instance.

    Storing Player objects
    You should store Player objects, but they will cause memory leaks when the player representing leaves. Therefore, make sure to un-store them when they leave. You can handle player leaving with PlayerQuitEvent. Storing UUIDs are not recommended because it only provides coding convenience, but not performance, because Bukkit.getPlayer(UUID) is an expensive method and will slow down the performance. You may also consider using WeakHashMap, which a key along with the corresponding entry will be removed when the key is no longer in ordinary use.

    plugin.yml Dependencies and Soft Dependencies
    For "depend" and "soft-depend" properties in plugin.yml, they are String arrays. Make sure to surround the values with [] and separate values with commas.

    @EventHandler Annootation
    Listener is a marker interface, and Bukkit does not know which method within your Listener is an event handling method. So make sure to add the EventHandler annotation. Furthermore, you can add extra attributes: priority and ignoreCancelled.

    Don't log for plugin enabling or disabling
    The server logs for you, so you don't need to log them for yourself. It does not cause performance issues, but the plugin users may not understand why the plugin enables and disables for "twice", and may cause other confusions.

    Using location.distance() inside a loop
    Commonly seen in code that draws circles, spheres etc. - using getDistance() to check if a block is inside a radius makes a call to Math.sqrt(), which is not cheap, and really inefficient if done inside a loop. Instead, calculate the square of your desired distance outside your loop, and using location.distanceSquared().

    onCommand vs PlayerCommandPreprocessEvent
    onCommand receives command execution from CommandSender, but PlayerCommandPreprocessEvent only receives command execution from Player. Also, PlayerCommandPreprocessEvent is fired before Bukkit checking whether the command really exists. So take good use of them.

    Do not use Thread.sleep()
    Executing Thread.sleep() freezes the entire thread, and you should never do this in the Bukkit server thread.

    Using tabs for indentation in yaml configuration files instead of spaces
    Yaml DOES NOT allow this as part of it's specification, and it will throw errors if you do this.
    Double/quad space indent only.
    This can cause your plugin to not load it's config.yml or worse, not load and spam console because of tabs in plugin.yml

    You are welcome to comment below and even suggest additions.
     
  6. Offline

    timtower Ninja on the waves Moderator

    @RcExtract This one conflicts with the original one though.
     
  7. Offline

    RcExtract

    I thought that u are going to remove the old one lol, but after thinking it seems that the old one cannot be removed due to archived. should I only talk about use Java 8, or Java 8 with additional common mistakes that are not in the old one?
     
  8. Offline

    timtower Ninja on the waves Moderator

    I want to unsticky the old one and sticky a new one. But I don't want them to conflict with each other.
     
  9. Offline

    RcExtract

    So do u mean u don't want me to copy contents from the old one? But they are also common mistakes too. If u unsticky the old one, and I don't copy some from the old one , people may not know about the mistakes.

    我從使用 Tapatalk 的 LG-H815 發送
     
  10. Offline

    timtower Ninja on the waves Moderator

    @RcExtract You are free to copy from it. But right now you have an item about UUID's that goes ahainst mbaxters version.
     
  11. Offline

    RcExtract

    What do u mean?
    Do u mean that I recommended to store player instances instead of UUIDs, which is different from mbaxters version? But his version is old. Also I heard a decent amount of message that storing UUIDs isn't as good as u think.

    我從使用 Tapatalk 的 LG-H815 發送
     
  12. Offline

    timtower Ninja on the waves Moderator

    @RcExtract And I heard that UUID's are better. Love programming.
    Probably deoends from what point of view you look. Memory issues vs speed issues.
     
  13. Offline

    RcExtract

    Does WeakHashMap solve both problem? You dont need to invoke Bukkit.getPlayer everytime, but once there is no more reference of the player object in other places, it will be removed from the map.
     
  14. Offline

    timtower Ninja on the waves Moderator

    @RcExtract Don't know. It depends on what you prefer to look after.
    Speed in Bukkit isn't a big concern in my opinion as the tick rate is limited anyways.
     
  15. Offline

    RcExtract

  16. @RcExtract
    Theoretically, a WeakHashMap solves the problem but it's totally unnecessary.

    The point about Bukkit.getPlayer(UUID) is total nonsense. It's literally just a look-up in a Map (so storing Player instances would be no different performance-wise). The whole idea with storing UUIDs instead of player instances is so that you don't have to worry about even accidentally causing a memory leak. You shouldn't store player instances unless you have a very good reason to.
     
  17. Offline

    RcExtract

    Instead of choosing one, we should provide both options, one with higher performance, another more convenience. :)

    Common Mistakes while coding a Bukkit plugin

    In this article I hope to cover a bunch of mistakes that I see frequently either in the Bukkit Plugin Development forum, the #bukkitdev IRC channel, in plugins that mbaxter help folks write in other places on the net. All of these issues or mistakes can occur on a plugin that compiles fine.

    Building with latest java version - 2
    While technically not a problem for your own computer, you need to keep in mind that plenty of Bukkit servers are running Java 7. However, Java 7 has reached EOL (end of life). There will be no more public updates, which is a very bad idea. Therefore, the server owners will update their Java, and you should also compile your code on Java 8 or higher versions. Note that the information above is probably only applicable to 2018. For anytime measures, you should compile your code with the latest version minus 2 of Java.

    Denying Console from using commands that it could otherwise use
    I see a lot of plugins that, for commands not affecting the sender, deny anyone that isn’t a Player from using them. If your command can operate with console using it, you should allow it for normal situation. Checking for instanceof Player prevents execution from console as well as other situations such as IRC plugins executing commands remotely.

    Not registering events or commands
    You’ve created your amazing event listener full of methods for catching all sorts of events. But none of them are firing! You created an onCommand method or a couple CommandExecutors for your commands but they aren’t triggering! Looks like you forgot to register your events and commands. Event registration is documented here, while command registration is documented here. Also, don't forget to register commands in your plugin.yml.

    Putting other plugins classes (or entire plugins) in your jar
    I’ve seen several developers, in the hope of ensuring their users also get the plugin theirs uses, include plugins or pieces of plugins in their code. First, this is often a license violation. More importantly this can breakthat other plugin and is a bad practice. Only include your own code (or code used with permission under license) in your plugins.

    Including CraftBukkit in your plugin
    This is certainly an impressive feat, but I’ve seen it in probably half a dozen submissions. Don’t include CraftBukkit (or Bukkit) in your plugin, the Bukkit server jar already has that.

    Importing individual permissions or chat plugins for chat formatting (prefix/suffix)
    You create your awesome chat plugin that needs to get prefix/suffix data from other plugins. You release it and it works fine for 5 months. In those 5 months a brand new awesome prefix plugin comes out and you have to now edit in support, or one of the plugins you supported completely changes its internals. Your plugin needs constant updates to keep track of all the other plugins you’re checking just for some simple data they all provide. The plugin Vault was created precisely to avoid this scenario. Vault handles keeping track of all these systems for you and is an easy dependency to have your users install. They just drop it in and go. Done! On your end, you now have a single plugin to support whose API isn’t going to change.

    Using Vault to check if a Player has a permission
    On the opposite side of the Vault issue, is people who believe that using Vault to check a permission is the ideal way to go. I’ve seen plugins require Vault just for checking has(Player,permission). Fun fact: Vault’s permission check just calls Bukkit’s methods. There is zero benefit in using Vault to check a permission. Just call player.hasPermission(permission) and your life is far easier!

    Using non-threadsafe methods in Async events (AsyncPlayerPreloginEvent, AsyncPlayerChatEvent) or Async tasks (scheduler)
    Async events are not running as part of the minecraft thread. A good way of judging if you should be calling a method from an async event is asking yourself “does this method change or query something in-game?” If the answer is yes, you probably shouldn’t be doing it. Permission checks are not safe, as are many other methods. Pretty much, if you’re not certain, don’t.
    Examples of valid ways to deal with various async features:
    • AsyncChat: Format the chat with your formatter. Use cached information about the player to format the chat.
    • AsyncPrelogin: Query an external database (async is perfect for this, the delay in reply won’t lag the server) to determine if that username or IP can join.
    • Async task: Update a MySQL database with new data.

    IO operations (writing non-config files, MySQL queries, metrics, update checks, etc) on the main thread
    IO calls can take a long time. Running your MySQL query or update check from the main thread can very quickly slow down the server in a highly noticeable way. Instead, schedule these actions to run in an async task. Need to do something that isn't thread-safe (see above) as a result of an IO operation? Schedule a sync task from that async task when it's time to react!

    Utilizing static variables for information sharing
    There are many reasons this is generally a poor decision. Here is a great description of some reasons: http://stackoverflow.com/questions/7026507/why-are-static-variables-considered-evil/7084473#7084473
    Storing constants statically is great. Storing temporary information is less so.

    Storing Player objects
    You should not store Player objects when they are offline, since they are intended to be garbage collected, and storing them will cause memory leaks. You can choose one out of three options below:
    1. Store their UUIDs. This provides a lot of convenience.
    2. Store Player objects, but set the variables to null when they leave, detected with PlayerQuitEvent
    3. Store Player objects within a WeakHashMap. I personally think its the best solution, since it weak references Player objects and will be removed when there are no strong references to them. If you need a "WeakHashSet", simply do Collections.newSetFromMap(weakHashMap).

    plugin.yml Dependencies and Soft Dependencies
    For "depend" and "soft-depend" properties in plugin.yml, they are String arrays. Make sure to surround the values with [] and separate values with commas.

    @EventHandler Annootation
    Listener is a marker interface, and Bukkit does not know which method within your Listener is an event handling method. So make sure to add the EventHandler annotation. Furthermore, you can add extra attributes: priority and ignoreCancelled.

    Don't log for plugin enabling or disabling
    The server logs for you, so you don't need to log them for yourself. It does not cause performance issues, but the plugin users may not understand why the plugin enables and disables for "twice", and may cause other confusions.

    Using location.distance() inside a loop
    Commonly seen in code that draws circles, spheres etc. - using getDistance() to check if a block is inside a radius makes a call to Math.sqrt(), which is not cheap, and really inefficient if done inside a loop. Instead, calculate the square of your desired distance outside your loop, and using location.distanceSquared().

    onCommand vs PlayerCommandPreprocessEvent
    onCommand receives command execution from CommandSender, but PlayerCommandPreprocessEvent only receives command execution from Player. Also, PlayerCommandPreprocessEvent is fired before Bukkit checking whether the command really exists. So take good use of them.

    Do not use Thread.sleep()
    Executing Thread.sleep() freezes the entire thread, and you should never do this in the Bukkit server thread.

    Using tabs for indentation in yaml configuration files instead of spaces
    Yaml DOES NOT allow this as part of it's specification, and it will throw errors if you do this.
    Double/quad space indent only.
    This can cause your plugin to not load it's config.yml or worse, not load and spam console because of tabs in plugin.yml

    Compile Maven artifacts with Maven Compiler Plugin
    If you are using Maven, compile your plugin with Maven Compiler Plugin. Using the export option may create incorrectly compiled jars.

    Do not start a new thread with new Thread(Runnable).start()
    Use the BukkitScheduler instead. If you start a new thread with the Thread class, Bukkit cannot monitor it, and may cause issues.

    Do not use existing package names
    Use your own package name. Avoid classes with same name and same package name, or conflicts will occur, and you will have a hard time to solve this issue with custom class loader.

    You are welcome to comment below and even suggest additions.
     
  18. @RcExtract
    But the fact that storing Player instances in a HashMap would be more efficient than UUIDs is total crap, regardless of whether they are online or not.

    You should always go for storing UUIDs since there is no performance difference, and the risk of memory leaks by storing Player instances is eliminated. Just store UUIDs. Period.

    I don't mean to be rude, but I don't think you should be editing this list. Nearly all points you made are wrong on some level or confusing at best.
     
  19. Offline

    RcExtract

    Doesnt have to be weakhashmap. WeakReferences also work for storing a single Player. Storing UUIDs in hash maps (or hash sets) should have similar speed with storing Players in weak hash maps (or weak hash sets).

    I can do some experiments if u want (i want too) but im not at home.

    EDIT: apparently both constructing or using the put method on a hashmap is faster than a weakhashmap about 2000 nano seconds. But havent measured the time cost for Bukkit.getPlayer
     
    Last edited: Aug 9, 2018 at 9:48 AM
  20. @RcExtract
    Actually, that's not true. WeakHashMaps and WeakHashSets are generally slower than a regular HashMap, which is yet another reason to not go with your recommendations.

    There is a reason people go on about storing UUIDs instead of Player instances. Who conjured you into thinking that it would be less efficient?
     
  21. Offline

    RcExtract

    Havent measured the time cost for Bukkit.getPlayer. U construct and put hashmap faster. But what about obtaining the player object with Bukkit.getPlayer? (i will measure it soon)

    A lot of people suggest to store Players using weak references, cant list them all.
     
  22. @RcExtract
    I haven't measured it, no, but even if a WeakHashMap turns out to be marginally better, that difference will be so insignificant that you should not under any circumstances worry about it.

    Why the hell do you think that Bukkit.getPlayer(UUID) is slow? It literally calls a method which fetches the player instance from a Map<UUID, Player>. The performance overhead of that is basically non-existent.
     
  23. Offline

    RcExtract

    They why u worry about this? :)
     
  24. @RcExtract
    Because you are making up non-existent reasons for people to go out of their way to store Player instances in Maps when they clearly shouldn't.

    When the reason for storing Player objects is total garbage, you (or someone else) is bound to get it wrong, and that's the whole point of storing UUIDs! So that there is no risk of getting it wrong!
     
  25. Offline

    RcExtract

    failure is the mother of success. I said people can use weak reference if they only store 1 player, not only with WeakHashMap. Also chill.

    6 tests below are all done after the required amount of players have joined for 1 minute.

    I have run three tests testing the speed of storing a player instance, obtaining the player instance, and removing the player instance reference when offline, which is not manually required in test 2 and 3.

    Test 1

    This test stores a player instance, and prevents memory leak by assigning the player variable to null when PlayerQuitEvent is called. To obtain the player, simply reference to the player variable. The result is between 247775 and 354006 nano seconds in 10 test runs.

    Test 2

    This test stores a UUID, and always does not have memory leak. To obtain the player, do Bukkit.getPlayer(uuid) where uuid is the variable storing the UUID. The result with 1 player is between 10281 and 20562 nano seconds in 10 test runs. The result with 2 players is between 20048 and 43695 nano seconds in 10 test runs.

    Test 3

    This test stores a WeakReference of a player, and always does not have memory leak. To obtain the player, do WeakReference<Player>.get(). The result is between 4112 and 7711 nano seconds in 10 test runs.

    Result

    WeakReference wins. So please use WeakReference when storing Player instances instead of UUIDs.

    I have run another three tests testing the speed of constructing player sets, storing some player instances, and removing offline Player instances, which is not manually required in test 2 and 3.

    Test 1

    This test stores player instances with HashSet, and prevents memory leak by removing the player from the set when PlayerQuitEvent is called. The result with 1 player is between 200482 and 242634 nano seconds in 10 test runs. The result with 2 players is between 249317 and 498120 nano seconds in 10 test runs.

    Test 2

    This test stores UUIDs with HashSet, and always does not have memory leak. To obtain a player, do Bukkit.getPlayer(uuid) where uuid is one of the UUIDs stored. The result with 1 player is between 42152 and 70426. The result with 2 players is between 49863 and 79678 nano seconds in 10 test runs.

    Test 3

    This test stores player instances with a Set constructed from WeakHashMap using Collections.newSetFromMap, and always does not have memory leak. The result with 1 player is between 58088 and 79164 nano seconds in 10 test runs. The result with 2 players is between 73510 and 92531 nano seconds in 10 test runs.

    Set<UUID> wins when the total players amount is less than 9. But note that when there are more than 9 players, A Set constructed from WeakHashMap using Collections.newSetFromMap is faster.

    Conclusion
    For storing a single player instance, WeakReference always wins. For storing multiple players, it depends on how much players you expect to have on your server. If less than 9, Set<UUID> is preferred, otherwise a Set constructed from WeakHashMap using Collections.newSetFromMap is preferred.
     
  26. Offline

    timtower Ninja on the waves Moderator

    @RcExtract Most developers on here are new to Java though.
    How will you get them to use WeakReference<Player> instead of the simpler Player?
     
  27. Offline

    RcExtract

    new WeakReference<Player>(player) to make a weak reference to player.
    WeakReference<Player>.get() to get the referencing object.
    @timtower
     
    Last edited: Aug 13, 2018 at 3:37 AM
  28. @RcExtract
    The data you provided means nothing - 10 tests is not nearly enough to draw any kind of remotely useful conclusion. The System.nanoTime() time is not guaranteed to even have a greater refreshing interval than System.currentTimeMillis(), meaning that the values you got are several times smaller than the error margin of the measurement. If you ran the test a couple tens of thousands of times, I might be more inclined to believe the figures.

    But that is not the point - any eventual differences in execution time will always always always be too small too matter in a game running at 20 ticks per second with at the very very most a couple thousand players online. The point is that you should not have to create some extra code just to make sure you won't cause a memory leak.

    That's what storing UUIDs are good for. It's simple, there is no risk of causing memory leaks (even accidentally) and you do not have to create some quite complex code (with catastrophic consequences if you get it wrong) just to do the simplest of tasks.

    I don't think I will ever be able to convince you to just use UUIDs. Fine. But I refuse to let you edit the guidelines for newcomers to say that they should do something ridiculous in the name ofperformance benefits (questionable if they even are benefits) which don't matter what so ever.
     
    timtower likes this.
  29. Offline

    RcExtract

    I think i can never convince u to use weak reference. The tests showed that overall performance of weak reference is better than UUIDs, and u still dont believe? Also, how complex is weak reference? Its just a simple new WeakReference(object) and WeakReference.get(). If you have more people on the server, Bukkit.getPlayer will become more expensive, but weak reference wont.
     
  30. Offline

    timtower Ninja on the waves Moderator

    @RcExtract How lomg do you use a WeakReference then? As it is only 1 player.
    And does Bukki.getPlayer get called often enough to produce lag?
    And what method do you call to get a player?
     

Share This Page