[Code Snippet] Workaround for the new Bukkit.getOnlinePlayers() method

Discussion in 'Plugin Development' started by caseif, Jun 29, 2014.

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

    caseif

    As you may or may not be aware, the return type for Bukkit.getOnlinePlayers() was changed in a recent commit from Player[] to Collection<? extends Player>. Unfortunately, this means that plugins which rely on class-specific methods from the two return types will only compile against the appropriate build, and plugins compiled with the new method will not run again servers running a version of CB older than the commit.

    However, I've written a code snippet that should allow you to compile and run again newer and older Bukkit/CB versions. I've used the .length/.size() methods as an example, as they were the ones that needed replacing in the Metrics.java class which this is derived from. (By the way, you'll need to fix it as well if you have it implemented in your plugin.)

    Code:
    int playersOnline = 0;
    try {
        if (Bukkit.class.getMethod("getOnlinePlayers", new Class<?>[0]).getReturnType() == Collection.class)
            playersOnline = ((Collection<?>)Bukkit.class.getMethod("getOnlinePlayers", new Class<?>[0]).invoke(null, new Object[0])).size();
        else
            playersOnline = ((Player[])Bukkit.class.getMethod("getOnlinePlayers", new Class<?>[0]).invoke(null, new Object[0])).length;
    }
    catch (NoSuchMethodException ex){} // can never happen
    catch (InvocationTargetException ex){} // can also never happen
    catch (IllegalAccessException ex){} // can still never happen
    This uses reflection to determine which class the method will return, and then acts accordingly. The .length/.size() methods can be replaced with any other class-specific method you wish. Keep in mind this is also extendable to allow for discrete blocks of code for each return type, rather than just a single variable assignment.

    Edit: formatting
     
  2. Offline

    Skyost

    Why ? :)
     
  3. Offline

    Conarnar

    Skyost
    https://bukkit.atlassian.net/browse/BUKKIT-5668
     
    Skyost likes this.
  4. Offline

    Zupsub

    Why did they use Collection<? extends Player> and not Collection<Player>?
     
  5. Offline

    Garris0n

    And yet they used a list?

    This is true of every single feature they add.

    I don't see why this is a big deal. People should keep their CB version up to date.
     
  6. Not all have the time to update every day/week. This means that a lot of outdated plugins that people use are going to break.
     
  7. Offline

    ZeusAllMighty11

    And plenty of kind developers updating them for free :)
     
    JBoss925 likes this.
  8. I'm talking about the ones from 3+ months ago that smaller servers use. :p
     
  9. Offline

    evilmidget38

    The API exposes a Collection, not a List.
    Outdated plugins will continue to function due to the use of Overmapped.

    Like Garris0n said, the only time plugins won't work is if you try to utilize the new method on old versions of Bukkit. This is true whenever we add new features or make changes, so I don't see how this is an issue or requires a supposed "workaround". If you want compatibility with old versions of Bukkit, then build against old versions of Bukkit.
     
    drtshock and Garris0n like this.
  10. Offline

    Garris0n

    They have time to update plugins but not the server?
     
  11. Offline

    RawCode

    absolutely completely useless, everything added or changed by bukkit team not reverse compatible, no exceptions.
    as for implementation itself - are you kidding? - 5 reflection invocations on 3 lines without any caching or prefetch.

    how it shoud be done:

    class BaseVersionDependantFetcher{methods for default version or marked abstract} implements IFetcher
    class Version89112Fetcher{methods for version 89112} extends BaseVersionDependantFetcher (compiled versus given version)
    class Version81999Fetcher{methods for version 81999} extends BaseVersionDependantFetcher (compiled versus given version separately from rest of project or part of individual project, linked magically by replacing stub classes after compilation)
    (fetchers shoud contain all version specific code, not just single method, if version have nothing special about method - just dont override method inside version fetcher)

    interface IFetcher{signatures for methods at play}

    class VersionDependantFactory{

    public IFetcher AssembleFetcherFor(String BukkitVersion)
    {
    cases for each version with something like
    return new VersionXFetcher()
    as final call
    }

    After that you can:
    IFetcher invocator = VersionDependantFactory.AssembleFetcherFor(Bukkit.getVersion());
    invocator.getsomethingorinvokesomething(params)

    or IFetcher invocator = Class.forName("blabla" + Bukkit.getVersion());
    invocator.staticmethodsaboutstuff();

    this will feature unlimited scalability and zero reflection.
    with a bit more magic you can store versionspecific fetchers on remote site and download them on demand, this will allow to fix predictable compatability issues in plugins already at play.

    such solution will be OK as guide for other users, but current code is very bad coding practice.

    also java itself is not reverse compatible - you can run java 1 code on JRE 10 but not java 10 code on JRE 1.

    as for array vs collection - this is oracle coding guide, you should never return array or null - collection or empty collection only acceptable return.
     
  12. Offline

    caseif

    This particular method is somewhat of a big deal, however, as it breaks the metrics class found in countless plugins. This code allows developers to continue supporting metrics while still compiling against the newest Bukkit versions. Regarding new methods never being reverse compatible, while that's true, this isn't effectively a new method. It's a modification of an existing method, meaning that developers using the old one don't have a choice but to use it.

    That's not quite what the issue with this method is. If a developer compiles their plugin to use the new method, it instantly becomes incompatible with any servers running a CB build older than 3096 (i.e. the latest one). This code block aims to avoid that scenario.

    I was just providing the code in case someone found it useful; no need for the hostility. The code is efficient and reliable, despite whether or not it's considered "bad practice." And regarding JREs not being reverse compatible: that's okay when a new JRE is released one every couple of years, but not when a new Bukkit build is release every month/day (depending on whether you count dev builds).
     
  13. Offline

    RawCode

    ShadyPotato
    Tell me date of last stable cbukkit release.
    Tell me current date.

    every month\day update eh?
     
    Garris0n likes this.
  14. Offline

    Garris0n

    If a developer compiles their plugin to use any method added to CraftBukkit, it instantly becomes incompatible with servers running an older version.

    Not too long ago, these methods were added. I don't see any reflection to only use them if the version allows it. Do you plan to do this for every single method ever added to CraftBukkit? Because that sounds rather ambitious.
     
    mazentheamazin, RawCode and AdamQpzm like this.
  15. Garris0n Pfft, who cares if a bat's awake or not.
     
  16. Offline

    Garris0n

    Santa Claus.
     
    Cirno, caseif, Gamecube762 and 3 others like this.
  17. Offline

    RawCode

    Methods added nearly every build, adding reflection to provide useless reverse compatability for each?

    It very very discussional to keep backward compatability for ages, some code just must die over time, reverse compatability IRL was implemented only few times - by microsoft developers who reimplement winapi bugs intentionally and wine developers who intentionally left winapi bugs.
     
  18. Offline

    Wolvereness Bukkit Team Member

    I made this change to the return type of getOnlinePlayers. I really wish horrendous advice like what ShadyPotato just posted would stay off the forums - it disappoints me so much, exploiting people's ignorance and impatience.
    • Stop using reflection. Seriously, stop doing it. Any code that uses it to get around compilation issues needs to be burned in a fire.
      • It adds unnecessary JVM overhead.
      • It usually doesn't even accomplish what the user wants it to accomplish (like the package versioning workarounds - people end up recompiling and editing their code anyway, making the only benefit a sense of self-satisfaction).
      • It obfuscates the purpose of code.
      • It reduces the ability to debug.
    • If you want to be compatible with old versions of CraftBukkit, compile against an old version of Bukkit.
      • The change was intentionally not marked as [BREAKING]
      • Newer versions of CraftBukkit work perfectly fine with the plugins build against old versions.
      • If you need a method that doesn't exist in an old version, your plugin doesn't need to worry about getOnlinePlayers to begin with - the server will have the new version anyway.
    • Supporting old version of CraftBukkit degrades the quality of the community.
      • It unnecessarily limits the propagation of critical fixes in the newest versions.
      • It adds unnecessary development overhead. Coders will build these up these 'workarounds' over time, never checking to see if they're necessary much less purging the old unused code.
    • Most importantly, it encourages bad practices as a positive feedback loop. Many people first learning to code get exposed to Bukkit as their first experience.
      • Every time this type of advice gets posted and propagated, it encourages poor conventions.
        • As users start using poor conventions, their problems become unnecessarily difficult to troubleshoot and provide help.
        • It changes the expectation for coding solutions; people end up wanting the bad solutions without any regard for the technical debt.
        • The view that this advice is appropriate makes future advice givers change their view on what to provide.
      • Users don't understand the underlying reasons - they don't learn to learn about problems.
        • Especially for NoSuchMethodError; I have a copy+pasta I use to help developers, with consistently positive responses. Users who read it are exposed to the key pieces of information they need to understand the problem(s) and prevent it in the future.
        • Your post provides absolutely no information or any learning points. Or, to rephrase, there is absolutely no elements of your post that positively affect the community.
        • There is a very clear distinction between explaining a problem followed by explaining the solution, and providing the exact solution. What you posted can't even be considered a solution; it's a cleaver to prevent gangrene when they only have a splinter.
      • This kind of post dismisses any legitimate concerns that users may have.
    While it may not be as grotesque as @RawCode's asynchronous suggestions, I almost wish I had the power to compel the removal of this type of content. Excusing the digression, I do wish to implore a more responsible approach from you ShadyPotato.
     
  19. Offline

    caseif


    The last beta was released about a month ago. Not quite stable, but close enough.


    The difference is, this commit also removes a method, making it impossible to compile plugins with new Bukkit builds without breaking reverse compatibility.

    It's necessary to provide backwards compatibility for a time, at least until a significant portion of the server base stops using 1.7.


    This is all true (sans the bit about it usually not working), but it's insignificant for such a small piece of code.

    If a dev wants to implement a 1.8 features (obviously with a fallback in this case, which isn't at all implausible), why make them drop support for 1.7 entirely just for a single method? This is meant as a temporary workaround until the 1.7 community wanes to a point where dropping support is acceptable.


    This can be true, but in this case, retaining support for the latest two versions isn't going to kill the community. I'm hoping that developers will have the sense to entirely transition to the new method come MC 1.9.


    Okay, fair point. I personally like reflection, but I can see where you're coming from here.



    I don't think this post was irresponsible, though I do now see your concern in terms of proliferating poor practices. That said, it's not a bad idea to support the next-latest Minecraft version, and there's no reason to break that support over a single method. I'm relying on the idea that most developers will take this advice with a grain of salt and use common sense when implementing and maintaining it.
     
  20. Offline

    RawCode

    Wolvereness
    My suggestion of doing something before asking is perfectly fine and follow generic rules of internet...
    Forums like stackoverflow or xda-developers "ban" users who ask without providing proof of work, but for some reason bukkit tradition is spoonfeeding...

    ShadyPotato
    Last stable recommended version was 8 month ago.
    If some server administrator mindlessly updates - its his own issue.
    Recommendation to keep reverse compatability with obsolete versions is joke...
     
  21. Offline

    desht

    Not when it comes to experimenting with non-thread-safe methods, it isn't. As has been pointed out more than once, making Bukkit (or OBC or NMS) calls from a different thread has a very good chance of appearing to work fine when you try it out on a quiet development server. And a very good chance of breaking badly when subsequently deployed on a busy production server.
    Oh, please. Stop flaming for the sake of flaming. Telling users not to make Bukkit calls from a different thread is not spoonfeeding, and there's certainly no tradition of spoonfeeding going on here.
     
    Wingzzz, Cirno, caseif and 2 others like this.
  22. Offline

    RawCode

    desht
    woot?
    who asked to use such code for plugin?

    i asked user to actually perform some work self, is this flaming or bad practice?
     
  23. Offline

    xize

    hence I don't understand the ticket... why was this needed to be changed just because players may not be called Player[0] ? or because serious issues with iteration?:p
     
  24. Offline

    caseif

    From what I understand, it's more or less for the sake of Java conventions. I personally don't think the change is justifiable, but I understand why it was made.
     
    xize likes this.
  25. Offline

    1Rogue

    collections > arrays in almost every situation.

    Even for java 8, now you can do something like this:

    Code:java
    1. Bukkit.getOnlinePlayers().stream().filter(SomeClass::somePredicate).forEach(SomeClass::someMethod);


    versus manual iteration
     
    Cirno, xize and caseif like this.
  26. Offline

    xize

    ShadyPotato
    well I agree about that though, it may be safer to iterate if the orginal player list gets cloned but again Player[0] needs now to be be called like .get(0) which makes it actually a bit worse/redurant if the tickets intention was only for the purpose to remove indexes and not for (never saw one in getOnlinePlayers()) ConcurentModifications in case its still cloning I don't see why it should be justified to if it only was counted on removing indexes because its a convention, cus thats how literly I interpret it reading the ticket, but though I don't like the compatability issues it brings:p
     
  27. Offline

    Zupsub

    But, when do you need the first or second or nth player object?
    In almost all cases you have to loop threw all players and maybe filter them by name / uuid / what ever.
     
  28. Offline

    Wolvereness Bukkit Team Member

    There is no first player though... As far as how Minecraft is designed, the actual online players is closer in semantics to a Set - only one instance of a player can be logged in at any time, and order is irrelevant.
     
  29. Offline

    1Rogue

    Code:java
    1. Bukkit.getOnlinePlayers().iterator().next();


    Obviously can be just as unsafe as getOnlinePlayers()[0] if you have no one online, but you can do pretty much anything with a collection that you can with an array.
     
    Garris0n likes this.
Thread Status:
Not open for further replies.

Share This Page