From cd7eafe5fd3a9d0c2864e9b7f0ccf83aa62a98a7 Mon Sep 17 00:00:00 2001 From: Technici4n <13494793+Technici4n@users.noreply.github.com> Date: Thu, 11 Jul 2024 00:36:17 +0200 Subject: [PATCH] Config cleanup, part 2 (#178) * Rewrite config read/write logic to be more straightforward * Remove config tracking from the `ModContainer` itself --- .../java/net/neoforged/fml/ModContainer.java | 15 +- .../neoforged/fml/config/ConfigTracker.java | 146 ++++++++++-------- .../neoforged/fml/config/ConfigWatcher.java | 14 +- .../net/neoforged/fml/config/IConfigSpec.java | 20 ++- .../neoforged/fml/config/LoadedConfig.java | 26 ++++ .../net/neoforged/fml/config/ModConfig.java | 37 ++--- .../fml/config/ConfigTrackerTest.java | 5 +- 7 files changed, 147 insertions(+), 116 deletions(-) create mode 100644 loader/src/main/java/net/neoforged/fml/config/LoadedConfig.java diff --git a/loader/src/main/java/net/neoforged/fml/ModContainer.java b/loader/src/main/java/net/neoforged/fml/ModContainer.java index c1f69b77..b3337840 100644 --- a/loader/src/main/java/net/neoforged/fml/ModContainer.java +++ b/loader/src/main/java/net/neoforged/fml/ModContainer.java @@ -7,7 +7,6 @@ import static net.neoforged.fml.Logging.LOADING; -import java.util.EnumMap; import java.util.IdentityHashMap; import java.util.Map; import java.util.Optional; @@ -21,7 +20,6 @@ import net.neoforged.fml.config.ModConfig; import net.neoforged.fml.event.IModBusEvent; import net.neoforged.fml.event.lifecycle.FMLConstructModEvent; -import net.neoforged.fml.loading.FMLPaths; import net.neoforged.neoforgespi.language.IModInfo; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -48,7 +46,6 @@ public abstract class ModContainer { protected final String namespace; protected final IModInfo modInfo; protected final Map, Supplier> extensionPoints = new IdentityHashMap<>(); - protected final EnumMap configs = new EnumMap<>(ModConfig.Type.class); public ModContainer(IModInfo info) { this.modId = info.getModId(); @@ -95,14 +92,6 @@ public void registerExtensionPoint(Class point, S extensionPoints.put(point, extension); } - private void addConfig(final ModConfig modConfig) { - configs.put(modConfig.getType(), modConfig); - - if (modConfig.getType() == ModConfig.Type.STARTUP) { - ConfigTracker.INSTANCE.openConfig(modConfig, FMLPaths.CONFIGDIR.get(), null); - } - } - /** * Adds a {@link ModConfig} with the given type and spec. An empty config spec will be ignored and a debug line will * be logged. @@ -117,7 +106,7 @@ public void registerConfig(ModConfig.Type type, IConfigSpec configSpec) { return; } - addConfig(ConfigTracker.INSTANCE.registerConfig(type, configSpec, this)); + ConfigTracker.INSTANCE.registerConfig(type, configSpec, this); } /** @@ -134,7 +123,7 @@ public void registerConfig(ModConfig.Type type, IConfigSpec configSpec, String f return; } - addConfig(ConfigTracker.INSTANCE.registerConfig(type, configSpec, this, fileName)); + ConfigTracker.INSTANCE.registerConfig(type, configSpec, this, fileName); } /** diff --git a/loader/src/main/java/net/neoforged/fml/config/ConfigTracker.java b/loader/src/main/java/net/neoforged/fml/config/ConfigTracker.java index ab513898..39ec586c 100644 --- a/loader/src/main/java/net/neoforged/fml/config/ConfigTracker.java +++ b/loader/src/main/java/net/neoforged/fml/config/ConfigTracker.java @@ -6,20 +6,27 @@ package net.neoforged.fml.config; import com.electronwill.nightconfig.core.CommentedConfig; -import com.electronwill.nightconfig.core.ConfigFormat; -import com.electronwill.nightconfig.core.file.CommentedFileConfig; +import com.electronwill.nightconfig.core.InMemoryCommentedFormat; +import com.electronwill.nightconfig.core.UnmodifiableCommentedConfig; +import com.electronwill.nightconfig.core.concurrent.ConcurrentCommentedConfig; +import com.electronwill.nightconfig.core.concurrent.SynchronizedConfig; import com.electronwill.nightconfig.core.file.FileWatcher; import com.electronwill.nightconfig.core.io.ParsingException; +import com.electronwill.nightconfig.core.io.ParsingMode; import com.electronwill.nightconfig.core.io.WritingMode; import com.electronwill.nightconfig.toml.TomlFormat; +import com.electronwill.nightconfig.toml.TomlParser; +import com.electronwill.nightconfig.toml.TomlWriter; import com.mojang.logging.LogUtils; import java.io.ByteArrayInputStream; import java.io.IOException; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.util.ArrayList; import java.util.Collections; import java.util.EnumMap; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; @@ -27,6 +34,7 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.ReentrantLock; +import java.util.function.Function; import net.neoforged.fml.ModContainer; import net.neoforged.fml.event.config.ModConfigEvent; import net.neoforged.fml.loading.FMLConfig; @@ -80,6 +88,11 @@ public ModConfig registerConfig(ModConfig.Type type, IConfigSpec spec, ModContai var lock = locksByMod.computeIfAbsent(container.getModId(), m -> new ReentrantLock()); var modConfig = new ModConfig(type, spec, container, fileName, lock); trackConfig(modConfig); + + if (modConfig.getType() == ModConfig.Type.STARTUP) { + openConfig(modConfig, FMLPaths.CONFIGDIR.get(), null); + } + return modConfig; } @@ -113,29 +126,19 @@ public void unloadConfigs(ModConfig.Type type) { this.configSets.get(type).forEach(ConfigTracker::closeConfig); } - public static void openConfig(ModConfig config, Path configBasePath, @Nullable Path configOverrideBasePath) { + static void openConfig(ModConfig config, Path configBasePath, @Nullable Path configOverrideBasePath) { LOGGER.trace(CONFIG, "Loading config file type {} at {} for {}", config.getType(), config.getFileName(), config.getModId()); - if (config.config != null) { - LOGGER.warn("Opening a config that was already loaded with value {} at path {}", config.config, config.getFileName()); + if (config.loadedConfig != null) { + LOGGER.warn("Opening a config that was already loaded with value {} at path {}", config.loadedConfig, config.getFileName()); } var basePath = resolveBasePath(config, configBasePath, configOverrideBasePath); var configPath = basePath.resolve(config.getFileName()); - var configData = CommentedFileConfig.builder(configPath) - .sync() - .preserveInsertionOrder() - .autosave() - .onFileNotFound((newfile, configFormat) -> setupConfigFile(config, newfile, configFormat)) - .writingMode(WritingMode.REPLACE_ATOMIC) - .build(); - LOGGER.debug(CONFIG, "Built TOML config for {}", configPath); - - loadConfig(config, configData); + + loadConfig(config, configPath, ModConfigEvent.Loading::new); LOGGER.debug(CONFIG, "Loaded TOML config file {}", configPath); - config.config = configData; - config.postConfigEvent(ModConfigEvent.Loading::new); if (!FMLConfig.getBoolConfigValue(FMLConfig.ConfigValue.DISABLE_CONFIG_WATCHER)) { - FileWatcher.defaultInstance().addWatch(configPath, new ConfigWatcher(config, configData, Thread.currentThread().getContextClassLoader())); + FileWatcher.defaultInstance().addWatch(configPath, new ConfigWatcher(config, configPath, Thread.currentThread().getContextClassLoader())); LOGGER.debug(CONFIG, "Watching TOML config file {} for changes", configPath); } } @@ -151,102 +154,123 @@ private static Path resolveBasePath(ModConfig config, Path configBasePath, @Null return configBasePath; } - static void loadConfig(ModConfig modConfig, CommentedFileConfig config) { + static void loadConfig(ModConfig modConfig, Path path, Function eventConstructor) { + CommentedConfig config; + try { - config.load(); + // Load existing config + config = readConfig(path); + if (!modConfig.getSpec().isCorrect(config)) { - LOGGER.warn(CONFIG, "Configuration file {} is not correct. Correcting", config.getFile().getAbsolutePath()); - backUpConfig(config); + LOGGER.warn(CONFIG, "Configuration file {} is not correct. Correcting", path); + backUpConfig(path); modConfig.getSpec().correct(config); - config.save(); + writeConfig(path, config); + } + } catch (NoSuchFileException ignored) { + // Config file does not exist yet + try { + setupConfigFile(modConfig, path); + config = readConfig(path); + } catch (IOException | ParsingException ex) { + throw new RuntimeException("Failed to create default config file " + modConfig.getFileName() + " of type " + modConfig.getType() + " for modid " + modConfig.getModId(), ex); } - } catch (ParsingException ex) { - LOGGER.warn(CONFIG, "Failed to parse {}: {}. Attempting to recreate", modConfig.getFileName(), ex); + } catch (IOException | ParsingException ex) { + // Failed to read existing file + LOGGER.warn(CONFIG, "Failed to load config {}: {}. Attempting to recreate", modConfig.getFileName(), ex); try { - backUpConfig(config); - Files.delete(config.getNioPath()); + backUpConfig(path); + Files.delete(path); - setupConfigFile(modConfig, config.getNioPath(), config.configFormat()); - config.load(); + setupConfigFile(modConfig, path); + config = readConfig(path); } catch (Throwable t) { ex.addSuppressed(t); throw new RuntimeException("Failed to recreate config file " + modConfig.getFileName() + " of type " + modConfig.getType() + " for modid " + modConfig.getModId(), ex); } } - modConfig.getSpec().acceptConfig(config); + + modConfig.setConfig(new LoadedConfig(config, path, modConfig), eventConstructor); } public static void acceptSyncedConfig(ModConfig modConfig, byte[] bytes) { - if (modConfig.config != null) { - LOGGER.warn("Overwriting non-null config {} at path {} with synced config", modConfig.config, modConfig.getFileName()); + if (modConfig.loadedConfig != null) { + LOGGER.warn("Overwriting non-null config {} at path {} with synced config", modConfig.loadedConfig, modConfig.getFileName()); } - modConfig.config = TomlFormat.instance().createParser().parse(new ByteArrayInputStream(bytes)); - // TODO: do we want to do any validation? (what do we do if it fails?) - modConfig.getSpec().acceptConfig(modConfig.config); - modConfig.postConfigEvent(ModConfigEvent.Reloading::new); // TODO: should maybe be Loading on the first load? + var newConfig = new SynchronizedConfig(InMemoryCommentedFormat.defaultInstance(), LinkedHashMap::new); + newConfig.bulkCommentedUpdate(view -> { + TomlFormat.instance().createParser().parse(new ByteArrayInputStream(bytes), view, ParsingMode.REPLACE); + }); + // TODO: do we want to do any validation? (what do we do if acceptConfig fails?) + modConfig.setConfig(new LoadedConfig(newConfig, null, modConfig), ModConfigEvent.Reloading::new); // TODO: should maybe be Loading on the first load? } public void loadDefaultServerConfigs() { configSets.get(ModConfig.Type.SERVER).forEach(modConfig -> { - if (modConfig.config != null) { - LOGGER.warn("Overwriting non-null config {} at path {} with default server config", modConfig.config, modConfig.getFileName()); + if (modConfig.loadedConfig != null) { + LOGGER.warn("Overwriting non-null config {} at path {} with default server config", modConfig.loadedConfig, modConfig.getFileName()); } - modConfig.config = createDefaultConfig(modConfig.getSpec()); - modConfig.getSpec().acceptConfig(modConfig.config); - modConfig.postConfigEvent(ModConfigEvent.Loading::new); + + modConfig.setConfig(new LoadedConfig(createDefaultConfig(modConfig.getSpec()), null, modConfig), ModConfigEvent.Loading::new); }); } private static CommentedConfig createDefaultConfig(IConfigSpec spec) { - var commentedConfig = CommentedConfig.inMemory(); - spec.correct(commentedConfig); + var commentedConfig = new SynchronizedConfig(InMemoryCommentedFormat.defaultInstance(), LinkedHashMap::new); + commentedConfig.bulkCommentedUpdate(spec::correct); return commentedConfig; } private static void closeConfig(ModConfig config) { - if (config.config != null) { - if (config.config instanceof CommentedFileConfig) { + if (config.loadedConfig != null) { + if (config.loadedConfig.path() != null) { LOGGER.trace(CONFIG, "Closing config file type {} at {} for {}", config.getType(), config.getFileName(), config.getModId()); - unload(config); - config.config = null; - config.getSpec().acceptConfig(null); - config.postConfigEvent(ModConfigEvent.Unloading::new); + unload(config.loadedConfig.path()); + config.setConfig(null, ModConfigEvent.Unloading::new); } else { - LOGGER.warn(CONFIG, "Closing non-file config {} at path {}", config.config, config.getFileName()); + LOGGER.warn(CONFIG, "Closing non-file config {} at path {}", config.loadedConfig, config.getFileName()); } } } - private static void unload(ModConfig config) { + private static void unload(Path path) { if (FMLConfig.getBoolConfigValue(FMLConfig.ConfigValue.DISABLE_CONFIG_WATCHER)) return; - Path configPath = config.getFullPath(); try { - FileWatcher.defaultInstance().removeWatch(configPath); + FileWatcher.defaultInstance().removeWatch(path); } catch (RuntimeException e) { - LOGGER.error("Failed to remove config {} from tracker!", configPath, e); + LOGGER.error("Failed to remove config {} from tracker!", path, e); } } - private static boolean setupConfigFile(ModConfig modConfig, Path file, ConfigFormat conf) throws IOException { + private static void setupConfigFile(ModConfig modConfig, Path file) throws IOException { Files.createDirectories(file.getParent()); Path p = defaultConfigPath.resolve(modConfig.getFileName()); if (Files.exists(p)) { LOGGER.info(CONFIG, "Loading default config file from path {}", p); Files.copy(p, file); } else { - conf.createWriter().write(createDefaultConfig(modConfig.getSpec()), file, WritingMode.REPLACE_ATOMIC); + writeConfig(file, createDefaultConfig(modConfig.getSpec())); } - return true; } - private static void backUpConfig(CommentedFileConfig commentedFileConfig) { - backUpConfig(commentedFileConfig, 5); //TODO: Think of a way for mods to set their own preference (include a sanity check as well, no disk stuffing) + private static ConcurrentCommentedConfig readConfig(Path path) throws IOException, ParsingException { + try (var reader = Files.newBufferedReader(path)) { + var config = new SynchronizedConfig(TomlFormat.instance(), LinkedHashMap::new); + config.bulkCommentedUpdate(view -> { + new TomlParser().parse(reader, view, ParsingMode.REPLACE); + }); + return config; + } + } + + static void writeConfig(Path file, UnmodifiableCommentedConfig config) { + new TomlWriter().write(config, file, WritingMode.REPLACE_ATOMIC); } - private static void backUpConfig(CommentedFileConfig commentedFileConfig, int maxBackups) { - backUpConfig(commentedFileConfig.getNioPath(), maxBackups); + private static void backUpConfig(Path commentedFileConfig) { + backUpConfig(commentedFileConfig, 5); //TODO: Think of a way for mods to set their own preference (include a sanity check as well, no disk stuffing) } private static void backUpConfig(Path commentedFileConfig, int maxBackups) { diff --git a/loader/src/main/java/net/neoforged/fml/config/ConfigWatcher.java b/loader/src/main/java/net/neoforged/fml/config/ConfigWatcher.java index ec07eb1c..2b2d5ead 100644 --- a/loader/src/main/java/net/neoforged/fml/config/ConfigWatcher.java +++ b/loader/src/main/java/net/neoforged/fml/config/ConfigWatcher.java @@ -5,8 +5,8 @@ package net.neoforged.fml.config; -import com.electronwill.nightconfig.core.file.CommentedFileConfig; import com.mojang.logging.LogUtils; +import java.nio.file.Path; import net.neoforged.fml.event.config.ModConfigEvent; import org.slf4j.Logger; @@ -14,12 +14,12 @@ class ConfigWatcher implements Runnable { private static final Logger LOGGER = LogUtils.getLogger(); private final ModConfig modConfig; - private final CommentedFileConfig commentedFileConfig; + private final Path path; private final ClassLoader realClassLoader; - ConfigWatcher(ModConfig modConfig, CommentedFileConfig commentedFileConfig, ClassLoader classLoader) { + ConfigWatcher(ModConfig modConfig, Path path, ClassLoader classLoader) { this.modConfig = modConfig; - this.commentedFileConfig = commentedFileConfig; + this.path = path; this.realClassLoader = classLoader; } @@ -32,11 +32,7 @@ public void run() { modConfig.lock.lock(); try { LOGGER.debug(ConfigTracker.CONFIG, "Config file {} changed, re-loading", modConfig.getFileName()); - if (this.modConfig.config != this.commentedFileConfig) { - LOGGER.warn(ConfigTracker.CONFIG, "Config file {} has a mismatched loaded config. Expected {} but was {}.", modConfig.getFileName(), commentedFileConfig, modConfig.config); - } - ConfigTracker.loadConfig(this.modConfig, this.commentedFileConfig); - this.modConfig.postConfigEvent(ModConfigEvent.Reloading::new); + ConfigTracker.loadConfig(this.modConfig, this.path, ModConfigEvent.Reloading::new); } finally { modConfig.lock.unlock(); } diff --git a/loader/src/main/java/net/neoforged/fml/config/IConfigSpec.java b/loader/src/main/java/net/neoforged/fml/config/IConfigSpec.java index f66e823c..91dfc829 100644 --- a/loader/src/main/java/net/neoforged/fml/config/IConfigSpec.java +++ b/loader/src/main/java/net/neoforged/fml/config/IConfigSpec.java @@ -18,8 +18,7 @@ *

Thread-safety

*

The {@link Config} objects themselves are thread-safe, but mod config code should not be assumed to be thread-safe in general. * FML will guard event dispatches behind a lock when necessary. - * A spec can safely mutate {@code Config} objects, but should let FML fire events, - * for example using {@link ModConfig#onConfigChanged()}. + * A spec can safely mutate {@code Config} objects, but should let FML fire events and saving the file by calling {@link ILoadedConfig#save()}. */ public interface IConfigSpec { /** @@ -53,5 +52,20 @@ public interface IConfigSpec { * This is called on loading and on reloading. * The config is guaranteed to be valid according to {@link #isCorrect}. */ - void acceptConfig(@Nullable CommentedConfig config); + void acceptConfig(@Nullable ILoadedConfig config); + + sealed interface ILoadedConfig permits LoadedConfig { + /** + * Accesses the current config. + * + *

The config locks internally, and is therefore safe to + * read/write across multiple threads without additional synchronization. + */ + CommentedConfig config(); + + /** + * Saves the current value of the {@link #config} and dispatches a config reloading event. + */ + void save(); + } } diff --git a/loader/src/main/java/net/neoforged/fml/config/LoadedConfig.java b/loader/src/main/java/net/neoforged/fml/config/LoadedConfig.java new file mode 100644 index 00000000..412a9d58 --- /dev/null +++ b/loader/src/main/java/net/neoforged/fml/config/LoadedConfig.java @@ -0,0 +1,26 @@ +/* + * Copyright (c) NeoForged and contributors + * SPDX-License-Identifier: LGPL-2.1-only + */ + +package net.neoforged.fml.config; + +import com.electronwill.nightconfig.core.CommentedConfig; +import java.nio.file.Path; +import net.neoforged.fml.event.config.ModConfigEvent; +import org.jetbrains.annotations.Nullable; + +record LoadedConfig(CommentedConfig config, @Nullable Path path, ModConfig modConfig) implements IConfigSpec.ILoadedConfig { + @Override + public void save() { + if (path != null) { + ConfigTracker.writeConfig(path, config); + } + modConfig.lock.lock(); + try { + modConfig.container.acceptEvent(new ModConfigEvent.Reloading(modConfig)); + } finally { + modConfig.lock.unlock(); + } + } +} diff --git a/loader/src/main/java/net/neoforged/fml/config/ModConfig.java b/loader/src/main/java/net/neoforged/fml/config/ModConfig.java index 1ea02cf8..bcb08c05 100644 --- a/loader/src/main/java/net/neoforged/fml/config/ModConfig.java +++ b/loader/src/main/java/net/neoforged/fml/config/ModConfig.java @@ -5,9 +5,6 @@ package net.neoforged.fml.config; -import com.electronwill.nightconfig.core.CommentedConfig; -import com.electronwill.nightconfig.core.file.CommentedFileConfig; -import com.electronwill.nightconfig.core.file.FileConfig; import java.nio.file.Path; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; @@ -21,14 +18,9 @@ public final class ModConfig { private final Type type; private final IConfigSpec spec; private final String fileName; - private final ModContainer container; - /** - * When a file config is open: this is a {@link CommentedFileConfig}. - * When a config was loaded from the server: this is a plain config. - * Otherwise this is null. - */ + final ModContainer container; @Nullable - CommentedConfig config; + LoadedConfig loadedConfig; /** * NightConfig's own configs are threadsafe, but mod code is not necessarily. * This lock is used to prevent multiple concurrent config reloads or event dispatches. @@ -61,31 +53,20 @@ public String getModId() { // TODO: remove from public API? public Path getFullPath() { - if (this.config instanceof FileConfig fileConfig) { - return fileConfig.getNioPath(); + if (this.loadedConfig != null && loadedConfig.path() != null) { + return loadedConfig.path(); } else { - throw new IllegalStateException("Cannot call getFullPath() on non-file config " + this.config + " at path " + getFileName()); - } - } - - /** - * To be called when the config changed in code, for example via {@code ModConfigSpec.ConfigValue#set}. - * This function will update the config on disk, and fire the reloading event, taking the appropriate lock first. - */ - public void onConfigChanged() { - if (!(this.config instanceof FileConfig fileConfig)) { - throw new IllegalStateException("Cannot call onConfigChanged on non-file config " + this.config + " at path " + getFileName()); + throw new IllegalStateException("Cannot call getFullPath() on non-file config " + this.loadedConfig + " at path " + getFileName()); } - - fileConfig.save(); - postConfigEvent(ModConfigEvent.Reloading::new); } - void postConfigEvent(Function constructor) { + void setConfig(@Nullable LoadedConfig loadedConfig, Function eventConstructor) { lock.lock(); try { - container.acceptEvent(constructor.apply(this)); + this.loadedConfig = loadedConfig; + spec.acceptConfig(loadedConfig); + container.acceptEvent(eventConstructor.apply(this)); } finally { lock.unlock(); } diff --git a/loader/src/test/java/net/neoforged/fml/config/ConfigTrackerTest.java b/loader/src/test/java/net/neoforged/fml/config/ConfigTrackerTest.java index d1902b68..22045fe9 100644 --- a/loader/src/test/java/net/neoforged/fml/config/ConfigTrackerTest.java +++ b/loader/src/test/java/net/neoforged/fml/config/ConfigTrackerTest.java @@ -14,6 +14,7 @@ import net.neoforged.fml.loading.FMLConfig; import net.neoforged.fml.loading.FMLPaths; import org.assertj.core.api.Assertions; +import org.jetbrains.annotations.Nullable; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -218,8 +219,8 @@ private boolean correct(UnmodifiableCommentedConfig config, boolean dryRun) { } @Override - public void acceptConfig(CommentedConfig config) { - loadedValue = config.getInt("configEntry"); + public void acceptConfig(@Nullable ILoadedConfig config) { + loadedValue = config == null ? 4 : config.config().getInt("configEntry"); } } }