DiscordSRV

DiscordSRV

Bukkit Plugins
86.8k Downloads

Plugin incompatibility, probably caused by CancellationDetector

blablubbabc opened this issue ยท 2 comments

commented

Expected behavior

I am not using the DiscordSRV plugin myself, but a user of one of my plugins (Shopkeepers) ran into this problem. After a rough look through the source code of DiscordSRV I might have identified the cause of the issue. Since I am not running the DiscordSRV plugin myself (i.e. I have not run a server with DiscordSRV, or reproduced the issue myself), some of the checks and fields of this issue form do not apply to me. I will try to fill them in as best as I can with the information that has been provided to me, but will then link this issue to the affected user, so that if you require any further information, they can hopefully complete any missing details.

Backstory:
In order to resolve certain other plugin incompatibilities, the Shopkeepers plugin (v2.14.0) reorders registered event listeners by first unregistering, and then re-registering them. Source code: https://github.com/Shopkeepers/Shopkeepers/blob/master/modules/main/src/main/java/com/nisovin/shopkeepers/util/bukkit/EventUtils.java#L275

When using the DiscordSRV plugin with a DebugLevel of 1, the user runs into the error below. I suspect that any DiscordSRV configuration that enables the MINECRAFT_TO_DISCORD debug option will trigger this issue.

Actual behavior

Log excerpt of the affected user:

[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Enabling Shopkeepers v2.14.0
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Config already loaded.
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Language file already loaded.
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] MC 1.16 exclusive features are enabled.
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] MC 1.17 exclusive features are enabled.
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Spigot-based server found: Enabling Spigot exclusive features.
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Testing server assumptions ...
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Server assumption tests passed (32.13 ms).
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Defaults already registered.
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Moving a handler for event 'PlayerInteractEntityEvent' at priority LOWEST in front of an event handler of plugin Towny
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Moving a handler for event 'PlayerInteractEvent' at priority LOWEST in front of an event handler of plugin mcMMO
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Moving a handler for event 'PlayerInteractEvent' at priority LOWEST in front of an event handler of plugin CoreProtect
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Moving a handler for event 'PlayerInteractEvent' at priority LOWEST in front of an event handler of plugin Towny
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Citizen shops enabled, but Citizens plugin not found or disabled.
[25.12 10:55:21] [Server] [INFO] [Shopkeepers] Moving a handler for event 'AsyncPlayerChatEvent' at priority LOWEST in front of an event handler of plugin Essentials
[25.12 10:55:21] [Server] [ERROR] Error occurred while enabling Shopkeepers v2.14.0 (Is it up to date?)
[25.12 10:55:21] [Server] java.lang.IllegalStateException This listener is already registered to priority LOWEST
[25.12 10:55:21] [Server]     at org.bukkit.event.HandlerList.register(HandlerList.java:113) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
[25.12 10:55:21] [Server]     at com.nisovin.shopkeepers.util.bukkit.EventUtils.enforceExecuteFirst(EventUtils.java:276) ~[Shopkeepers-2.14.0.jar:?]
[25.12 10:55:21] [Server]     at com.nisovin.shopkeepers.util.bukkit.EventUtils.enforceExecuteFirst(EventUtils.java:192) ~[Shopkeepers-2.14.0.jar:?]
[25.12 10:55:21] [Server]     at com.nisovin.shopkeepers.util.bukkit.EventUtils.enforceExecuteFirst(EventUtils.java:145) ~[Shopkeepers-2.14.0.jar:?]
[25.12 10:55:21] [Server]     at com.nisovin.shopkeepers.chatinput.ChatInput.onEnable(ChatInput.java:62) ~[Shopkeepers-2.14.0.jar:?]
[25.12 10:55:21] [Server]     at com.nisovin.shopkeepers.SKShopkeepersPlugin.onEnable(SKShopkeepersPlugin.java:327) ~[Shopkeepers-2.14.0.jar:?]
[25.12 10:55:21] [Server]     at org.bukkit.plugin.java.JavaPlugin.setEnabled(JavaPlugin.java:264) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
[25.12 10:55:21] [Server]     at org.bukkit.plugin.java.JavaPluginLoader.enablePlugin(JavaPluginLoader.java:370) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
[25.12 10:55:21] [Server]     at org.bukkit.plugin.SimplePluginManager.enablePlugin(SimplePluginManager.java:500) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
[25.12 10:55:21] [Server]     at org.bukkit.craftbukkit.v1_18_R1.CraftServer.enablePlugin(CraftServer.java:564) ~[paper-1.18.1.jar:git-Paper-101]
[25.12 10:55:21] [Server]     at org.bukkit.craftbukkit.v1_18_R1.CraftServer.enablePlugins(CraftServer.java:478) ~[paper-1.18.1.jar:git-Paper-101]
[25.12 10:55:21] [Server]     at net.minecraft.server.MinecraftServer.loadWorld0(MinecraftServer.java:727) ~[paper-1.18.1.jar:git-Paper-101]

I.e. the Shopkeepers plugin unregisters a RegisteredListener, and then receives this error ("already registered") when it tries to immediately re-register it.

The issue seems to only affect the reordering of registered event handlers for the AsyncPlayerChatEvent.

Steps to reproduce

Run Shopkeepers, Essentials, and DiscordSRV with the above mentioned configuration at the same time.

Server software and version

Paper 101 1.18.1

Checks

  • [] I am not using an outdated version of DiscordSRV.
  • [] I asked in DiscordSRV's Discord server to see whether this issue is in fact a bug that needs to be fixed.

Anything else

My guess is that this issue is caused by the CancellationDetector class. This class is only used when debug option MINECRAFT_TO_DISCORD is active, and it only affects the AsyncPlayerChatEvent.

My guess on what is happening:

Additional related problems:

  • The add implementation always wraps the given listener. However, as this issue shows, the given listener might itself already be a DelegatedRegisteredListener. To not wrap it twice, and then also invoke your 'event-cancelled' callback twice, you will probably want to not wrap the given listener again in this case.
    And you will probably also have to store the unwrapped listener inside the backup map, so that all traces of DelegatedRegisteredListener are gone once the backup is applied.
    Similar in remove: If the given listener is already a DelegatedRegisteredListener, you probably need to remove the wrapped original listener from the backup.
  • A more minor (because there are probably not many plugins affected by this) but general problem with dynamically wrapping already registered RegisteredListeners: Plugins may create and hold references to their own RegisteredListener instances. Any equality checks between these RegisteredListeners with registered listeners retrieved from HanderLists will fail.
    These comparisons do not necessarily need to be done by the affected plugins themselves in order for them to run into this issue. For example, HandlerList#register(RegisteredListener) calls the contains method on the backing list that stores the registered listeners. This contains method will then do the equality comparison.
    An example of a plugin holding a reference to a RegisteredListener and doing equality checks with it, or calling the register method, is the DiscordSRV plugin itself: https://github.com/DiscordSRV/DiscordSRV/blob/develop/src/main/java/github/scarsz/discordsrv/modules/alerts/AlertListener.java#L175
    Even if your custom backing-list implementation is updated to account for the contains method, there is no guarantee that there are not other occurences, now or in the future, that would also perform these kinds of object comparisons. And it doesn't account for comparisons done by plugins themselves (like DiscordSRV itself is currently doing them). However, I don't have a good idea for how this issue could be avoiding in general. Maybe some reflection trickery could be used to ensure that HandlerList never exposes those custom wrapper instances. But that doesn't sound like a good solution and might have its own issues (e.g. plugins would no longer be able to inspect/invoke/etc. the actually registered/used RegisteredListeners).
commented

Thank you for your detailed bug report. I have addressed the concerns regarding the CancellationDetector by entirely rewriting it in a way that leaves the original RegisteredListeners unaffected, and instead adds it's own listeners around the regular listeners. Those listeners are also never stored (and cannot be added) in/to the list so List#contains will always return false and the Exception regarding the listener already being there cannot occur. Incase you need a development version, they're available here: https://snapshot.discordsrv.com

commented

Thank you. I haven't tested these changes, but I assume that this should indeed resolve the issue.