From 1989f122c60a012a5993bb195bf7bc676678ddbe Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Thu, 3 Aug 2023 17:52:50 -0400 Subject: [PATCH] Remove locking system for Night Config files This can cause deadlocks if mods themselves are also using their own internal locks (Sophisticated Backpacks does this on 1.16 by using a CHM) This system will be replaced by a command/keybind to manually reload configs --- .../modernfix/forge/config/ConfigFixer.java | 23 ++-- .../forge/config/NightConfigFixer.java | 103 ++---------------- 2 files changed, 26 insertions(+), 100 deletions(-) diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java b/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java index eaf9621bf..49558094b 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java @@ -8,6 +8,9 @@ import org.embeddedt.modernfix.core.ModernFixMixinPlugin; import java.util.Optional; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Consumer; public class ConfigFixer { @@ -35,6 +38,7 @@ public static void replaceConfigHandlers() { private static class LockingConfigHandler implements Consumer { private final Consumer actualHandler; private final String modId; + private final Lock lock = new ReentrantLock(); LockingConfigHandler(String id, Consumer actualHandler) { this.modId = id; @@ -43,14 +47,17 @@ private static class LockingConfigHandler implements Consumer configsToReload = new LinkedHashSet<>(); public static void monitorFileWatcher() { + if(!ModernFixMixinPlugin.instance.isOptionEnabled("bugfix.fix_config_crashes.NightConfigFixerMixin")) + return; CommonModUtil.runWithoutCrash(() -> { FileWatcher watcher = FileWatcher.defaultInstance(); Field field = FileWatcher.class.getDeclaredField("watchedFiles"); @@ -38,30 +31,6 @@ public static void monitorFileWatcher() { private static final Class WATCHED_FILE = LamdbaExceptionUtils.uncheck(() -> Class.forName("com.electronwill.nightconfig.core.file.FileWatcher$WatchedFile")); private static final Field CHANGE_HANDLER = ObfuscationReflectionHelper.findField(WATCHED_FILE, "changeHandler"); - public 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); @@ -82,27 +51,6 @@ public Object computeIfAbsent(Path key, Function mappingFunctio } } - private static final Set> UNKNOWN_FILE_CONFIG_CLASSES = Collections.synchronizedSet(new ReferenceOpenHashSet<>()); - - public static Object toWriteSyncConfig(Object config) { - if(config == null) - return null; - try { - if(WRITE_SYNC_CONFIG.isAssignableFrom(config.getClass())) { - return config; - } else if(AUTOSAVE_CONFIG.isAssignableFrom(config.getClass())) { - FileConfig fc = (FileConfig)AUTOSAVE_FILECONFIG.get(config); - return toWriteSyncConfig(fc); - } else { - if (UNKNOWN_FILE_CONFIG_CLASSES.add(config.getClass())) - ModernFixMixinPlugin.instance.logger.warn("Unexpected FileConfig class: {}", config.getClass().getName()); - return null; - } - } catch(ReflectiveOperationException e) { - return null; - } - } - static class MonitoringConfigTracker implements Runnable { private final Runnable configTracker; @@ -110,47 +58,18 @@ static class MonitoringConfigTracker implements Runnable { this.configTracker = r; } - private void protectFromSaving(FileConfig config, Runnable runnable) throws ReflectiveOperationException { - Object writeSyncConfig = toWriteSyncConfig(config); - if(writeSyncConfig != null) { - // keep trying to write, releasing the config lock each time in case something else needs to lock it - // for any reason - while(true) { - // acquiring synchronized block here should in theory prevent any other concurrent loads/saves, based - // off WriteSyncFileConfig implementation - synchronized (writeSyncConfig) { - if(CURRENTLY_WRITING.getBoolean(writeSyncConfig)) { - ModernFixMixinPlugin.instance.logger.fatal("Config being written during load!!!"); - try { Thread.sleep(500); } catch(InterruptedException e) { Thread.currentThread().interrupt(); } - continue; - } - // at this point, currentlyWriting is false, and we acquired synchronized lock, should be good to - // go - runnable.run(); - break; - } - } - } else { - runnable.run(); - } - } - /** - * 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. + * Add the config */ @Override public void run() { - try { - Field theField = getCurrentlyWritingFieldOnWatcher(this.configTracker); - if(theField != null) { - CommentedFileConfig cfg = (CommentedFileConfig)theField.get(this.configTracker); - // will synchronize and check saving flag - protectFromSaving(cfg, configTracker); + synchronized(configsToReload) { + int oldSize = configsToReload.size(); + configsToReload.add(configTracker); + if(oldSize == 0) { + ModernFixMixinPlugin.instance.logger.info("Config file change detected on disk. The Forge feature to watch config files for changes is currently disabled due to random corruption issues."); + ModernFixMixinPlugin.instance.logger.info("This functionality will be restored in a future ModernFix update."); } - } catch(ReflectiveOperationException e) { - e.printStackTrace(); } } }