Infamous-Misadventures / Dungeons-Mobs

A mod that ports the mobs from Minecraft: Dungeons to Minecraft: Java Edition.
MIT License
25 stars 15 forks source link

[1.19.2] Item model properties registration must be enqueued #182

Open XFactHD opened 1 year ago

XFactHD commented 1 year ago

I noticed this while looking at the log of a user in the Forge Discord. The data structures used by ItemProperties are not thread-safe and as such must be enqueued to run sequentially through FMLClientSetupEvent#enqueueWork(), otherwise you may get a ConcurrentModificationException when at least one other mod makes the same mistake.

Example stacktrace ``` [17:53:02] [Worker-Main-2/ERROR] [ne.mi.fm.ja.FMLModContainer/LOADING]: Caught exception during event FMLClientSetupEvent dispatch for modid dungeons_mobs java.util.ConcurrentModificationException: null at java.util.HashMap.computeIfAbsent(HashMap.java:1221) ~[?:?] {re:mixin} at net.minecraft.client.renderer.item.ItemProperties.register(ItemProperties.java:53) ~[client-1.19.2-20220805.130853-srg.jar#336!/:?] {re:classloading,pl:accesstransformer:B,pl:runtimedistcleaner:A} at com.infamous.dungeons_mobs.client.ModItemModelProperties.(ModItemModelProperties.java:10) ~[dungeons_mobs-1.19.2-4.0.6-beta.jar#266!/:1.19.2-4.0.6-beta] {re:classloading} at com.infamous.dungeons_mobs.DungeonsMobs.doClientStuff(DungeonsMobs.java:92) ~[dungeons_mobs-1.19.2-4.0.6-beta.jar#266!/:1.19.2-4.0.6-beta] {re:classloading} at net.minecraftforge.eventbus.EventBus.doCastFilter(EventBus.java:260) ~[eventbus-6.0.3.jar#129!/:?] {} at net.minecraftforge.eventbus.EventBus.lambda$addListener$11(EventBus.java:252) ~[eventbus-6.0.3.jar#129!/:?] {} at net.minecraftforge.eventbus.EventBus.post(EventBus.java:315) ~[eventbus-6.0.3.jar#129!/:?] {} at net.minecraftforge.eventbus.EventBus.post(EventBus.java:296) ~[eventbus-6.0.3.jar#129!/:?] {} at net.minecraftforge.fml.javafmlmod.FMLModContainer.acceptEvent(FMLModContainer.java:107) ~[javafmllanguage-1.19.2-43.2.0.jar#338!/:?] {} at net.minecraftforge.fml.ModContainer.lambda$buildTransitionHandler$10(ModContainer.java:122) ~[fmlcore-1.19.2-43.2.0.jar#337!/:?] {} at java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1804) [?:?] {} at java.util.concurrent.CompletableFuture$AsyncRun.exec(CompletableFuture.java:1796) [?:?] {} at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373) [?:?] {} at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182) [?:?] {} at java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655) [?:?] {re:computing_frames} at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622) [?:?] {re:computing_frames} at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165) [?:?] {} ```

Forge version: 1.19.2-43.2.0 Dungeons Mobs version: 1.19.2-4.0.6-beta


Reading the involved code also makes me question why a constructor is used for the registration instead of a static method, considering the event bus registration of that instance seems to be completely pointless (https://github.com/Infamous-Misadventures/Dungeons-Mobs/blob/1.19/src/main/java/com/infamous/dungeons_mobs/DungeonsMobs.java#L92).

Thelnfamous1 commented 1 year ago

You're right. We had a similar issue in Dungeons Gear that was fixed. I had no idea the same problem was also present here.

As for why a constructor gets registered to the event bus, I...don't know what to tell you. I had the same reaction when I saw that in Dungeons Gear. I'll have to look back at the commit history and see who added that.

Patrigan commented 1 year ago

Fixed for 1.19.2 in next version. Need to backport

Thelnfamous1 commented 1 year ago

Fixed in 4.0.7-beta for 1.19.2.