Skip to content

Commit 3afe18a

Browse files
quat1024quat1024
committed
Yucky config state machine thing, remove debounce-time hack, ref #52 #21 #3 
1 parent c8d88c7 commit 3afe18a

File tree

10 files changed

+217
-51
lines changed

10 files changed

+217
-51
lines changed

‎.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ Thumbs.db
77

88
## ForgeGradle
99
/run
10+
/runServer
1011

1112
## eclipse
1213
/.settings

‎changelog.txt

+2-1
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
1-
- this update should fix a (not so) rare concurrency issue during modules init when having disabled modules
1+
- this update should fix a (not so) rare concurrency issue during modules init when having disabled modules
2+
- Try and load config files in a more principled way with a lot of verbose logging about when and where things are happening

‎src/main/java/org/violetmoon/zeta/Zeta.java

+29-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package org.violetmoon.zeta;
22

3+
import java.util.function.Supplier;
4+
35
import com.google.common.base.Stopwatch;
46
import net.minecraft.core.BlockPos;
57
import net.minecraft.world.InteractionHand;
@@ -21,15 +23,23 @@
2123
import org.violetmoon.zeta.module.ZetaCategory;
2224
import org.violetmoon.zeta.module.ZetaModuleManager;
2325
import org.violetmoon.zeta.network.ZetaNetworkHandler;
24-
import org.violetmoon.zeta.registry.*;
25-
import org.violetmoon.zeta.util.*;
26+
import org.violetmoon.zeta.registry.BrewingRegistry;
27+
import org.violetmoon.zeta.registry.CraftingExtensionsRegistry;
28+
import org.violetmoon.zeta.registry.DyeablesRegistry;
29+
import org.violetmoon.zeta.registry.PottedPlantRegistry;
30+
import org.violetmoon.zeta.registry.RenderLayerRegistry;
31+
import org.violetmoon.zeta.registry.VariantRegistry;
32+
import org.violetmoon.zeta.registry.ZetaRegistry;
33+
import org.violetmoon.zeta.util.NameChanger;
34+
import org.violetmoon.zeta.util.RaytracingUtil;
35+
import org.violetmoon.zeta.util.RegistryUtil;
36+
import org.violetmoon.zeta.util.ZetaCommonProxy;
37+
import org.violetmoon.zeta.util.ZetaSide;
2638
import org.violetmoon.zeta.util.handler.FuelHandler;
2739
import org.violetmoon.zeta.util.zetalist.IZeta;
2840
import org.violetmoon.zeta.util.zetalist.ZetaList;
2941
import org.violetmoon.zeta.world.EntitySpawnHandler;
3042

31-
import java.util.function.Supplier;
32-
3343
/**
3444
* do not touch forge OR quark from this package, it will later be split off
3545
*/
@@ -41,6 +51,7 @@ public Zeta(String modid, Logger log, ZetaSide side, boolean isProduction) {
4151
this.modid = modid;
4252
this.side = side;
4353
this.isProduction = isProduction; //TODO: either have all these constants or static helpers here or in Utils. Not both
54+
this.proxy = createProxy(side);
4455

4556
this.modules = createModuleManager();
4657
this.registry = createRegistry();
@@ -78,6 +89,7 @@ public Zeta(String modid, Logger log, ZetaSide side, boolean isProduction) {
7889
// Be careful when using this. Load bus will only fire stuff to this zeta events. Play bus however will not as it delegate to forge bus
7990
public final ZetaEventBus<IZetaPlayEvent> playBus; //common mod event bus. Each zeta will have their own object for now but internally they all delegate to the same internal bus
8091
public final ZetaModuleManager modules;
92+
public final ZetaCommonProxy proxy;
8193

8294
//registry
8395
public final ZetaRegistry registry;
@@ -133,6 +145,7 @@ public final void loadModules(@Nullable Iterable<ZetaCategory> categories, @Null
133145

134146
this.configManager = new ConfigManager(this, rootPojo);
135147
this.configInternals = makeConfigInternals(configManager.getRootConfig());
148+
asZeta().log.info("Doing super early config setup for {}", asZeta().modid);
136149
this.configManager.onReload();
137150

138151
this.modules.doFinalize();
@@ -151,6 +164,18 @@ public <T> T modIntegration(String compatWith, Supplier<Supplier<T>> yes, Suppli
151164
}
152165
}
153166

167+
// proxy
168+
public ZetaCommonProxy createProxy(ZetaSide effectiveSide) {
169+
try {
170+
if(effectiveSide == ZetaSide.CLIENT)
171+
return (ZetaCommonProxy) Class.forName("org.violetmoon.zeta.client.ZetaClientProxy")
172+
.getConstructor(Zeta.class).newInstance(this);
173+
else return new ZetaCommonProxy(this);
174+
} catch (Exception e) {
175+
throw new RuntimeException("Failed to construct proxy", e);
176+
}
177+
}
178+
154179
// config
155180
public abstract IZetaConfigInternals makeConfigInternals(SectionDefinition rootSection);
156181

‎src/main/java/org/violetmoon/zeta/client/ZetaClientProxy.java

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package org.violetmoon.zeta.client;
2+
3+
import net.minecraft.client.Minecraft;
4+
import org.violetmoon.zeta.Zeta;
5+
import org.violetmoon.zeta.util.ZetaCommonProxy;
6+
7+
public class ZetaClientProxy extends ZetaCommonProxy {
8+
public ZetaClientProxy(Zeta zeta) {
9+
super(zeta);
10+
}
11+
12+
@Override
13+
public void tryToExecuteOnMainThread(Runnable runnable) {
14+
Minecraft.getInstance().execute(runnable);
15+
}
16+
}

‎src/main/java/org/violetmoon/zeta/config/IZetaConfigInternals.java

-5
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,4 @@ public interface IZetaConfigInternals {
55
<T> void set(ValueDefinition<T> definition, T value);
66

77
void flush();
8-
9-
//for debouncing Forge ConfigChangedEvent cause its glitchy af
10-
default long debounceTime() {
11-
return 0;
12-
}
138
}

‎src/main/java/org/violetmoon/zeta/util/ZetaCommonProxy.java

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package org.violetmoon.zeta.util;
2+
3+
import net.minecraft.server.MinecraftServer;
4+
import net.minecraftforge.server.ServerLifecycleHooks;
5+
import org.violetmoon.zeta.Zeta;
6+
7+
public class ZetaCommonProxy {
8+
public ZetaCommonProxy(Zeta zeta) {
9+
this.zeta = zeta;
10+
}
11+
12+
protected Zeta zeta;
13+
14+
/**
15+
* Try and execute something on "the main thread". On the physical client this is well-defined (the render thread),
16+
* but on the physical server... make a best-effort to use the server thread, and otherwise yolo
17+
*/
18+
public void tryToExecuteOnMainThread(Runnable runnable) {
19+
MinecraftServer server = ServerLifecycleHooks.getCurrentServer();
20+
if(server != null)
21+
server.execute(runnable);
22+
else {
23+
zeta.log.warn("Using thread '{}' instead of the server thread", Thread.currentThread().getName());
24+
runnable.run();
25+
}
26+
}
27+
}

‎src/main/java/org/violetmoon/zetaimplforge/ForgeZeta.java

+7-2
Original file line numberDiff line numberDiff line change
@@ -149,9 +149,14 @@ public void start() {
149149
//load
150150
IEventBus modbus = FMLJavaModLoadingContext.get().getModEventBus();
151151

152-
modbus.addListener(EventPriority.LOWEST, CreativeTabManager::buildContents);
153-
modbus.addListener(ConfigEventDispatcher::configChanged);
152+
//hook up config events
153+
ConfigEventDispatcher configEventDispatcher = new ConfigEventDispatcher(this);
154+
modbus.addListener(configEventDispatcher::modConfigReloading);
155+
modbus.addListener(configEventDispatcher::commonSetup);
156+
MinecraftForge.EVENT_BUS.addListener(configEventDispatcher::serverAboutToStart);
154157

158+
//other stuff
159+
modbus.addListener(EventPriority.LOWEST, CreativeTabManager::buildContents);
155160
modbus.addListener(EventPriority.HIGHEST, this::registerHighest);
156161
}
157162

‎src/main/java/org/violetmoon/zetaimplforge/config/ConfigEventDispatcher.java

+134-24
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,146 @@
11
package org.violetmoon.zetaimplforge.config;
22

3+
import java.util.concurrent.atomic.AtomicInteger;
4+
5+
import net.minecraftforge.event.server.ServerAboutToStartEvent;
36
import net.minecraftforge.fml.event.config.ModConfigEvent;
7+
import net.minecraftforge.fml.event.lifecycle.FMLCommonSetupEvent;
48
import org.violetmoon.zeta.Zeta;
5-
import org.violetmoon.zeta.event.load.ZConfigChanged;
6-
import org.violetmoon.zeta.util.zetalist.ZetaList;
9+
import org.violetmoon.zeta.util.ZetaSide;
710
import org.violetmoon.zetaimplforge.event.load.ForgeZConfigChange;
811

912
public class ConfigEventDispatcher {
10-
public static void configChanged(ModConfigEvent event) {
11-
for(Zeta z : ZetaList.INSTANCE.getZetas()) {
12-
String modid = z.modid;
13-
if(!event.getConfig().getModId().equals(modid) || z.configInternals == null)
14-
continue;
15-
16-
// https://github.com/VazkiiMods/Quark/commit/b0e00864f74539d8650cb349e88d0302a0fda8e4
17-
// "The Forge config api writes to the config file on every single change
18-
// to the config, which would cause the file watcher to trigger
19-
// a config reload while the config gui is committing changes."
20-
//TODO: investigate. this doesnt look that sound
21-
if(System.currentTimeMillis() - z.configInternals.debounceTime() > 20)
22-
handleConfigChange(z);
13+
public ConfigEventDispatcher(Zeta z) {
14+
this.z = z;
15+
}
16+
17+
private final Zeta z;
18+
19+
// The story is:
20+
// - Forge dispatches ModConfigEvent.Reloading directly from the NightConfig filewatcher thread
21+
// (which means it can be received multiple times in a row, as the file save settles)
22+
// (and also, updating our data structures/event bus ("refreshing the config") becomes
23+
// a thread safety hazard)
24+
// - Forge seemingly dispatches ModConfigEvent.Reloading for fun sometimes
25+
// (including in the middle of other modloading events, and 3-4 times during FMLCommonSetupEvent#enqueueWork)
26+
// - We call dispatchAllInitialLoads from FMLCommonSetupEvent#enqueueWork
27+
// (on the server, this at least reliably gets us onto a thread called 'main'?
28+
// who's thread is that? idk)
29+
// - We don't always have access to a "server thread" because this all can run before the server starts!
30+
//
31+
// To try and untangle this mess, here's what we do on the client:
32+
// - All configs start in the NOT_READY state.
33+
// - In FMLCommonSetupEvent#enqueueWork we refresh the config once on the enqueueWork thread
34+
// and transition to ACCEPT_FILE_RELOADS.
35+
//
36+
// here's what we do on the server:
37+
// - All configs start in the NOT_READY state.
38+
// - In FMLCommonSetupEvent#enqueueWork we refresh the config once on the enqueueWork thread
39+
// and transition to WAITING_FOR_SERVER_START.
40+
// - In ServerAboutToStartEvent we transition to ACCEPT_FILE_RELOADS.
41+
//
42+
// In ModConfigEvent.Reloading, we:
43+
// - try to find a better thread (which on the client is the render thread, and on the server,
44+
// since we wait for the server to start, is hopefully the server thread)
45+
// - if the config is in ACCEPT_FILE_RELOADS: transition to BUSY, refresh the config, and
46+
// transition back to ACCEPT_FILE_RELOADS.
47+
// - otherwise just drop the request to refresh the config on the floor.
48+
// (It's too early, or it's reentrant, and that will cause problems.)
49+
// This also removes an ugly "debounce-time" hack where we'd just ignore ModConfigEvents
50+
// if enough time hadn't passed since the last one. You lose the ability to reliably refresh
51+
// the config *while* the server is starting, but like, that's fine.
52+
//
53+
// If this is wrong/incorrect/causing problems, BLAME QUAT
54+
//
55+
// Parallel Mod Loading Considered Harmful episode 309128313
56+
57+
//leaving this comment here but the statemachine seems to subsume the old debounce-time hack
58+
// https://github.com/VazkiiMods/Quark/commit/b0e00864f74539d8650cb349e88d0302a0fda8e4
59+
// "The Forge config api writes to the config file on every single change
60+
// to the config, which would cause the file watcher to trigger
61+
// a config reload while the config gui is committing changes."
62+
63+
private static final int BEFORE_INIT = 0;
64+
private static final int WAITING_FOR_SERVER_START = 1;
65+
private static final int ACCEPT_FILE_RELOADS = 2;
66+
private static final int BUSY = 3;
67+
68+
private final AtomicInteger state = new AtomicInteger(BEFORE_INIT);
69+
70+
public void commonSetup(FMLCommonSetupEvent event) {
71+
event.enqueueWork(() -> {
72+
z.log.info("Common setup: Performing initial refresh of {}'s config on thread '{}'", z.modid, Thread.currentThread().getName());
73+
74+
z.configManager.onReload();
75+
z.loadBus.fire(new ForgeZConfigChange());
76+
77+
int oldState;
78+
if(z.side == ZetaSide.CLIENT) {
79+
//on the client we always have a render thread, we can start accepting config refreshes now
80+
oldState = state.getAndSet(ACCEPT_FILE_RELOADS);
81+
z.log.info("{}'s config is now ready to accept filewatcher changes", z.modid);
82+
} else {
83+
//on the server we want to wait for the existence of the server thread to process config refreshes on
84+
oldState = state.getAndSet(WAITING_FOR_SERVER_START);
85+
z.log.info("Waiting for server start before accepting filewatcher changes to {}'s config", z.modid);
86+
}
87+
88+
if(oldState != BEFORE_INIT)
89+
z.log.warn("Common setup: {}'s config was previously in state '{}'... weird, but trying to continue. Report this.", z.modid, fmtState(oldState));
90+
});
91+
}
92+
93+
public void serverAboutToStart(ServerAboutToStartEvent e) {
94+
if(z.side == ZetaSide.SERVER) {
95+
z.log.info("Server starting, accepting filewatcher changes for {}'s config", z.modid);
96+
97+
int oldState = state.getAndSet(ACCEPT_FILE_RELOADS);
98+
if(oldState != WAITING_FOR_SERVER_START)
99+
z.log.warn("Server start: {}'s config was previously in state '{}'... weird, but trying to continue. Report this.", z.modid, fmtState(oldState));
23100
}
24101
}
25-
26-
public static void dispatchAllInitialLoads() {
27-
for(Zeta z : ZetaList.INSTANCE.getZetas())
28-
handleConfigChange(z);
102+
103+
public void modConfigReloading(ModConfigEvent.Reloading event) {
104+
String modid = z.modid;
105+
if(!event.getConfig().getModId().equals(modid))
106+
return;
107+
108+
if(z.configManager == null || z.configInternals == null) {
109+
z.log.info("Ignoring request to refresh {}'s config WAY too early", z.modid);
110+
return;
111+
}
112+
113+
z.log.info("About to refresh {}'s config, looking for better thread than '{}'...", z.modid, Thread.currentThread().getName());
114+
z.proxy.tryToExecuteOnMainThread(() -> {
115+
//if the state is ACCEPT_FILE_RELOADS, transition it to BUSY and continue
116+
//otherwise drop the request to refresh this config; we might be waiting for
117+
//FMLCommonSetup/ServerAboutToStart, or it might currently be in the process of refreshing
118+
int oldState = state.compareAndExchange(ACCEPT_FILE_RELOADS, BUSY);
119+
if(oldState != ACCEPT_FILE_RELOADS) {
120+
z.log.info("{}'s config is '{}', ignoring config refresh. Current thread: {}", z.modid, fmtState(oldState), Thread.currentThread().getName());
121+
return;
122+
} else
123+
z.log.info("Refreshing {}'s config on thread '{}'", z.modid, Thread.currentThread().getName());
124+
125+
try {
126+
//actually refresh the config
127+
z.configManager.onReload();
128+
z.loadBus.fire(new ForgeZConfigChange());
129+
} finally {
130+
//ready for another file reload
131+
z.log.info("All done refreshing {}'s config", z.modid);
132+
state.setRelease(ACCEPT_FILE_RELOADS);
133+
}
134+
});
29135
}
30-
31-
private static void handleConfigChange(Zeta z) {
32-
z.configManager.onReload();
33-
z.loadBus.fire(new ForgeZConfigChange());
136+
137+
private static String fmtState(int state) {
138+
return switch(state) {
139+
case BEFORE_INIT -> "BEFORE_INIT";
140+
case WAITING_FOR_SERVER_START -> "WAITING_FOR_SERVER_START";
141+
case ACCEPT_FILE_RELOADS -> "ACCEPT_FILE_RELOADS";
142+
case BUSY -> "BUSY";
143+
default -> "weird unknown state " + state + "???";
144+
};
34145
}
35-
36146
}

‎src/main/java/org/violetmoon/zetaimplforge/config/ForgeBackedConfig.java

+1-9
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212

1313
public class ForgeBackedConfig implements IZetaConfigInternals {
1414
private final Map<ValueDefinition<?>, ForgeConfigSpec.ConfigValue<?>> definitionsToValues = new HashMap<>();
15-
private long debounceTime = System.currentTimeMillis();
1615

1716
public ForgeBackedConfig(SectionDefinition rootSection, ForgeConfigSpec.Builder forgeBuilder) {
1817
walkSection(rootSection, forgeBuilder, true);
@@ -57,20 +56,13 @@ public <T> T get(ValueDefinition<T> definition) {
5756
@SuppressWarnings("unchecked")
5857
public <T> void set(ValueDefinition<T> definition, T value) {
5958
ForgeConfigSpec.ConfigValue<T> forge = (ForgeConfigSpec.ConfigValue<T>) definitionsToValues.get(definition);
60-
debounceTime = System.currentTimeMillis();
6159
forge.set(value);
6260
}
6361

6462
@Override
6563
public void flush() {
66-
debounceTime = 0; //force ConfigChangedEvent to not debounce this, it's important
67-
6864
//just pick one; they all point to the same FileConfig anyway
65+
//this dispatches a forge ModConfigEvent.Reloading
6966
definitionsToValues.values().iterator().next().save();
7067
}
71-
72-
@Override
73-
public long debounceTime() {
74-
return debounceTime;
75-
}
7668
}

‎src/main/java/org/violetmoon/zetaimplforge/mod/ZetaModCommonProxy.java

-6
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ public class ZetaModCommonProxy {
5252

5353
public void registerEvents(Zeta zeta) {
5454
IEventBus bus = FMLJavaModLoadingContext.get().getModEventBus();
55-
bus.addListener(this::onSetup);
5655

5756
zeta.loadBus
5857
.subscribe(RecipeCrawlHandler.class)
@@ -72,11 +71,6 @@ public void registerEvents(Zeta zeta) {
7271

7372
}
7473

75-
76-
public void onSetup(FMLCommonSetupEvent event) {
77-
event.enqueueWork(ConfigEventDispatcher::dispatchAllInitialLoads);
78-
}
79-
8074
public void addKnownZetaLoadEvents(ForgeEventsRemapper<IZetaLoadEvent, Event> r) {
8175

8276
//TODO: repace all these with explicit ones

0 commit comments

Comments
 (0)