search

dmulloy2 / ProtocolLib

Provides read and write access to the Minecraft protocol with Bukkit.
GNU General Public License v2.0
1.03k stars 258 forks source link

Fix ConcurrentModificationException when shutting down async listeners #3169

Closed SzczurekYT closed 6 days ago

SzczurekYT commented 1 month ago

This makes a copy of the list before iterating over it so that ConcurrentModificationException isn't thrown

For example the FastLogin plugin calls unregisterAsyncHandlers like this in their onDisabled method

if (getServer().getPluginManager().isPluginEnabled("ProtocolLib")) {
            ProtocolLibrary.getProtocolManager().getAsynchronousManager().unregisterAsyncHandlers(this);
        }

which resulted in this error before the fix

[11:02:00 ERROR]: Error occurred while disabling FastLogin v1.12-SNAPSHOT-4dd6b9a
java.util.ConcurrentModificationException: null
        at java.base/java.util.HashMap$HashIterator.nextNode(HashMap.java:1605) ~[?:?]
        at java.base/java.util.HashMap$ValueIterator.next(HashMap.java:1633) ~[?:?]
        at com.google.common.collect.TransformedIterator.next(TransformedIterator.java:52) ~[guava-32.1.2-jre.jar:?]
        at com.google.common.collect.Iterators$ConcatenatedIterator.hasNext(Iterators.java:1400) ~[guava-32.1.2-jre.jar:?]
        at ProtocolLib.jar/com.comphenix.protocol.async.AsyncFilterManager.unregisterAsyncHandlers(AsyncFilterManager.java:289) ~[ProtocolLib.jar:?]
        at ProtocolLib.jar/com.comphenix.protocol.async.AsyncFilterManager.unregisterAsyncHandlers(AsyncFilterManager.java:283) ~[ProtocolLib.jar:?]
        at FastLoginBukkit.jar/com.github.games647.fastlogin.bukkit.FastLoginBukkit.onDisable(FastLoginBukkit.java:198) ~[FastLoginBukkit.jar:?]
        at org.bukkit.plugin.java.JavaPlugin.setEnabled(JavaPlugin.java:291) ~[paper-api-1.20.6-R0.1-SNAPSHOT.jar:?]
        at io.papermc.paper.plugin.manager.PaperPluginInstanceManager.disablePlugin(PaperPluginInstanceManager.java:237) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
        at io.papermc.paper.plugin.manager.PaperPluginInstanceManager.disablePlugins(PaperPluginInstanceManager.java:161) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
        at io.papermc.paper.plugin.manager.PaperPluginManagerImpl.disablePlugins(PaperPluginManagerImpl.java:97) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
        at org.bukkit.plugin.SimplePluginManager.disablePlugins(SimplePluginManager.java:541) ~[paper-api-1.20.6-R0.1-SNAPSHOT.jar:?]
        at org.bukkit.craftbukkit.CraftServer.disablePlugins(CraftServer.java:595) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
        at net.minecraft.server.MinecraftServer.stopServer(MinecraftServer.java:978) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
        at net.minecraft.server.dedicated.DedicatedServer.stopServer(DedicatedServer.java:842) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
        at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:1273) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
        at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:323) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
        at java.base/java.lang.Thread.run(Thread.java:1583) ~[?:?]

On thing that bothers me though is the : null part of the exception. Is this the correct fix? I don't know, but it works ¯\_(ツ)_/¯

SzczurekYT commented 1 month ago

Hmm Locally the tests also failed (before making the change) so probably something is wrong with master branch.