From b499a054b93d8804ddc978ad8da85280440e519f Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Tue, 18 Jul 2023 12:44:11 -0400 Subject: [PATCH 1/3] Possible fix for the elusive Forge config corruption bug Block file watcher from proceeding until config is done saving Related: https://github.com/MinecraftForge/MinecraftForge/issues/9122 --- .../forge/config/NightConfigFixer.java | 119 ++++++++++++++++++ .../forge/ModernFixPlatformHooksImpl.java | 3 + 2 files changed, 122 insertions(+) create mode 100644 forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java b/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java new file mode 100644 index 00000000..26c6e20f --- /dev/null +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java @@ -0,0 +1,119 @@ +package org.embeddedt.modernfix.forge.config; + +import com.electronwill.nightconfig.core.file.CommentedFileConfig; +import com.electronwill.nightconfig.core.file.FileConfig; +import com.electronwill.nightconfig.core.file.FileWatcher; +import cpw.mods.modlauncher.api.LamdbaExceptionUtils; +import it.unimi.dsi.fastutil.objects.ReferenceOpenHashSet; +import net.minecraftforge.fml.common.ObfuscationReflectionHelper; +import org.embeddedt.modernfix.core.ModernFixMixinPlugin; +import org.embeddedt.modernfix.util.CommonModUtil; + +import java.lang.reflect.Field; +import java.nio.file.Path; +import java.util.Collections; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; + +/** + * Relatively simple patch to wait for config saving to finish, made complex by Night Config classes being package-private, + * and Forge not allowing mixins into libraries. + */ +public class NightConfigFixer { + public static void monitorFileWatcher() { + CommonModUtil.runWithoutCrash(() -> { + FileWatcher watcher = FileWatcher.defaultInstance(); + Field field = FileWatcher.class.getDeclaredField("watchedFiles"); + field.setAccessible(true); + ConcurrentHashMap theMap = (ConcurrentHashMap)field.get(watcher); + // replace the backing map of watched files with one we control + field.set(watcher, new MonitoringMap(theMap)); + ModernFixMixinPlugin.instance.logger.info("Applied Forge config corruption patch"); + }, "replacing Night Config watchedFiles map"); + } + + private static final Class WATCHED_FILE = LamdbaExceptionUtils.uncheck(() -> Class.forName("com.electronwill.nightconfig.core.file.FileWatcher$WatchedFile")); + private static final Class CONFIG_WATCHER = LamdbaExceptionUtils.uncheck(() -> Class.forName("net.minecraftforge.fml.config.ConfigFileTypeHandler$ConfigWatcher")); + private static final Field CHANGE_HANDLER = ObfuscationReflectionHelper.findField(WATCHED_FILE, "changeHandler"); + + private static final Field COMMENTED_FILE_CONFIG = ObfuscationReflectionHelper.findField(CONFIG_WATCHER, "commentedFileConfig"); + + private static final Class WRITE_SYNC_CONFIG = LamdbaExceptionUtils.uncheck(() -> Class.forName("com.electronwill.nightconfig.core.file.WriteSyncFileConfig")); + private static final Class AUTOSAVE_CONFIG = LamdbaExceptionUtils.uncheck(() -> Class.forName("com.electronwill.nightconfig.core.file.AutosaveCommentedFileConfig")); + private static final Field AUTOSAVE_FILECONFIG = ObfuscationReflectionHelper.findField(AUTOSAVE_CONFIG, "fileConfig"); + + private static final Field CURRENTLY_WRITING = ObfuscationReflectionHelper.findField(WRITE_SYNC_CONFIG, "currentlyWriting"); + + static class MonitoringMap extends ConcurrentHashMap { + public MonitoringMap(ConcurrentHashMap oldMap) { + super(oldMap); + } + + @Override + public Object computeIfAbsent(Path key, Function mappingFunction) { + return super.computeIfAbsent(key, path -> { + Object watchedFile = mappingFunction.apply(path); + try { + Runnable changeHandler = (Runnable)CHANGE_HANDLER.get(watchedFile); + // replace Forge's config tracker with our own + if(CONFIG_WATCHER.isAssignableFrom(changeHandler.getClass())) + CHANGE_HANDLER.set(watchedFile, new MonitoringConfigTracker(changeHandler)); + else + ModernFixMixinPlugin.instance.logger.warn("Unexpected Forge config tracker class: {}", changeHandler.getClass().getName()); + } catch(ReflectiveOperationException e) { + e.printStackTrace(); + } + return watchedFile; + }); + } + } + + static class MonitoringConfigTracker implements Runnable { + private final Runnable configTracker; + + MonitoringConfigTracker(Runnable r) { + this.configTracker = r; + } + + private static final Set> UNKNOWN_FILE_CONFIG_CLASSES = Collections.synchronizedSet(new ReferenceOpenHashSet<>()); + + private boolean isSaving(FileConfig config) throws ReflectiveOperationException { + if(WRITE_SYNC_CONFIG.isAssignableFrom(config.getClass())) { + return CURRENTLY_WRITING.getBoolean(config); + } else if(AUTOSAVE_CONFIG.isAssignableFrom(config.getClass())) { + FileConfig fc = (FileConfig)AUTOSAVE_FILECONFIG.get(config); + return isSaving(fc); + } else { + if(UNKNOWN_FILE_CONFIG_CLASSES.add(config.getClass())) + ModernFixMixinPlugin.instance.logger.warn("Unexpected FileConfig class: {}", config.getClass().getName()); + return false; + } + } + + /** + * This entrypoint runs when the file watcher has detected a change on the config file. Before passing + * this through to Forge, use reflection hacks to confirm the config system is not still writing to the file. + * If it is, spin until writing finishes. Immediately returning might result in the event never being observed. + */ + @Override + public void run() { + try { + CommentedFileConfig cfg = (CommentedFileConfig)COMMENTED_FILE_CONFIG.get(this.configTracker); + while(isSaving(cfg)) { + // block until the config is no longer marked as saving + // Forge shouldn't save the config twice in a row under normal conditions so this should fix the + // original bug + try { + Thread.sleep(500); + } catch (InterruptedException e) { + return; + } + } + } catch(ReflectiveOperationException e) { + e.printStackTrace(); + } + configTracker.run(); + } + } +} diff --git a/forge/src/main/java/org/embeddedt/modernfix/platform/forge/ModernFixPlatformHooksImpl.java b/forge/src/main/java/org/embeddedt/modernfix/platform/forge/ModernFixPlatformHooksImpl.java index 6f9146e6..40c0fafe 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/platform/forge/ModernFixPlatformHooksImpl.java +++ b/forge/src/main/java/org/embeddedt/modernfix/platform/forge/ModernFixPlatformHooksImpl.java @@ -29,6 +29,7 @@ import org.embeddedt.modernfix.core.ModernFixMixinPlugin; import org.embeddedt.modernfix.api.constants.IntegrationConstants; import org.embeddedt.modernfix.forge.classloading.FastAccessTransformerList; import org.embeddedt.modernfix.forge.classloading.ModernFixResourceFinder; +import org.embeddedt.modernfix.forge.config.NightConfigFixer; import org.embeddedt.modernfix.forge.packet.PacketHandler; import org.embeddedt.modernfix.forge.spark.SparkLaunchProfiler; import org.embeddedt.modernfix.util.CommonModUtil; @@ -180,6 +181,8 @@ public class ModernFixPlatformHooksImpl { if(ModernFixMixinPlugin.instance.isOptionEnabled("feature.spark_profile_launch.OnForge")) { CommonModUtil.runWithoutCrash(() -> SparkLaunchProfiler.start("launch"), "Failed to start profiler"); } + + NightConfigFixer.monitorFileWatcher(); } private Method defineClassMethod = null; From 36eb73b28e7a93ab60b362c40fe44dd19410c5a9 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Tue, 18 Jul 2023 13:48:01 -0400 Subject: [PATCH 2/3] Generify config patch to also work with Night Config Fixes mod --- .../forge/config/NightConfigFixer.java | 50 ++++++++++++------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java b/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java index 26c6e20f..b05dbbb1 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java @@ -12,6 +12,8 @@ import org.embeddedt.modernfix.util.CommonModUtil; import java.lang.reflect.Field; import java.nio.file.Path; import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; @@ -34,17 +36,32 @@ public class NightConfigFixer { } private static final Class WATCHED_FILE = LamdbaExceptionUtils.uncheck(() -> Class.forName("com.electronwill.nightconfig.core.file.FileWatcher$WatchedFile")); - private static final Class CONFIG_WATCHER = LamdbaExceptionUtils.uncheck(() -> Class.forName("net.minecraftforge.fml.config.ConfigFileTypeHandler$ConfigWatcher")); private static final Field CHANGE_HANDLER = ObfuscationReflectionHelper.findField(WATCHED_FILE, "changeHandler"); - private static final Field COMMENTED_FILE_CONFIG = ObfuscationReflectionHelper.findField(CONFIG_WATCHER, "commentedFileConfig"); - private static final Class WRITE_SYNC_CONFIG = LamdbaExceptionUtils.uncheck(() -> Class.forName("com.electronwill.nightconfig.core.file.WriteSyncFileConfig")); private static final Class AUTOSAVE_CONFIG = LamdbaExceptionUtils.uncheck(() -> Class.forName("com.electronwill.nightconfig.core.file.AutosaveCommentedFileConfig")); private static final Field AUTOSAVE_FILECONFIG = ObfuscationReflectionHelper.findField(AUTOSAVE_CONFIG, "fileConfig"); private static final Field CURRENTLY_WRITING = ObfuscationReflectionHelper.findField(WRITE_SYNC_CONFIG, "currentlyWriting"); + private static final Map, Field> CONFIG_WATCHER_TO_CONFIG_FIELD = Collections.synchronizedMap(new HashMap<>()); + + private static Field getCurrentlyWritingFieldOnWatcher(Object watcher) { + return CONFIG_WATCHER_TO_CONFIG_FIELD.computeIfAbsent(watcher.getClass(), clz -> { + while(clz != null && clz != Object.class) { + for(Field f : clz.getDeclaredFields()) { + if(CommentedFileConfig.class.isAssignableFrom(f.getType())) { + f.setAccessible(true); + ModernFixMixinPlugin.instance.logger.debug("Found CommentedFileConfig: field '{}' on {}", f.getName(), clz.getName()); + return f; + } + } + clz = clz.getSuperclass(); + } + return null; + }); + } + static class MonitoringMap extends ConcurrentHashMap { public MonitoringMap(ConcurrentHashMap oldMap) { super(oldMap); @@ -56,11 +73,7 @@ public class NightConfigFixer { Object watchedFile = mappingFunction.apply(path); try { Runnable changeHandler = (Runnable)CHANGE_HANDLER.get(watchedFile); - // replace Forge's config tracker with our own - if(CONFIG_WATCHER.isAssignableFrom(changeHandler.getClass())) - CHANGE_HANDLER.set(watchedFile, new MonitoringConfigTracker(changeHandler)); - else - ModernFixMixinPlugin.instance.logger.warn("Unexpected Forge config tracker class: {}", changeHandler.getClass().getName()); + CHANGE_HANDLER.set(watchedFile, new MonitoringConfigTracker(changeHandler)); } catch(ReflectiveOperationException e) { e.printStackTrace(); } @@ -99,15 +112,18 @@ public class NightConfigFixer { @Override public void run() { try { - CommentedFileConfig cfg = (CommentedFileConfig)COMMENTED_FILE_CONFIG.get(this.configTracker); - while(isSaving(cfg)) { - // block until the config is no longer marked as saving - // Forge shouldn't save the config twice in a row under normal conditions so this should fix the - // original bug - try { - Thread.sleep(500); - } catch (InterruptedException e) { - return; + Field theField = getCurrentlyWritingFieldOnWatcher(this.configTracker); + if(theField != null) { + CommentedFileConfig cfg = (CommentedFileConfig)theField.get(this.configTracker); + while(isSaving(cfg)) { + // block until the config is no longer marked as saving + // Forge shouldn't save the config twice in a row under normal conditions so this should fix the + // original bug + try { + Thread.sleep(500); + } catch (InterruptedException e) { + return; + } } } } catch(ReflectiveOperationException e) { From 8ac43d561749fc6e721ccc8b7cfb8f76fd1a9a05 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Tue, 18 Jul 2023 13:51:21 -0400 Subject: [PATCH 3/3] Fix incorrect import --- .../org/embeddedt/modernfix/forge/config/NightConfigFixer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java b/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java index b05dbbb1..b78db791 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java @@ -5,7 +5,7 @@ import com.electronwill.nightconfig.core.file.FileConfig; import com.electronwill.nightconfig.core.file.FileWatcher; import cpw.mods.modlauncher.api.LamdbaExceptionUtils; import it.unimi.dsi.fastutil.objects.ReferenceOpenHashSet; -import net.minecraftforge.fml.common.ObfuscationReflectionHelper; +import net.minecraftforge.fml.util.ObfuscationReflectionHelper; import org.embeddedt.modernfix.core.ModernFixMixinPlugin; import org.embeddedt.modernfix.util.CommonModUtil;