AsyncPlayerChatEvent

Discussion in 'Plugin Development' started by chaseoes, Aug 3, 2012.

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

    turt2live

    Look into the conversation API? I know it's not quite on topic, but it's similar to what you are looking for.

    Found this at one point, sadly I can't source it :/
     
  2. Offline

    ZachBora

    It's because of the people on the irc that I used that. I was like "what does it return?" and they got me to return null. And when I asked can I use the return value they say it's going to slow down the process (which is true, but still...)

    Seriously it's horrible to have done it like this. "We changed the event, if you do it incorrectly it won't work. Oh btw people are going to bug you about this." And I'm still waiting for a post about this with examples of how to change our code.

    I looked at what some people did for 1.3 asyncchat and it didn't look safe. My other plugin (iChatPlayerList) doesn't look safe at all but it works with 2 people on.

    Yes, but there's a lot of plugins out there listening to the chat without changing it. I was suprised when I saw the number of plugins that listened to it (about 7) on my server when I thought only 2 did.
     
  3. Offline

    desht

    I have looked at the conversation API, but TBH it's overkill for what I need to do here - I just need to intercept & cancel a single chat message, perform a string substitution, and issue a command (either with Bukkit.dispatchCommand() or player.chat()). I think I know how to do this in a thread-safe manner now, but I'm still not 100% confident. Really hate being blind-sided by changes like this (yeah, yeah, follow Twitter, read IRC... no. I develop plugins in my spare time, for fun. I can't spend all my time reading Twitter or monitoring #bukkitdev).
     
  4. Offline

    turt2live

    True, I would probably grab your own plugin instance in the event. Your plugin would have the HashMap and a few methods to manage it. I'd call it 'safe' if you used the ConcurrentHashMap.
     
  5. Offline

    urndel

    Soo bukkit has this massive API and suddenly they decide to change 1 single yet essential bit to be multithreaded and leave everything else so you can't use the api at all?
    I mean what plugin doesn't use permissions in chat?
    Also what if a plugin relies on the position of the player?
    Ooh and 1 more question: Why does the bukkit team, after the big fight they waged against multithreading, suddenly decide to force this upon everybody in just 1 release? Couldn't they just introduced the events first and deprecate the old one later?
     
  6. Offline

    Superkabii

    Because the original chat event was causing massive performance issues. Any server with a chat plugin was facing tons of unwanted lag.

    And it's still the case with the deprecated event on a queue message system, just to a much less serious extent.
     
  7. Offline

    urndel

    But the chat worked? The big issue is the extreme speed at which this event is forced on the developers
     
  8. Offline

    ZachBora

    Actually the chat slowed down the rest of the servers and some people which used to run at ~20TPS dropped to 1-2.
    It would have been fine if the chat was as slow as before, but not in this case.
     
  9. Offline

    turt2live

    Bukkit is just an API for Minecraft's server.

    So Bukkit needs to match Minecraft as close as possible, and that means throwing Async chat on the table.

    There shouldn't be an argument about it. Developers should know that Bukkit simply does what Minecraft throws out, and makes an easier to use API for it. Because of this, developers should have known, after at least a MONTH, that 1.3 would have all sorts of new changes, like THREADED CHAT.

    If developers actually paid any sort of attention to the 1.3 release change log, they could have prepared for Bukkit to do something as large as Async chat.

    And not to mention that after the event change was released there was still days before the RB to fix your plugins up, and if you didn't know how the Bukkit team was in #bukkitdev on espernet helping people make thread-safe plugins.

    Still not convinced? Well, here's an example of what I did:

    I make a plugin called AntiShare, it managed Creative and Survival players. In 1.3 there was an added GameMode, once I heard, and confirmed, this change I went ahead and made some guesses as to how the API would change (like the obvious GameMode enum) and prepared COMMENTED code so I could uncomment and fix later when there was a dev build released. I did the preparation about 2-3 weeks ago.

    Please, stop complaining and ask for help from the Bukkit team in the IRC. Complaining they aren't helping developers is not going to get you anywhere, DO SOMETHING about it (like hop in the IRC).
     
  10. Offline

    urndel

    Where did I say that you aren't helping the developers? I complained about the way in which it is forced.
    I think that its to quick to start nagging and clogging up the console with messages about it. Yes you can deprecate stuff but you can't go and force it in one iteration. The warning on chat is just not necessary if the developers setup java to report the usage of deprecated stuff.
     
  11. Offline

    dreadiscool

    I mean at the time, AsyncPlayerChat was the best option, but what I feel that the solution here is that Bukkit should do away with Jeb's code about handling the chat. Before 1.3, chat packets were handled normally, they were added to a queue and handled when it was "their turn". But now, in 1.3, because Jeb changed his code to give chat packets priority (move them to the front of the line, ahead of other important things like world events and ticks), Bukkit has followed the same path.

    Yes, it makes chat faster, but all of a sudden, servers that were fast and could handle chat events fast have begun to lag horribly. I feel that Jeb can leave his server software the way it is, but Bukkit should do what is best for the plugins, which is follow the old method of handling chat events. Don't give them priority, and this entire problem is gone.

    Or am I just stupid and I misread something? :L
     
  12. Offline

    ZachBora

    Isn't Async handled by a seperate CPU core? I don't know much about this stuff, but I assumed the chat was running in parallel in a seperate core of your CPU, thus not getting in priority.
     
  13. Offline

    dreadiscool

    No, multiple threads can run on the same cpu (otherwise single cpu machines would be limited to one thread!). bukkit added the async chat event so that the chat event is handled outside the main thread. all of bukkit runs in one thread. however, the new chat implementation, which pushed chat packet events to the top of the 'event list' made the server lag. so bukkit added the asyncplayerchat event, which just moves the chat events to a new thread. this is fine and dandy, but now its virtually impossible to write thread-safe plugins that involve chat.
     
  14. Offline

    ZachBora

    Yes I understand all that but if you have multiple cores will it use the other ones.
     
  15. The problem is the documentation doesn't account for thread safety at all. Besides looking at the code, there is now way to know if any of the methods are or are not thread safe. For the most part, they aren't. This is a major problem as plenty of chat plugins (Including a private one that I develop) use things such as Permissions, Configuration, etc in the event, and now we have to find a way to run it on the main thread (The only way to my knowledge is to schedule a sync scheduler), which is a big pain. Until everything is documented/made thread safe (Which likely won't happen as that would require a major rewrite), it's going to be a...interesting time.
     
    desht likes this.
  16. Offline

    frelling

    Sleaker, I'm hard pressed to find how permission checks are not thread-safe? Maybe I'm not seeing the forest for the trees, but PermissibleBase.hasPermission() is very basic and I don't see any visibility or access problems. If you know different, I would certainly like to know. I will agree that setting permissions/attachments is not thread safe, give the liberal application of recalculatePermissions().
     
  17. Offline

    ZachBora

    We say it isn't because bukkit told us so. Are they wrong?
     
  18. Offline

    Sleaker

    the backing is a LinkedHashMap. If a permission update is happening when hasPermission is called it will not return a proper result and may cause a CME. Just because the check looks basic doesn't mean it's thread safe. If the variable you're checking could ever be written to from any other thread, and doesn't specifically have any controls on it's access, it's not thread safe.

    It's a lot easier to ask, What portion of the permission system actually is thread safe? If you see no controls anywhere, the simple answer is... None of it.
     
  19. Offline

    frelling

    Someone get me an axe so I can cut down this forest :) I had my blinders on too tight thinking that permissions could only change with player actions. I feel bad having wasted your time, but as always, your responses are appreciated. Its too bad that Mojang had to front-load the chat thread decoupling. It would have served better to back-load it after the event handler chain where performance impacts were felt.
     
  20. Offline

    dreadiscool

    You know what is needed? A library on top of Bukkit for making thread-safe calls ;s Who wants to get together and write one? We'd have to write a lot of messy code to work with Bukkit, but hopefully it helps people in the long run :)
     
  21. Offline

    desht

    Nice idea, but it would be equivalent to building a house on a bed of quicksand.
     
    wirher likes this.
  22. As far as i know reading position from player, reading permissions are SAFE!

    Why do i think so ...
    When i change some value, i creating new one on totally different space in memory and then change the "memory address" to that new data in memory.

    If bukkit calculates player position (x, y, z) and then replace Location obiect then it's safe!
    If chat event will read in the same time it will read totally good data before change or after change.

    The same for permissions, it should recalculate it in memory then replace, so the only problem is that you can get not "totally wrong data" but "older data", The only problem is how plugins are changing data, massive one change or "in time slow changes".

    So if you don't change anything in your plugin it's ok!
    If you change something, think 6times before doing that, that's all.

    I might be wrong, but the only problem with multithreading is "may contain older data" but if you need to get actual, then just sleep some server ticks. If you experiencing "wrong data" call the author that write code that changes them.

    http://stackoverflow.com/questions/...cation-exploit-a-multi-core-machine-very-well
    Yes, now chat and main server can use different cores, keep up that good work, make more events at different threads !
     
  23. Offline

    desht

    No. Read what Sleaker said above. The permissions system doesn't use thread-safe data structures internally, and nor does the configuration system. On top of that, there's no attempt at any kind of synchronization, and a member of the Bukkit dev team has explicitly stated that there are no plans to add any.

    Even if you don't try to change anything from the async thread, if you try to read something while another thread is writing something, the best you can hope for is a ConcurrentModificationException. At worst, you could end up with corrupted data.

    Meant to add to my previous post, but: the above statements are all, unfortunately, completely wrong.

    Had Bukkit actually made use of immutable objects in the first place, it would be well-placed to exploit a multi-threaded architecture. Regrettably, objects such as Location are mutable, and this cannot be changed without a major API break. Mutable objects don't make for good multi-threading.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 27, 2016
  24. ConcurrentModificationException - would be if you try to write not read as far i know english :)

    I'm not a guru but i know even changeing string in some class is a "create a new Class string, set text and replace reference".
    Yes, not every "change event" do that in 100% but the one and only place that sould do that is the person that changing things, and yes, in c++ and all other "low level" programs changing some variables could cause wrong data, that in java/objected oriented programs it is hard to get corrupted data unintentionally, that's why java is so slow ;/

    The the main problem is when you have class with 200 variables and you need to change 4 of them, then the second thread when reading something from that class of 200, it get a COPY of a memory where it could get 2 old and 2 new from that 4 changes, that's why example "Player location" using a class Location, and on location change you create new object, fill it, and make a single change in that "visible" class.
     
  25. Offline

    desht

    So what do you suppose happens if you perform a map.get() while another thread is already doing map.put() on the same HashMap? Go and have a think about it.

    (Yes, a ConcurrentHashMap is a thread-safe structure, but it's not a silver bullet - the object that uses it still needs to be aware of potential race conditions, deadlocking, etc.)

    In Java, String is an immutable object. "Changing" a String involves creating a new String object - the existing one doesn't get touched. That makes String a thread-safe class. Not all classes are like that and certainly not all of the classes in Bukkit.

    Seriously: you need to drop the notion that the Bukkit permissions and configuration API's are in any way thread-safe. They are not. The Bukkit devs themselves have admitted as much.
     
    Bone008 likes this.
  26. Offline

    dreadiscool

    Actually, there is a way within the bukkit api to make sync callbacks. So you could create a runnable from the chat thread, have it do some work, and pause until you get the callback. you may think "wait, pausing the chat event thread? not good!" but remember, all events were handled on the same thread before, so a callback shouldn't take much longer than doing the event itself.

    If you get what I mean, I realize I'm not being the best explainer here ;s

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 27, 2016
  27. Offline

    desht

    Yeah, I'm aware of callSyncMethod(), but that's hardly creating a new library on top of Bukkit - it's just using an existing Bukkit method and waiting for the response.
     
  28. Offline

    ZachBora

    A bukkit dev himself told me using callSyncMethod would slow down the chat as it will wait for a response.
     
  29. Offline

    dreadiscool

    oh crap, there is a callSyncMessage? ;o
     
  30. Offline

    Sleaker

    And guess what using a syncCallBack does to the system? It locks the entire main thread while your logic is run, so the whole point of having aSynch player chat is lost.
     
Thread Status:
Not open for further replies.

Share This Page