Not able to register event handlers in onClose, unless I scheduleSyncDelayedTask

[hopperelec]

This is a bit of a strange issue and is likely to be more of a Bukkit-related issue so I'm not expecting a "fix" to be made in AnvilGUI, but I imagine the issue is being caused by something AnvilGUI is doing and I don't know what that could be so I wouldn't really know what to ask in any Bukkit-related forums so hopefully it's ok to ask about it here.

Basically, my plugin uses a custom GUI system and I insist on only using one listener (GUILIstener) for handling inventory-related events for all these GUIs. I also insist on this listener only being registered when one of my plugin's GUIs is actually open. So, whenever one of my plugin's GUIs is opened, I register the listener, and then I unregister the listener once the inventory is closed.

I am using AnvilGUI to allow players to search through my plugin's paginated GUIs. So, when the AnvilGUI is closed, I re-open the paginated GUI. As a result, my plugin tries to re-register the GUIListener. However, for some reason it doesn't actually start listening, but it does if I re-open the inventory from outside of onClose or within a scheduleSyncDelayedTask.

My first guess was that this was because onClose was being executed outside of the main thread but, looking at AnvilGUI's source, it seems that it just stems from onInventoryClose (which calls closeInventory(false)) which, as a Bukkit event, should be executed on the main thread.

Of course, there's not really any problems with me just using scheduleSyncDelayedTask to get around this, but I'm very curious what could be causing this and I'm also worried that it could cause further issues down the line. Any ideas?

[0dinD]

I insist on only using one listener

Not necessarily a bad thing, in fact it's often better that way. Not that it matters that much, but yeah you'll want to avoid having 100s of listeners.

I also insist on this listener only being registered when one of my plugin's GUIs is actually open. So, whenever one of my plugin's GUIs is opened, I register the listener, and then I unregister the listener once the inventory is closed.

This sounds counterproductive. Not that I've looked at your code in detail, but in general there's very little reason to unregister listeners especially if you intend to re-register them again and so on. Registering or unregistering listeners is bad for performance because the HandlerList needs to be rebaked each time. At least, as far as I've understood it. You can read more about it or ask in the SpigotMC or PaperMC Discord servers, for example, here is an old answer in the SpigotMC Discord.

---

As for this issue you're reporting, I don't know exactly what's causing it, but if you want to keep debugging it I would suggest setting some breakpoints to verify what happens when you try to register the event handler. It's not clear to me whether you've verified that the event listener doesn't get registered at all or whether one (or more) events didn't fire, but maybe the listener is in fact still registered. Either way here are a few possible causes that I would also investigate:

• The close event used to be fired twice in AnvilGUI on some Minecraft versions, make sure to update to the latest version of AnvilGUI in order to get this fixed.
• Opening or closing inventories from a click event is "not safe". Not sure if this applies to other events etc as well but it's clear that inventories are a bit finicky in Bukkit.

Of course, if there's a problem in AnvilGUI, then we should fix it. So please do investigate this and report your findings. 🙂

[hopperelec]

Registering or unregistering listeners is bad for performance because the HandlerList needs to be rebaked each time

My understanding was that this was only really a problem if you were unregistering from all different event handlers, but I'm only unregistering from InventoryClickEvent and InventoryOpenEvent. However, the helpers in the SpigotMC Discord certainly seem very against it, and most of the responses I've seen are for GUIs. So, I might stop doing this, but it's worth noting that I think AnvilGUI is doing the same thing.

AnvilGUI/api/src/main/java/net/wesjd/anvilgui/AnvilGUI.java

Line 221 in fe66ea6

HandlerList.unregisterAll(listener);

It's not clear to me whether you've verified that the event listener doesn't get registered at all or whether one (or more) events didn't fire

I haven't directly checked if the event listener has been registered, but, if I close the search GUI while nobody else is viewing a GUI, neither InventoryCloseEvent nor InventoryClickEvent are fired. But then, if someone else opens a GUI while I'm still looking at mine, they start being fired again, which I can only assume means the event listener wasn't registered the first time. I'll double-check, though!

make sure to update to the latest version of AnvilGUI in order to get this fixed.

I'm on the latest version

Opening or closing inventories from a click event is "not safe". Not sure if this applies to other events etc as well but it's clear that inventories are a bit finicky in Bukkit.

This is not actually something I knew and I'm pretty sure I'm currently doing what is being advised against here😅I imagine it would be documented if this also applied to InventoryCloseEvent, though.

[hopperelec]

I haven't directly checked if the event listener has been registered
...
I'll double-check, though!

Double-checked now by doing this

System.out.println(Arrays.toString(InventoryCloseEvent.getHandlerList().getRegisteredListeners()));

And you're right, it is being registered

[RegisteredListener{plugin="ItemRace", listener="uk.co.hopperelec.mc.itemrace.listeners.GUIListener@2d5aae64", executor="TimedEventExecutor['ASMEventExecutor['public void uk.co.hopperelec.mc.itemrace.listeners.GUIListener.onInventoryClose(org.bukkit.event.inventory.InventoryCloseEvent)']']", priority="MONITOR (5)", ignoringCancelled=true}]

but for some reason the events aren't being fired🤨But then, if I re-register it, the output of the above code doesn't change, but they start being fired.

[hopperelec]

Regarding the warning about not opening inventories from InventoryClickEvent, do you know if this applies to AnvilGUI.Builder.open? In other words, can I trust that AnvilGUI.Builder.open calls openInventory within a runTask (or similar) call?

[0dinD]

My understanding was that this was only really a problem if you were unregistering from all different event handlers, but I'm only unregistering from InventoryClickEvent and InventoryOpenEvent. However, the helpers in the SpigotMC Discord certainly seem very against it, and most of the responses I've seen are for GUIs. So, I might stop doing this, but it's worth noting that I think AnvilGUI is doing the same thing.

I could be wrong, but I think the rebaking has to happen whenever any listener is registered or unregistered. It could be that the server waits until the end of the tick or something to do it (so that multiple registrations are batched together), but either way I think it's "best practice" to register listeners once, when your plugin starts. Of course, there are some exceptions, like if the user changes the config and a listener is no longer needed at all (in which case, unregistering can make sense). Now, in the end, the performance impact of (un)registering listeners all the time might not be that noticeable if your server doesn't have many plugins installed (i.e. there are not that many listeners to rebake), so again, it's a best practice, but probably not the end of the world if you don't follow it.

And you're right, AnvilGUI is also not following best practice here, in fact AnvilGUI is registering and unregistering a listener each time the anvil is opened, for each player. To be honest I was not aware of this issue, I was not involved in the initial development of AnvilGUI. Fixing this now would require a bit of refactoring, which is extra tricky in AnvilGUI since this is a library that many people depend on, and ideally we want to avoid accidentally breaking existing functionality in the plugins which use AnvilGUI. If someone has evidence that the performance impact for AnvilGUI is large, then it might be a good idea to investigate such a refactor, but otherwise, I think it's pretty low priority.

I imagine it would be documented if this also applied to InventoryCloseEvent, though.

Yeah one would hope so, but you never know with the Bukkit documentation. If it's a real close event triggered by the player pressing the escape key or something, I think it's safe to do most inventory-related things in the close event handler, but I could be wrong. The Bukkit Javadocs I linked does warn that it's "not an exhaustive list".

Regarding the warning about not opening inventories from InventoryClickEvent, do you know if this applies to AnvilGUI.Builder.open? In other words, can I trust that AnvilGUI.Builder.open calls openInventory within a runTask (or similar) call?

I'm pretty sure Builder.open just directly opens the inventory, which may also involve closing any previously opened inventory. If you want to do this from a click event, I would probably wrap it in a task.

[hopperelec]

but I think the rebaking has to happen whenever any listener is registered or unregistered

I get that it would need to rebake, but I thought each event had a distinct handler list meaning the scope of the rebaking would be limited? I honestly have no idea though lol

The Bukkit Javadocs I linked does warn that it's "not an exhaustive list".

I thought that was just referring to the list of methods not to use in InventoryClickEvent?

[0dinD]

I get that it would need to rebake, but I thought each event had a distinct handler list meaning the scope of the rebaking would be limited? I honestly have no idea though lol

Yeah that's a good point, I think you're right about that. But inventory listeners are pretty common among plugins, so I still think the rebaking argument can be valid. Especially if other plugins register multiple inventory listeners.

I thought that was just referring to the list of methods not to use in InventoryClickEvent?

Probably yes, but it's also an indication that some things are undocumented and that inventories are quite finicky in general.

[0dinD]

@hopperelec I just had a look at the Spigot source code and can confirm that it looks like my theory is correct, strange and undefined behavior will happen if you open an inventory from a close event (AnvilGUI's onClose is basically just a wrapper around InventoryCloseEvent) without wrapping it in a scheduled task. More specifically, it looks like the close handler will run before the inventory is actually fully closed, which means that if you open an inventory, it will be closed immediately when the close handler finishes (because Spigot just closes whatever inventory is currently open). That's probably why your event handler isn't firing (and I guess the client still shows the inventory because it doesn't know what has happened). Again, inventories are a bit finicky.

I would argue that regardless, this is a documentation issue on Spigot's side, because even though InventoryClickEvent has the warning, it's not obvious that it also applies to InventoryCloseEvent. Interestingly, Paper has copied the warning to InventoryCloseEvent though: https://jd.papermc.io/paper/1.21.1/org/bukkit/event/inventory/InventoryCloseEvent.html

I'll keep this issue open for the time being because I think AnvilGUI should also warn about this or maybe even schedule the click/close handlers as tasks (though it should probably still be optional).