Getting your priorities straight: The plugin version

Discussion in 'Plugin Development' started by Dinnerbone, Jan 16, 2011.

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

    mattmoss

    Regarding my previous post and the points raised particularly by @Eris, I am considering handling events that want to change state, yet respect the final status of the cancelled flag, in a manner similar to this:

    Code:
    // Does not assume any priority level.
    public void onBlockDamage(final BlockDamageEvent event) {
        // Re/set cancelled flag as appropriate.
        plugin.getServer().getScheduler().scheduleSyncDelayedTask(plugin, new Runnable() {
            public void run() {
                if (!event.isCancelled()) {
                    // do block state changes here
                }
            }
        });
    }
    
    In this way, I only register one event, always get the latest cancelled state, keep the work near the event handler, and am thread-safe. I can register the event with a NORMAL priority (or whatever is appropriate), or possibly use MONITOR and do it this way:

    Code:
    // Assumes MONITOR priority.
    public void onBlockDamage(final BlockDamageEvent event) {
        if (!event.isCancelled()) {
            plugin.getServer().getScheduler().scheduleSyncDelayedTask(plugin, new Runnable() {
                public void run() {
                    // do block state changes here
                }
            });
        }
    }
    
    The other way to do things safely seems to have been mentioned earlier: register two events. The first event handler at NORMAL (or whatever) priority only the re/sets the cancelled flag, while a MONITOR priority handler acts upon the final cancelled flag.

    Comments?
     
  2. Offline

    _ralts

    You could construct your own BlockBreakEvent and pass it to PluginManager.callEvent(). I don't know what would happen, though.
     
  3. Offline

    Gavjenks

    So maybe I'm misunderstanding this, but this seems like a completely bizarre way to handle priorities.

    Let's say plugin A is set to lowest priority, and it listens for event X. As soon as event X happens, plugin A will place a block at spawn, or whatever. Then plugin B also listens for event X at highest priority. It cancels event X, and does not place any blocks.

    The end result, if I understand correctly, is that a block will be placed at spawn, even though the higher priority plugin did not say to place any blocks, and canceled the event?!

    Lower priority plugins should not be able to do ANYTHING (cancel the event or ANYTHING else in response to an event) if a higher priority plugin cancels it. They should never even be called. That seems like the obvious, intuitive meaning of low priority. The way it is now, how am I supposed to write a plugin that overrides other plugins, including BOTH their cancellation methods and also their reactions to the event?

    I would expect the system to do:
    > check all registered plugins for this event. Start with the highest priority one
    > if it cancels the event, then look no further / do not call any lower priority plugins for that event.
    > if it says nothing about cancellation, then call lower priority plugins until you either run out of plugins or something cancels the event.
    > if the higher priority plugin specifically says .setCancellation(false), then go ahead and call the lower priority plugins still, but "lock in" the ultimate state of the event as not-cancelled.

    Eh? Am I missing something here?
     
  4. Offline

    lycano

    @Gavjenks your example doesnt make sense. But anyway ... lets get through your example:

    Plugin A listen on Lowest was triggered through an event X.
    A Block is beeing placed by Plugin A.

    Plugin B wouldn't get the event from Plugin A cause onBlockPlace() is triggered via Player action not via plugin Action.

    So why should it cancel event X?

    Anyway your example doesnt make sense. Lowest and Low isnt for placing blocks! Its for protection / permissions. In this Prio Level events shall be cancelled or not but its not for block placement because the event cant be cancelled by other plugins thus it would be placed regardless what happens later (which you dont want!)

    If the author of Plugin A wants to support permissions he has to listen at least on normal. If Plugin B wants to cancel events for Plugin A, Plugin B has to listen on Low and setCancelled(true) plus Plugin A has to check for event.isCancelled().

    If it doesnt check for it then it would place it anyway regardless what the lower queue said. Which means the plugin doesnt respect other plugins and shall not be used cause it is most likely incompatible with any other plugin that does plugin block placement.

    Also dont try to change the way another author's event handling... For a good reason you cant overwrite the actions beeing done in someone else's plugin.

    Every plugin has a right to say what "should" happen in the upper queues (Lowest to Highest then Monitor (only for checking the outcome of an event)). The way you want it would result in a battle of prios. Who can tell the right order of event execution in this way?....

    You can cancel an event from lower prios and say "its cancelled" but you cant't decide if another plugin you didnt wrote has to do so as you say its a guideline rather than a command =)

    Dont think about names for Prios it could be 0 = Lowest, 1 = Low, 2 = Normal, 3 = High, 4 = Highest, 5 = Monitor (log)

    Every event gets the last cancel state of the lower queue ... common practice is check for isCancelled() in upper queues but thats up to the author so it doestn't have to be that way ^^
     
  5. Offline

    Gavjenks

    Dude, whatever. Pretend I said "normal" and "high" instead. That's not the point. Block placement is not the point. it's just an example. The problem is that in general, you can't override other plugins unless they go out of their way to let themselves be overriden. That doesn't make any sense.

    I see no good reason for that, actually.

    The term "priority" implies superiority / having the last say. If either lower of higher plugins all receive the event and can do whatever they want with it regardless of any other plugins or their priorities, then it is not a priority system! it's a disorganized free for all system...

    No... it is NOT in the best interest of the plugin author to override something unless there is a good reason. If you just go and make your random stupid novelty plugin highest priority, and cancel a bunch of other events, then your plugin will break everything, and nobody will download it.

    There is only an advantage to overriding when it helps the server. Such as if somebody puts out a big plugin that does 18 different things, if I make an upgraded plugin that does thing #7 better, I want to be able to override that guy's plugin entirely, so that the better version occurs, without conflicts.

    If I did that, then it would be helping the servers, and people would be happy to download it. BUT if the original plugin guy put out a new version with an even higher priority than me, out of spite (the "battle" you refer to), then it would be moving backward and breaking my plugin, thus causing servers to not be able to use the nicer version of thing #7.

    That would result in all the server owners complaining at that guy, and he would back down. End of "battle." It's not in his best interest to try and one up me.
     
  6. Offline

    Evenprime

    If plugin A wants to place a block, it should do so on Monitor priority level, AFTER it is clear that there is no reason for cancelling the event. No matter how you shuffle priorities around and how complex you get with cancelling events, as long as plugin authors are behaving stupid it will break.

    Rules of thumb:

    If you need to cancel events based on their original data, use Lowest.
    If you need to modify the values of the event, use Low, Normal or High.
    If you need to uncancel an event (there are only very, very few reasons why you'd need to do that) or need to have the last word on modifications, use High or Highest.
    If you need to do something based on the event, but not modify the event itself, use Monitor.
    Always evaluate the "isCancelled()" before taking any action.

    In your example it is plugin A's fault for not using Monitor priority to place the block at spawn. Plugin A doesn't modify event X, so there is no reason at all for it to listen on any other level than Monitor. Plugin B on the other hand should use priority lowest, to save all other plugins that come afterwards from doing useless work.
     
    xYourFreindx likes this.
  7. Offline

    Don Redhorse

    ähmm... so if I want to cancel an event and want to make sure that no other plugin can use that event I need to have lowest priority?

    oh wait no... the event goes to all plugins and that is why I need in the plugin itself to check if the event was cancelled by a lower priority

    honestly... it is a little bit confusing..

    so to keep it short:

    a) events will always go to ALL registered listeners, even if cancelled by another plugin
    b) events have a status (isCancelled)
    c) order of plugins to set the isCancelled status is from low to highest
    d) always check if an event isCancelled before you process it
    e) monitor is if you don't want to do anything with the event

    it that correct?
     
  8. Offline

    Evenprime

    Yes. That's pretty much best practices for all events that are cancellable.
     
  9. Offline

    Don Redhorse

    I think I know the problem than...

    we don't have a best practice for coding plugins for bukkit... this topic here is similar to the petition about the reload command... a lot of stuff in under the hood which a lot of people don't know about..

    and it is not presented in an easy way... I wonder if some good developers would keep up the task to write something up in the wiki?
     
  10. Offline

    Gavjenks

    No, the problem is that as the writer of plugin B, I have no way to stop plugin A from placing a block, period. Right now it makes no difference what priority either event is in terms of that: the block will be placed no matter what. therefore it is no developer A's fault. It is Bukkit's fault.

    I should have a way to stop other plugins from doing things which my own plugin renders unnecessary (or even harmful!) for the beneficial running of a server. I can't just uninstall plugin A, though, because it might be doing other things that are needed, in addition to screwing up pluing B.

    The only solution is having an ACTUAL priority system that, you know, actually gives things priority, i.e. the ability to stop another plugin from receiving an event if it is of a lower priority.
     
  11. Offline

    Evenprime

    @Gavjenks : In that case you should go to plugin author of A and tell him: "Respect the 'cancelled' state of events, you idiot!" I've done that many times now - minus the "you idiot" - and in most cases the author listened and changed his plugin accordingly. Problem fixed. Plugin B cancels the event, plugin A, which comes afterwards, ignores the event because it got cancelled before.

    If you start to really hide cancelled events from all other plugins, plugin authors will just start giving their plugin the priority level that guarantees that they get the event first. All would be (in bukkits case) listening on the lowest priority level and it would become a matter of chance which plugin ultimately gets to decide what to do with the event, if and why to modify it or cancel it.

    There are also situations where a cancelled event still carries important information for other plugins that come afterwards in the priority queue, e.g. PlayerMoveEvents. If a plugin cancels a PlayerMoveEvent, it is technically (roughly) equal to a teleportation of that player to its previous location. This is something my plugin has to know, because a teleportation invalidates lots of the data that I keep about that player. Now if cancelling an event means hiding it completely from other plugins, I'd no longer be able to handle that.

    Also plugins that do logging and want to log cancelled events as well (e.g. cancelled PlayerChatEvents) would cease to work.
     
  12. Offline

    Gavjenks

    The last couple of points can be addressed by the monitor level - if that is actually used as a monitor, then you can do all your logging without changing anything. That part makes sense.

    As for the "battle of the priorities" that you predict: what is an example of a situation where it would actually be in the best (even if selfish) interests of two or more plugin developers to battle it out for higher priority?
     
  13. Offline

    4am

    The Catch 22 here is that in such a system, other plugins can prevent yours from seeing anything to even prevent it from happening.

    The event system already receives complaints for being slow in its implementation (indeed there is a pull request out now to implement a faster event system based on almost the same model); really the only realistic way for things to work is for plugin authors to decide on and implement their events at the proper priority level (and respond to cancelations properly).

    My only suggestion (not even sure it would be a good suggestion - what do you think?) is to implement two listeners per event, one for "regular" and one for canceled events. If you don't care about receiving canceled events, simply don't listen for them. This has the advantage that once an event is canceled, plugins that don't play nice won't even see it (unless they purposefully look for it by listening for the canceled version). It may speed up the code as well because the event system will have only to call listeners which specifically request canceled events once an event has been canceled. It's a bit unecessary from the perspective of keeping Bukkit minimal - you should already be checking the canceled flag, but with this model you'd need to go out of your way to respond to it.

    Of course, this might also result in further "priority wars" as everyone would try to snag events before they become canceled (which is silly, because you could still listen on both "channels" and respond to the canceled events just the same). I've seen many bad habbits from plugin authors (hard coding the path to plugins/ grrrr) and while most of the authors or larger/more popular plugins use the current system how they are supposed to, there will always be n00bs/jerks/whatever who think it makes their plugin more reliable to listen on lowest.

    How do you enfore a system like this programatically? I'm not sure you can - at some point you need to turn control to the plugin authors and hope that good plugins just know better than to do bad things.
     
  14. Offline

    Gavjenks

    I like that solution.

    The reason this doesn't work is because a lot of plugin developers are pretty inexperienced or clueless / just learning java / etc. They don't even know how to do this sort of tidy, thorough work.

    OR they do know, but they don't care, because they're coding for a game, not a job.

    Your solution makes it so that lazy or ignorant people by default do not make bad neighbors. While people who know what they're doing have a way to still make things work (and probably are also more likely to not be jerks).

    So yeah, that's pretty good.
     
  15. Offline

    4am

    Except that it's easily defeated by everyone listening at whatever extreme end of the priority spectrum they need to listen at in order to receive the event before it is possibly cancelled.

    My only other thought would be that for any API a plugin can call that will affect the world or gameplay at all, an event should be generated based on that action so that other plugins have a standard way of being aware of any actions being taken in the game world. This would require significant alteration of the entire API; block placement would need to be passed the Player who is supposedly triggering this placement, for example. In the end, this would just make a huge mess as plugins would end up fighting back and forth about an event, stalling the server out. Besides that, it would result in several orders of magnitude more events, and there just isn't time for that type of processing logic when you need to have 20 ticks/second.

    The real fatal flaw in any event model I can think of is determining what plugin gets priority. How do you decide? Do you leave this in the hands of the plugin authors (as we have now)? Server admins certainly won't want to set up call chains themselves and then edit that list everytime they install something new. Bukkit needs to be easy to use for n00bs and people who just want a nice server (I have friends who run Bukkit+PermissionsBukkit just to keep unknown users from griefing, totally vanilla otherwise) all the way up to game hosts who need power and flexibility. It's extremely difficult to cater to both demographics and everyone in between and make it easy.
     
  16. Offline

    Gavjenks

    Again, what is a situation where people would realistically fight back and forth over priorities? I still don't see how it is in anybody's best interest to do so.
     
  17. Offline

    lycano

    ... @Gavjenks you cant really produce a live example where your question would be fully answered or create an example where this applies 100% cause it could happen anywhere with any plugin. Furthermore if you redesign it the way you want it "Priority is Priority and not Priority-Level" then this problem will happen more often and it would be harder than it actually is to handle events.


    (Short Story)
    According to the comments in Bukkit#Event.java high doesnt mean it has "high priority" its seen from event execution view. I.e. High prio level = High importance (see comments)


    (Long Story)
    Ex: BlockPlacement
    Currently: Lowest to Highest (0..6) Everyone get the event. Canceled state carried. Check for isCanceled on your registered event level.

    Two Plugins: One Listen on normal (A) and monitor (for logging) and the other (B) is listening on high.

    Both receiving the event (Remember: First loaded plugin receives the event first, and here we have another variable)
    Plugin A does only check against permissions plugin "Player has permission?" Answer was no -> doesnt place a block and setCanceled(true) -> no damage done.


    Plugin B does check against eConomy plugin which also has a permission bridge and decides "Player has permission to place a block because he has enough gold". Uncanceled the event and place a block.

    Plugin A will see this on monitor level and will log "Block was placed"
    Plugin B doesnt care about monitor cause it already has the "nearly" last say in what will happen.


    Every other plugin listening on normal (maybe they need some information) would then receive "event canceled, nothing to do.

    Now lets switch the load order. Plugin B (listening on High) was loaded first then Plugin A (listening on normal). What would happen? Still the same because the order is from lowest to highest doesnt depend on plugin load order.

    Only on the same level it would depend in what order a plugin was loaded but not on higher levels.

    Battle of Priorities: What would happen if everyone listens on High or Highest? ... Depending on the plugin load order it would mean that they battle for the last speak "what will happen" inside the same level. Plugin C says "hey im placing a block on high" is it canceled? No. Does the player have permission? No -> cancel event, dont place block.

    Plugin D says you can place a block! (Checked against eConomy or whatever) Uncancel event, place block. Plugin E want to place a block too, only checks for isCanceled(). No? -> Place block...


    Anyways ... this can be played indefinitely cause "how authors check and do stuff" is variable and thats okay cause it may be reasonable. Furthermore there is no native communication channel between plugins (only monitor could tell you what did really happen).


    And! .. It would be the same if it would be redesigned to your system "lower queues cant do anything besides fetching events .. everything is done on high or highest"
    If i got it correctly thats what it would be according to your suggestion.

    Plus if it would be Highest to Lowest then everyone would listen on Highest to receive the event ... and it would be impossible for anyone else to react on an event IF Highest would cancel the event resulting in not sending the event to lower prio levels. Though we would then need a master slave order (plugin load order depends on same level). If all this could be unified we wouldnt need any other prio level...

    Well, i hope you see now why it isnt that easy to create one event system that fits all the needs but i think the current event prio-system is okay for everyone cause its most variable... you can track all levels and react on it if something may went wrong (another plugin did something that you didnt expected).

    Someone may only have to remember that Low isn't Low priority it is in fact an event that runs on the lowest level and maybe used by another plugin for further customisation.

    It would be nice if you give a use case where you really need to override another plugins action.
     
  18. Offline

    Gavjenks

    I dont think you are answering the same question I meant to ask. I am not asking "what, technically, happens when two plugins listen on the same channel?" I already know that.

    I am asking "why would everybody even be trying to listen on the same channel in the first place, with any of these proposed systems?" Even if we assume plugin makers are 100% selfish, it is still not in their best interest to all set their plugins to whatever gets the highest priority or last say.

    For example, if I am making a protection plugin, it would be in my best interest to make it a priority that CAN be overriden by other plugins. Why? Because that makes the most sense for a protection plugin, and makes it easier to work with other plugins. Which means that more people will like my plugin and will download it. If I make it so that nothing can override it, then it will be virtually useless and will lock out other plugins, and NOBODY will download it.

    People keep saying "Oh but you can't be able to override other people's plugins, cause there would be a war of one-upping" I don't see how anybody is arriving at that conclusion? Why would there be? It would be dumb for people to do this - it only hurts them to one-up when they shouldn't be.
     
  19. Offline

    hatstand

    @Evenprime summed it up pretty well:
    Whether or not it is the smart or logical thing to do is irrelevant, not everyone thinks in that way.
     
  20. Offline

    4am

    [quote="hatstand, post: 787577]Whether or not it is the smart or logical thing to do is irrelevant, not everyone thinks in that way.[/quote]

    That's just the thing - in my suggestion, you can still access cancelled events at the appropriate priority level, you just need to explicitly listen for them. Those that need to do so will do just that, and those that don't know/don't care will never know about them - and therefore won't interfere when they shouldn't.

    If someone already is aware that they need to register for cancelled events, why would they instead try to capture everything up front instead of capturing cancelled events appropriately? They'd need to _intentionally_ violate the guidelines. Doesn't completely solve the problem, but I believe a lot fewer plugin authors are actually fighting for priority as opposed to accidentally ignoring isCanceled().
    This also has the advantage of lower costs of calling event listeners; if no plugins (or even if fewer plugins) register for cancelled events, once an event becomes cancelled that results in fewer calls into listeners that the server even needs to make; that means fewer context changes (jumping down a listener just to have it return immediately), fewer iterations in the event processing loop - events will exit quicker. Savings is typically insignificant, except perhaps in cases of *_MOVE events, but every little bit counts.

    I'm thinking that at Monitor level, all events should hit under one listener, cancelled or not. If you're using Monitor you should only be logging, and so it's implied that you want everything anyway. It's also a supporting reason to leave isCanceled() in the Event object, which means less of a need to alter the existing structure.
     
  21. Offline

    hatstand

    I actually quite like the sounds of this method, and it wouldn't interfere with many (probably any) plugins, if Monitor level is implemented as you suggest.
     
  22. Offline

    mattmoss

    Can I get a bit of clarity on this? Are these correct (in particular, #2)?

    1. If I am going to cancel the event, or change the event per se, I need to use a priority other than Monitor.

    2. If I will not cancel the event or change it per se, but I will change the environment based on event content, is it okay to use Monitor?
     
  23. Offline

    Evenprime

    Pretty much that's how it is intended.
     
  24. Offline

    4am

    On #2: You should be careful how you react to this event. If you aren't doing massive block changes a-la WorldEdit, you should post any changes you make into the event queue and watch to see if they're canceled using Plugin.callEvent(). I've never done this, so I assume this just posts the event into the queue and doesn't actually trigger the action the event is describing (i.e. posting a PLAYER_CHAT doesn't cause a broadcast - you'll need to do that seperately). After you call callEvent(), check imediately afterwards to see if your event was canceled; abort if it was.

    For example, player issues a command to plugin A which places a block; plugin A should callEvent() with a Event which has a BLOCK_PLACE Type. Your Listener on Monitor level catches this as having made it through the queue uncancelled by other plugins; but you are Monitoring and you get the final say, so you cancel it because the player isn't allowed to build in that region.

    After the Event goes through the call chain, callEvent() returns to the original plugin. The original plugin checks isCancelled() and determines that you disallowed the action, and returns before actually placing the block.

    This is how it works now, but I'm not sure how many plugin authors actually post their own events before taking actions on the world - sometimes this makes things difficult to police when a player isn't the direct cause of an environmental change.
     
    mattmoss likes this.
  25. Offline

    Don Redhorse

    Like I said... we need some more examples on top of the Code your first plugin Wiki page...

    Best Practices is the word..
     
    4am likes this.
  26. Offline

    Sleaker

    The plugin author should be generating their own custom events for this. That's what the more advanced systems do, and it works very well. If a plugin authors understands that their plugin's actions should be modifiable by other plugins, they they should provide a way to do it through it, bukkit's event system serves this function wonderfully (regardless of the optimizations that are/aren't in place currently)
     
  27. Offline

    4am

    Custom events (as in the ones caught by CustomEventListener) should only be reserved for events which do not exist in Bukkit (joining a chat channel, for instance), correct? I should still post a BLOCK_PLACE event if I want to place a block programatically (which was not directly caused by a player right-click), correct?
     
  28. Offline

    Sleaker

    Right, I was just saying that if you generate a block during a block place event it's generally that plugin's responsibility to generate a new event for it, rather than that new method being cancelled/dealt with in the current event or being cancelled by downstream choices.
     
  29. Offline

    4am

    Cool; just wanted to verify that posting non-custom events wasn't a bad thing.
     
  30. Offline

    SniperFodder

    Mmm, Interesting thread to say the least. I'm a novice programmer and I generally ignored event priority, only because I had no idea what it was. My plugin worked, all I cared about. However reading this thread I have re-adjusted those views and ensured I ignore events that are cancelled.

    I think it would be an excellent idea to at least ensure this thread is linked somewhere within the plugin tutorials on the wiki to help remove confusion on this topic. It won't remove all confusion, but it will help alleviate it. I'd rather all the new guys learn the right way from the start. Otherwise it could be a problem down the road for them.
     
Thread Status:
Not open for further replies.

Share This Page