Listener uses EventPriority.HIGH, should use EventPriority.LOW

[LadyCailinBot]

WORLDGUARD-2784 - Reported by NOVUS

Most of the WorldGuard event handlers target EventPriority.HIGH.

As a block protection plugin, this is improper. Handlers should target EventPriority.LOW so that other plugins are able to correctly check event.isCancelled().

This issue caused a problem with my plugin, WeaponLevels. Using normal event priority, I checked if the event was cancelled before removing the block from the world. Since WorldGuard hadn't gotten the event call yet, my plugin was thinking that the block was able to be broken. This issue would enable unlimited griefing in supposedly "guarded" areas.

By changing handlers to EventPriority.LOW, it would make events much safer and prevent issues like this from occurring in the future.

[LadyCailinBot]

Comment by wizjany

I agree we shouldn't be using HIGH (for most things, not everything worldguard does is block protection) but we shouldn't be using low. On the other hand, you should probably be using highest.

[LadyCailinBot]

Comment by Dark_Arc

What is the disadvantage to using low priority?

[LadyCailinBot]

Comment by sk89q

There isn't really one. It's supposed to be LOW.

When I ported WorldGuard, I was under the impression that HIGH happened first. I realized my error not long after but I never got around to changing it.

[LadyCailinBot]

Comment by NOVUS

Pull request made. https://github.com/sk89q/worldguard/pull/274

[LadyCailinBot]

Comment by Tomy.Lobo

question: what you do doesn't sound like it involves modifying the event, so why don't you have it at MONITOR priority like you should?

[LadyCailinBot]

Comment by sk89q

@Dark_Arc there is onnne disadvantage -- poorly coded plugins that setCancelled(false) when they shoud not -- that is partly why I didn't change it

[LadyCailinBot]

Comment by Tomy.Lobo

maybe insert at both LOWEST (to cancel the event) and HIGHEST (to make sure it'S still cancelled)?

[LadyCailinBot]

Comment by level1kid

@sk89q: Don't plugins have to explicitly ignore canceled in order to un-cancel the events?

@Tomy Lobo: Other plugins wouldn't know it was canceled.

[LadyCailinBot]

Comment by Dark_Arc

@sk89q That's true, there may be more benefits to switching it though. I mean I don't know of any plugins that make the mistake of uncanceling events, then again I don't use many plugins that aren't made "in house" anymore.

@Tomy Lobo, don't you think that would cause more problems than it would solve?

@level1kid They don't have to ignore cancel to uncancel events, they can't even uncancel events if they ignore canceled.

[LadyCailinBot]

Comment by Tomy.Lobo

If you're replying to my first comment: He doesn't cancel the event (or at least he doesn't mention that). That's why I said it doesn't sound like he's modifying the event (setCancelled is a modification)

If you're replying to my second comment: I said insert it at both. All plugins > LOWEST would know it was cancelled. The 2nd invocation would make it --absolutely sure--more likely that it stays cancelled.

[LadyCailinBot]

Comment by Tomy.Lobo

@darkarc, i dont know which post you're replying to.

[LadyCailinBot]

Comment by level1kid

@Dark_Arc you have to explicitly set ignoreCanceled to true in the @EventHandler annotation to receive canceled events.

[LadyCailinBot]

Comment by Dark_Arc

@level1kid No you don't... That's the exact opposite of what that does...

[LadyCailinBot]

Comment by Dark_Arc

@Tomy Lobo I was replying the second comment. If you have to check twice, your either going to be doubling the db processing, or creating a cache... both of which are not as cheap as just doing it once.

[LadyCailinBot]

Comment by sk89q

@Tomy Lobo I've considered that in the past, but then we'd have to keep track of our last cancel and recancel on HIGHEST, which is overhead that I have not been enthusiastic for adding.

@level1kid You have that backwards unfortunately. The presence of ignoreCancelled=true auto-ignores cancelled events on your behalf, but its absence of it means that your handler get called for both cancelled and non-cancelled events.

[LadyCailinBot]

Comment by Tomy.Lobo

A cache would be a good idea anyway - just a position -> applicable regions cache that'd be cleared whenever some region's extents, priorities or parent is modified or a region is added/deleted. And since this is "just" user interactions, I don't see it being too big of a hit anyway.

[LadyCailinBot]

Comment by sk89q

I do have a region cache that I wrote a while ago, but IMO, it should be an event cache in this case.

[LadyCailinBot]

Comment by level1kid

@Dark_Arc @sk89q : oops, I'm an idiot.

[LadyCailinBot]

Comment by Tomy.Lobo

An event cache would certainly be the fastest way, but what if the underlying Bukkit implementation re-uses event instances to be more efficient? Nothing in the API guarantees it doesn't.

What we could be reasonably sure about, however, would be that no 2 events of the same type intersect on the same thread. Right now, block events are still all on one thread, I think, so we could just store the last instance and compare and clear it. It's cheap, too. (ignoreCancelled should definitely be false though :P)

Thread safety can be added by using a java.lang.ThreadLocal.

[LadyCailinBot]

Comment by sk89q

Intersect on the same thread?

[LadyCailinBot]

Comment by Tomy.Lobo

It's possible, if one event handler emits another event or does something that emits an event. Although I don't know why anyone would emit BlockEvents :)

[LadyCailinBot]

Comment by sk89q

A block that places other blocks would emit block events, as well as custom mechanics.

[LadyCailinBot]

Comment by Dark_Arc

There is really no need to have a cache imo. If people have mods that conflict, is it really our problem? I don't think we should cater to these particular cases. LOW, may provide performance boost for some servers as some code will likely be ignored that is otherwise ran.

[LadyCailinBot]

Comment by sk89q

Our problem if we get the complaints.

[LadyCailinBot]

Comment by tomco

I strongly advise you use LOWEST as the priority when checking events you may want to cancel. Using HIGH causes problems with our plugin [1] and probably many other plugins as well.

Even the Bukkit documentation says to use LOWEST for protection plugins [2]. Also note that GriefPrevention uses LOWEST for their event listeners [3].

[1] https://github.com/craftinc/craftinc-gates/issues/25 [2] http://wiki.bukkit.org/Event_API_Reference#Event_Priorities [3] https://github.com/Tux2/GriefPrevention/blob/master/src/me/ryanhamshire/GriefPrevention/BlockEventHandler.java#L170

[LadyCailinBot]

Comment by Dark_Arc

I wouldn't go lowest, the lookup system is not at all light weight. If we go lowest we loose the ability to avoid "block nukers" which may have been prevented by annother plugin, and then this lookup may very well kill the server.

[LadyCailinBot]

Comment by sk89q

This thread is being all banter.

Conclusions, in summary:

1. LOWEST OR LOW was the original intention.
2. There are bad plugins out there that uncancel events.

Let's ignore #2. If it becomes a problem, we can deal with it.

The deciding factor between LOWEST and LOW is whether any of WG's stuff relies on another plugin having cancelled an event, and we need to check for it. I can't think of anything substantial.

[LadyCailinBot]

Comment by Tomy.Lobo

agreed

[LadyCailinBot]

Comment by Dark_Arc

Like I said LOWEST should be avoided because many AntiHacking plugins will be listening there that may cancel the event (thus saving the targeted server from a potentially very demanding lookup, especially in the case of a "block nuker").

Edit: The above is true only for region lookup based functions, and we could alternatively provide some kind of innate protection in WorldGuard against "block nukers". Other things like the blacklist would be fine to set on LOWEST however.

[LadyCailinBot]

Comment by sk89q

Everything will be NORMAL, as I rewrote most of the listeners.