Fuzss / forgeconfigapiport

NeoForge's & Forge's config systems provided to other modding ecosystems. Designed for a multiloader architecture.
https://www.curseforge.com/minecraft/mc-mods/forge-config-api-port-fabric
Mozilla Public License 2.0
70 stars 10 forks source link

[Bug]: Race condition with unloading server config #26

Closed SquidDev closed 1 year ago

SquidDev commented 1 year ago

Mod Loader (Required)

Fabric

Minecraft Version (Required)

1.19.3

Mod Version (Required)

4.2.6

Notes (Required)

When unloading a world, it's possible for the config file watcher to observe a file update at the same time as the config is cleared. This can cause a race condition where ForgeConfigSpec.isLoaded() returns true at the start of the method, but disappears out under you while reading config values!

I'm afraid I've not spent much time debugging this: my guess is that the config object being saved before unloading is triggering the file watcher. However, I've not observed the behaviour on Forge, so it's very odd!

Reproduction case

Consider the following class body, where onReload is subscribed to a mod's config reload bus.

public static final ForgeConfigSpec configSpec;
private static final ConfigValue<Integer> someConfigOption;

public static void onReload(ModConfig config) {
    LOG.info("Syncing config {}/{} with loaded={}", config.getModId(), config.getType(), spec.isLoaded());
    try {
        if (!configSpec.isLoaded()) return;

        for(int i = 0; i < 1000; i++) someConfigOption.get();
    } finally {
        LOG.info("Done syncing config loaded={}", serverSpec.isLoaded());
    }
}

When exiting a world, one may (it's not deterministic, maybe 1 in 5 times) see the following output:

[18:10:22] [Thread-1/INFO] (ConfigSpec) Syncing config computercraft/SERVER with loaded=true
[18:10:22] [Thread-1/INFO] (ConfigSpec) Done syncing loaded=false
[18:10:22] [Thread-1/INFO] (Minecraft) [STDERR]: java.lang.IllegalStateException: Cannot get config value before config is loaded.
This error is currently only thrown in the development environment, to avoid breaking published mods.
In a future version, this will also throw in the production environment.

[18:10:22] [Thread-1/INFO] (Minecraft) [STDERR]:    at com.google.common.base.Preconditions.checkState(Preconditions.java:502)
[18:10:22] [Thread-1/INFO] (Minecraft) [STDERR]:    at net.minecraftforge.common.ForgeConfigSpec$ConfigValue.get(ForgeConfigSpec.java:952)
[18:10:22] [Thread-1/INFO] (Minecraft) [STDERR]:    at dan200.computercraft.shared.config.ConfigSpec.sync(ConfigSpec.java:420)
[18:10:22] [Thread-1/INFO] (Minecraft) [STDERR]:    at net.minecraftforge.api.fml.event.config.ModConfigEvents$ModConfigEventsHolder.lambda$create$2(ModConfigEvents.java:113)
[18:10:22] [Thread-1/INFO] (Minecraft) [STDERR]:    at net.minecraftforge.fml.config.ConfigFileTypeHandler$ConfigWatcher.run(ConfigFileTypeHandler.java:159)
[18:10:22] [Thread-1/INFO] (Minecraft) [STDERR]:    at com.electronwill.nightconfig.core.file.FileWatcher$WatcherThread.run(FileWatcher.java:181)
[18:10:23] [Render thread/INFO] (ConfigSpec) Syncing config computercraft/SERVER with loaded=false
[18:10:23] [Render thread/INFO] (ConfigSpec) Done syncing config loaded=false

You can see that the config is reloaded on two separate threads, in such a way that ForgeConfigSpec.isLoaded changes from true to false during our method execution!

latest.log (Optional)

No response

Fuzss commented 1 year ago

Does this open Forge PR look like it would fix your issue? https://github.com/MinecraftForge/MinecraftForge/pull/9016

SquidDev commented 1 year ago

That definitely looks like the same bug, yep! I think applying the patch from ConfigTracker would be "good enough" (so swapping save and unload), at least for my needs.

Fuzss commented 1 year ago

I did that in the latest version, hope all works fine now.

SquidDev commented 1 year ago

Sorry, only just got round to updating. This appears to work perfectly, thank you!