Skip to content

Commit

Permalink
Remove locking system for Night Config files
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@embeddedt
embeddedt committed Aug 3, 2023
1 parent 5853f9b commit 1989f12
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 100 deletions.
23 changes: 15 additions & 8 deletions forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -35,6 +38,7 @@ public static void replaceConfigHandlers() {
private static class LockingConfigHandler implements Consumer<ModConfig.ModConfigEvent> {
private final Consumer<ModConfig.ModConfigEvent> actualHandler;
private final String modId;
private final Lock lock = new ReentrantLock();

LockingConfigHandler(String id, Consumer<ModConfig.ModConfigEvent> actualHandler) {
this.modId = id;
Expand All @@ -43,14 +47,17 @@ private static class LockingConfigHandler implements Consumer<ModConfig.ModConfi

@Override
public void accept(ModConfig.ModConfigEvent modConfigEvent) {
Object cfgObj = NightConfigFixer.toWriteSyncConfig(modConfigEvent.getConfig().getConfigData());
if(cfgObj != null) {
// don't synchronize on 'this' as it produces a deadlock when used alongside NightConfigFixer
synchronized (cfgObj) {
this.actualHandler.accept(modConfigEvent);
}
} else {
this.actualHandler.accept(modConfigEvent);
try {
if(lock.tryLock(2, TimeUnit.SECONDS)) {
try {
this.actualHandler.accept(modConfigEvent);
} finally {
lock.unlock();
}
} else
ModernFix.LOGGER.error("Failed to post config event for {}, someone else is holding the lock", modId);
} catch(InterruptedException e) {
Thread.currentThread().interrupt();
}
}

Expand Down
103 changes: 11 additions & 92 deletions forge/src/main/java/org/embeddedt/modernfix/forge/config/NightConfigFixer.java
View file
@@ -1,29 +1,22 @@
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.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.LinkedHashSet;
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 final LinkedHashSet<Runnable> 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");
Expand All @@ -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<Class<?>, 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<Path, Object> {
public MonitoringMap(ConcurrentHashMap<Path, ?> oldMap) {
super(oldMap);
Expand All @@ -82,75 +51,25 @@ public Object computeIfAbsent(Path key, Function<? super Path, ?> mappingFunctio
}
}

private static final Set<Class<?>> 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;

MonitoringConfigTracker(Runnable r) {
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();
}
}
}
Expand Down

0 comments on commit 1989f12

Please sign in to comment.