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

    Dinnerbone Bukkit Team Member

    When you register events in bukkit, you assign it a priority. Most people use Priority.NORMAL, and that just makes sense, right? Normal's normal. Until you think about other plugins.

    There's 6 priorities in bukkit:
    Highest
    High
    Normal
    Low
    Lowest
    Monitor

    They're actually called in this order:
    Lowest
    low
    Normal
    High
    Highest
    Monitor

    They look sane, right? Except for that obvious bolded "Monitor" one which just sticks out. Not just because it's bold. Really. I'll get to that in a second.

    The theory behind this system is, every plugin gets a say in what happens, and every plugin must get a chance to know the outcome of an event. So, we pass events to plugins even after they've been cancelled. A plugin can actually uncancel an event after another plugin cancelled it. This is where priorities become really important.

    Let's say a BLOCK_PLACE event is being handled. The lowest priority listener is called to get its say in whether it should be cancelled or not. Then the low priority listener is called to see if it wants to override the low, etc. Eventually it hits monitor, and at this point nothing should change the outcome of the event. Monitor should be used to see the outcome of an event, without changing any aspect of it.
    If we have three plugins enabled; one is a basic area protection plugin, one is a fancy plugin using signs, and another is a logging plugin.
    The protection plugin listens on Priority.LOWEST. It says they can't place blocks in this area, and cancels the event.
    The fancy sign plugin listens on Priority.NORMAL. It says they can place signs here, and uncancels the event.
    The log plugin listens on Priority.MONITOR. It sees that the event was actually allowed, and logs it.


    Basically, if you want to change the outcome of an event, choose very carefully from LOWEST to HIGHEST. I'd suggest generalized protection plugins on lowest, more specific plugins on normal, and override plugins on high.
    If you want to act when an event happens, but not change the outcome, use monitor. It's really really important that you use monitor, or an event might get cancelled after you've acted from it, and it's even more important that you don't change the outcome of the event on monitor or it'll break other plugins.

    I could explain this better, but I'm hoping you'll get the idea. Any questions or comments?
     
  2. Offline

    Raphfrk

    This presumably means that we should check the cancelled state of an event before doing anything?

    Btw, on the question of protection plugins, it would be nice if there was a way to check if a block is protected.
     
  3. Does this work in the reverse also, e.g. an event is not cancelled with a plugin of lowest, but that event could get cancelled by a plugin of high
     
  4. Offline

    DjDCH

    Nice topic. I was asking myself how bukkit works when multiple plugin used the same event and someone of them cancelled the event.

    Depending what your pluging doing, it should be always done, like a programming standard, to check if an event is cancelled or not.
    From what I read, yes.
     
    Corndogoz likes this.
  5. Offline

    feverdream

    Very nice topic.
     
  6. Offline

    Purre

    Extremely useful! As this is the first time I'm developing plugins for a Minecraft mod, I haven't been quite sure on how to handle those priorities.. Thanks :)
     
  7. Offline

    Raphfrk

    Btw, if monitor priority events are not supposed to cancel events, maybe you could include an error message if they end up doing it.
     
    glen3b likes this.
  8. Offline

    apexearth

    So it's more of an order than a priority. Confusing, but not so much since I've read this.
     
  9. Offline

    DjDCH

    Or directly remove the setCancelled mutator.
     
  10. Offline

    Mattie

    FYI-- Priorities are broken at the moment because of some missorting, so Monitor was firing first before even things with Highest priority: http://bugs.bukkit.org/issues/211

    I submitted a pull request here to resolve it which I'm using on my server: https://github.com/Bukkit/Bukkit/pull/53

    *Edit*: Looks like this change landed so future Bukkits shouldn't show this issue.
    -Mattie
     
  11. Offline

    4am

    Now, if I where writing a protection plugin, I would register my events on Highest, so that no matter what other plugins did (in case one was configured incorrectly) I could still cancel the action. Doesn't it seem to open the door to misconfiguration and security issues to recommend that protection plugins register low and have 5 tiers of other plugins which could possibly override?

    I know it''s much more complex, but perhaps there should be some type of system where the admin allows plugins to override each other? Perhaps first on the list gets highest, etc? Or perhaps we need some more events which fire when an event completes with no cancelation - so for example if a player modifies a block, BLOCK_CHANGE (or whatever) is fired, goes through the whole sequence and isn't canceled by anything, protection plugins can be notified though something like BLOCK_CHANGE_SUCCESS and reverse the action?

    Not that I think there will be that many popular rouge or miscoded plugins, but this level system seems a bit to ambiguous, and I feel like it's going to lead to race conditions or unexpected in-game results. In the example you gave: what if I don't want FancySign to override my Protection plugin and my users start dropping signs everywhere? Does that require plugin authors to code plugins against each other to be compatible, i.e. instead of coding FancySign to make fancy signs, will I need to code FancySign to be aware of Protection (and Realms and Cuboid etc) and check their permissions and boundaries before I allow my action? What does an admin do in a case where this type of conflict arises and there is no configuration solution?

    I think perhaps at minimum there should be an option setting in the server.properties file for each plugin:

    plugins:
    PluginName:
    - prioritymax: <highest level>
    - prioritymin: <lowest level>

    Which overrides the programmers's wishes. If this will break functionality, then the plugin should throw an error and refuse to load.

    THoughts?

    EDIT: Another issue, what happens when two plugins are registered at the same event level? What order do they receive the events? What if they result in two different outcomes for the event?
     
  12. Offline

    Mattie

    First to fire should win like in every priority scheme I know in software systems. This is just the way things go. Let's wait until we have some real reasons to implement other "levels" before changing that. Other projects will implement "filters" if they come up with a brilliant reason to do that, but you shouldn't go that route until you have to. KISS.

    But we need to stop processing the event right away whenever a plugin uses setCancelled(). For all practical purposes, once an event says it is over from the highest level, we need to pretend it never happened for those below (like all other systems like this).
     
  13. Offline

    4am

    I agree, although I do see a need for being able to detect an event being canceled as well. It's certainly a chicken-and-egg type of problem.

    Perhaps if it's not easy to act on canceled events without checking it would certainly alleviate many concerns. As I understand it now, callbacks need to take an extra step and check to see if the event has a canceled flag set? Perhaps hooks should only register to see active events and require an extra parameter set to true in order to receive those events which where previously canceled? Otherwise I see many (probably less experienced) authors riding in the Highest queue to ensure reliability of their event trapping. Poor coding form, for sure, but certainly possible.

    I'm a bit confused by this statement. I know events will queue in the order in which they happen; My question is, if a BLOCK_CHANGE fires, and two plugins are watching at (for example) the Normal level, which plugin gets it first? Is it order-of-loading (which I believe currently appears to be reverse-alphabetical by class name, but no I haven't read the source [​IMG]), or is the order undetermined? What about when threads are implemented - the event record will need to be mutex; who gets first dibs at a given level? What happens when two plugins disagree about the outcome (canceled/not canceled)?
     
  14. I WANT TO ASK EVERYONE PLUGIN DEVELOPERS
    When you listen event not in Lowest priority, check first if that event wasn't cancelled! Cause there can be other plugins, whitch cancells that event.
     
  15. Offline

    Redecouverte

    I have exactly that problem right now.

    I want to cancel all player issued commands for a specific player for 15mins. Now normally I would register a PlayerListener to the PLAYER_COMMAND event with priority LOWEST and just cancel the command event.

    BUT as most plugins my server is using (Essentials, ...) don't care whether the event has been canceled or not, i have no way to stop them from acting upon these command events.
     
  16. I'm doing event.setMessage(""), if I want to cancell command event, and it works, but making an errors in server console.
     
  17. Offline

    Redecouverte

    that's an awesome idea!

    thanks for sharing :)
     
  18. Offline

    Eris

    I guess I'm coming a little late to the discussion, but I feel that conflating filtering and reacting is a huge design mistake. Apple's Cocoa framework actually has three types of delegate methods for events: the "should", the "will" and the "did"—although I haven't seen any events that have all three.

    Allow me to create a hypothetical situation here to show the flow of how these work.

    Code:
    public class BlockListener implements Listener {
    
        // Called when a player attempts to place a block.
        // Return true to allow the placement; false to disallow.
        public boolean shouldPlaceBlock(BlockPlaceEvent event) {
            return true;
        }
    
        // Called before the block is placed; it's too late to do anything about
        // it, though. If the event was cancelled, this doesn't get called.
        //
        // Should be overridden when the plugin needs to react before the action
        // has occurred in-world.
        public void willPlaceBlock(BlockPlaceEvent event) {
        }
    
        // Called after the block is placed; obviously too late to cancel now. If
        // the event was cancelled, this doesn't get called either.
        //
        // Should be overridden when the plugin needs to react after the action
        // has occurred in-world.
        public void didPlaceBlock(BlockPlaceEvent event) {
        }
    
        // etc.
    }
    
    The downside is that it would take three times as long to dispatch cancellable events that have both before and after callbacks. If the Bukkit devs don't feel that it's necessary to have "will" and "did" as separate phases, then I think it's perfectly reasonable to have only one or the other.

    On the other hand, with just a filter phase and a single post-reaction phase, it would be possible to dispatch the reaction phase asynchronously on another thread once the action has completed. Nice.
     
  19. Offline

    Dinnerbone Bukkit Team Member

    I don't see what the difference is between priorities and your example are, aside that yours takes up more processing time. As long as you use priorities right, they're doing exactly this currently; Should and will are handled depending on how important you think your plugin is compared to another, and did is monitor.
     
  20. Offline

    Eris

    @Dinnerbone

    I think the big difference is that your model makes no attempt at catching accidental "bad behavior": it's perfectly possible for a plugin registered with normal or any other priority to act on an event which is later cancelled by a higher priority plugin, and it's impossible to know what other plugin developers are doing in this respect. It's my understanding that Monitor should have no side effects (modifying the world etc.), so we shouldn't react to events there beyond logging, yes?

    It also makes a few use cases impossible. Ostensibly, "Highest" means your plugin has the final say in any matter, but as highlighted before, a lower priority plugin is perfectly able to react, with side effects, to an event only to have it cancelled later by some other plugin with a higher priority. In most cases, this is probably not what the plugin developer wanted, but there is no way of knowing what will happen higher up on the priority chain.

    I'd liken the current solution to the cooperative multitasking of Mac OS 9 or Windows 3.x: it's effective and it's computationally efficient, but it's not an optimal solution and you can shoot yourself (and others) in the foot too easily without meaning to. It relies too much on cooperation and convention.

    It's true that my suggestion is more computationally intensive, but the main bottlenecks for a Minecraft server seem to be disk access followed by memory. The added computational hit could be mitigated since the handlers won't be called for events that are cancelled. Dividing "filtering" and "reacting" into separate classes would further reduce the number of redundant calls, I think.

    I believe I agree with you however, after consideration, that "did" is essentially filling the role of Monitor. There should only be two phases, then: filtering events and reacting to them.
     
  21. Offline

    sunkid

    Here is my use case: I want to work compatibility with protection plugins into CreativeStick. CreativeStick lets you place, replace, and remove blocks by wielding a stick around. It does so currently, by simply listening to onPlayerAnimation(...), finding the block pointed at, and changing its Type according to the stick's current settings.

    In order for other plugins to be able to prevent the stick's action, they will need some cancelable event to be fired. In order for the plugin to continue to work like it does and to fit in with the priorities, it would then have to fire that event itself in a listener called very early (e.g. one using Priority.Lowest) but only act on the intended action in a listener called very late (e.g. one using Priority.Highest).

    Accordingly, shouldn't the recommendation be that all protection-type plugins listen with a priority level of Priority.Low to allow other plugins to generate cancelable events first?
    --- merged: Mar 6, 2011 10:14 PM ---
    EDIT: It seems that my head cold has my brain as congested as my nose is... essentially, my previous point is moot since a protection plugin would be listening to different events than my plugin or other creative plugins.

    What is important though, is that developers of creative plugins build in the necessary creation of events and then make sure that when those events are cancelled, that the plugin also undoes its own action. Alternatively, bukkit should fire events whenever blocks are changed in any way. Right now, it doesn't seem that block.setTypeId(...) fires anything within bukkit (it does probably within Mojang's server implementation though, as cacti pop off, for example, when you change the type of an air block next to them).
     
  22. Offline

    Nohup

    Not totally moot though, since you could point your stick with an intended change to air and *poof* protected blocks are gone. That is one of the problems I am trying to deal with right now between DropBonus and WorldGuard. To "override" what a block drops when it is broken basically I turn it into air so it drops nothing by default. Once I heard back from on of the servers using my plugin that protected blocks were disappearing I figured out what it was and told them a workaround for now, but even then the drops configured for those blocks were dropping.

    So, I updated my code to check for cancelled in onBlockBreak, and set my thread to highest. Unfortunately this doesn't seem to be working :( I checked out WorldGuard's source and their onBlockBreak is set to high, so why would my check on cancelled NOT see it as cancelled on a protected block? I can try Monitor if that is truly guaranteed to process last, I am not looking to change the event just react to it.

    However, the main point being that High isnt reporting to Highest properly from what I can see. Thoughts? My first thought is that these are all firing in asynchronous threads and there is no guarantee, but I haven't dug through the code to see if I am right there...
     
  23. Offline

    sunkid

    I think the problem is simply that BlockBreakEvent is not fully implemented yet, at least I don't see onBlockBreak fire when I create an instance of it. I worked around this for now by using BlockPlaceEvent for all edits, even replacements.
     
  24. Offline

    Nohup

    onBlockBreak works fine, my code is fired as it should be, but the issue I am having is that it is too efficient ;) I am trying to make my code check isCancelled so that when WorldGuard sets the event to cancelled I don't continue along and drop bonus items for that block...
     
  25. Offline

    sunkid

    Sorry, I may not have been clear or I am not understanding your approach. I used an edited version of AntiGrief (inserting debugging statements only) to test my set-up and found that onBlockBreak was not called in AntiGrief even if I created that event. On the other hand, onBlockPlaced is called when I create a BlockPlacedEvent.
     
  26. Offline

    Nohup

    what version of CB are you using? We are getting a bit off track here... I am not creating an event, just responding to the ones that CB are creating. WorldGuard is also responding to this same event. It's priority is marked high, mine is marked highest, therefore my processing should occur AFTER WorldGuard's processing per this thread. WorldGuard sets event to cancelled, but although I am checking for isCancelled I am still getting false back. the only thing I can speculate is that my thread is processing before the event is set to cancelled...

    I have updated my priority to monitor, will see if that helps at all, but even if it does there seems to be an inconsistency here.

    There may be an inconsistency in what I thought was happening and what is... turns out he is using iZone and not WorldGuard lol. iZone's code doesn't appear to be available to see what priority level is being used.
     
  27. Offline

    lycano

    Hi,

    i'm currently trying to understand these Priority Levels. Maybe someone could help me with it. After reading this thread there are some side effects if you want to assign a written plugin to a specific Priority-Level e.g Protection Plugin to Lowest and Chat/PlaceBlock to Normal because most of the plugins listens on Highest and in many cases they overwrite decisions made by lower levels disrespecting event.isCanceled().

    Let's say we have a protection plugin that listens on essentially all Player Events on Highest. Another Plugin that can filter the players message wants to act on normal priority as suggested.

    If the user has done something wrong and the plugin wants to notify the users about what he has done wrong it would normally check against event.isCanceled and if not send this message to the player, modify his message and send the modified message.

    Now the Plugin listening on Highest is canceling the event PLAYER_CHAT preventing the user from chatting. But the notify to the user was send on normal priority because the PlayerChatEvent .getPlayer() player.sendMessage doesn't know about what will happen later. It only sends a message to the player disrespecting events canceled.

    If the protection plugin would listen on lowest the chat filter plugin could catch a canceled event. As i've read this is the design of those priority queues Lowest to Highest; Respect canceled events on each priority-level. If anyone who writes a plugin would respect event.isCanceled() everything would be fine.. but currently many plugins don't respect a decision made by a lower level =)

    To get the event.isCanceled() (if it is triggered on high) then the chat plugin has to listen on high to respect the canceled event set by the protection plugin. The conclusion is as mentioned in this thread => Allways listen on highest. Is there another common way?

    In addition if Highest should be used to override actions then this is a bit confusing. Ok, i agree proctection plugins mostly "overrides"/disallowing actions that the player requested but this can be done on lowest level. Benefits: Other plugins will be notified because of event.setCanceled(true); Therefore if other plugins respect isCanceled() = no problem. This should be the only way! Another Benefit is that a simple plugin that only wants to know "can the player chat?" simply has to check against isCanceled(). Simple permission support is then given if the permission plugin would set a cancelEvent()

    \-\-\-

    Asking about Events:

    1) What will change the outcome of an event in detail? (I mean in which order plugins are loaded?)
    1a) Is it random?
    1b) or will a plugin load in alphabetic order
    1c) depending on its package name?

    2) What is the current implementation of "How a plugin is loaded at runtime inside each Priority level"
    2a) If 1b) is true explanation given.
    2b) plugin a: low, b:low; c:normal, d:normal; e:high, f:high. What plugin will decide the outcome of an event in each queue if the load order is not defined?
    2c) If random, how can anyone rely on events?

    Thanks for your time,

    Regards, lycano
     
  28. Offline

    Eris

    @lycano looks like the best you can do is only act on events in a monitor-level listener, which will mean multiple listeners for the same events if you want to both filter and act. It's your responsibility to take into consideration the cancelled state of an event if you don't want to override other plugins' decisions. I guess this is an acceptable event model provided everybody follows the rules, although I don't think it's ideal.
     
  29. Offline

    lycano

    @Eris: Hmm not the answer i want to hear but thanks. I will handle it then as i did before (monitor is for logging only as i've heard; my last post is deprecated anyway :D)
     
  30. Offline

    mattmoss

    Does "outcome of the event" merely mean cancellation status? Or is changing the block state (in the case of a BlockPlaceEvent or similar) also considered the outcome of the event?
     
Thread Status:
Not open for further replies.

Share This Page