MysticMods / MysticalWorld

Library mod for use with the Mystic Mods (Embers, Roots, Gadgetry, Mystic World, etc.)
MIT License
27 stars 30 forks source link

[1.15.2] Mystical World init isn't thread safe. #178

Closed Vaelzan closed 4 years ago

Vaelzan commented 4 years ago

General Information

Describe the bug: In https://github.com/MysticMods/MysticalWorld/blob/1.15/src/main/java/epicsquid/mysticalworld/setup/ModSetup.java you should be aware that the common setup event is run in parallel by Forge. So is the client setup event. You're modifying compostables, potions, and world generation there (which aren't thread-safe objects), so it should be called inside a DeferredWorkQueue instead of just being run directly.

Similarly, https://github.com/MysticMods/MysticalWorld/blob/1.15/src/main/java/epicsquid/mysticalworld/setup/ClientSetup.java should do the same for at least the spawn eggs, but possibly other registration, since client setup is also run in parallel.

There is an example of how to use DeferredWorkQueue in my own mod, here: https://github.com/Vaelzan/Scenic/blob/3c0e95e8a594dc27d688781f913a9b9baf7602a3/src/main/java/net/evilcult/scenic/Scenic.java#L78

It just causes the registration to happen on the main thread in series rather than parallel. It will prevent concurrency-related crashes during both common and client setup phases.

To Reproduce: It's a concurrency issue, so it will appear pretty much at random, but particularly in a large mod pack where other mods may also not be thread safe (there are several big ones that still aren't, but it's taking me a while to get to all of them).

Expected behavior: Thread-safe registration. 😄

Environment Versions

Mystic Mods Versions

Other Versions:


Logging Information

Crash Report (if available): https://gist.github.com/Vaelzan/4a6c79422ab8657d704d2b3be1d7b6b5

Additional context (optional): Reference: https://mcforge.readthedocs.io/en/latest/conventions/loadstages/

Note: This also impacts 1.14, and you should be aware of it if porting to 1.16.

noobanidus commented 4 years ago

Didn't reference it when committing, but this is resolved in 1.15. I'll retro-fit to 1.14.

noobanidus commented 4 years ago

This should be resolved with the latest update.

Vaelzan commented 4 years ago

As always, I appreciate the prompt fixes, nooby! :)