search

neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.05k stars 140 forks source link

ModConfigEvent can cause concurrency issues in mods #40

Closed embeddedt closed 6 days ago

embeddedt commented 12 months ago

Forge fires ModConfigEvent.Loading when a config is first loaded, and ModConfigEvent.Reloading when a config reloads as a result of the file watcher detecting a change. However, there is no locking done in the function that posts these events, meaning that it is possible for both Loading and Reloading to fire simultaneously.

Many mods have ModConfigEvent event handlers that are not protected against this, and will therefore occasionally throw CMEs. This is technically a bug in those mods but I also think the design of this event is confusing/misleading - it really does not make sense for a config to load and reload at the exact same time.

https://github.com/ExcessiveAmountsOfZombies/Croptopia/issues/517 is an example of this occuring in the wild; I patched it in ModernFix by wrapping the Forge function that posts the event with a wrapper that synchronizes on the underlying config object, ensuring that all mods will never see two config events posted concurrently.

TheCurle commented 12 months ago

Interesting. This should definitely not be possible. We should look into rethinking how we fire the reload events, there seem to be several unresolved issues.

Thanks for looking into and reporting this.

Technici4n commented 6 months ago

Sadly the pastes from https://github.com/ExcessiveAmountsOfZombies/Croptopia/issues/517 have expired so I cannot verify the stack trace anymore. I have an idea for the fix though.