Improve threadsafety for filesystems, and remove the "transformed-mod…

…-" prefix from mod filesystem names. Hopefully improve the virtual file systems not being thread-safe (#382)
QuiltMC · Nov 3, 2023 · e898d41 · e898d41
1 parent 06c9518
commit e898d41
Show file tree

Hide file tree

Showing 7 changed files with 101 additions and 76 deletions.
diff --git a/src/main/java/org/quiltmc/loader/impl/QuiltLoaderImpl.java b/src/main/java/org/quiltmc/loader/impl/QuiltLoaderImpl.java
@@ -414,7 +414,8 @@ private void setup() throws ModResolutionException {
 					List<Path> paths = new ArrayList<>();
 
 					long start = System.nanoTime();
-					paths.add(new QuiltZipFileSystem("transformed-mod-" + modid, transformedModBundle.resolve(modid)).getRoot());
+					String fsName = modid + "-" + modOption.version();
+					paths.add(new QuiltZipFileSystem(fsName, transformedModBundle.resolve(modid)).getRoot());
 					if (modOption.couldResourcesChange()) {
 						paths.add(modOption.resourceRoot());
 					}
@@ -429,7 +430,7 @@ private void setup() throws ModResolutionException {
 					 if (paths.size() == 1) {
 						 resourceRoot = paths.get(0);
 					 } else {
-						 resourceRoot = new QuiltJoinedFileSystem("final-mod-" + modid, paths).getRoot();
+						 resourceRoot = new QuiltJoinedFileSystem("_" + fsName, paths).getRoot();
 					 }
 				}
 			}

diff --git a/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltBaseFileSystem.java b/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltBaseFileSystem.java
@@ -27,6 +27,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.regex.Pattern;
 
 import org.jetbrains.annotations.Nullable;
@@ -36,11 +37,17 @@
 @QuiltLoaderInternal(QuiltLoaderInternalType.LEGACY_EXPOSED)
 public abstract class QuiltBaseFileSystem<FS extends QuiltBaseFileSystem<FS, P>, P extends QuiltBasePath<FS, P>>
 	extends FileSystem {
+
 	static {
 		DelegatingUrlStreamHandlerFactory.load();
 	}
 
 	private static final Map<String, Integer> uniqueNames = new HashMap<>();
+	private static final AtomicLong SYNC_ASSIGNMENT = new AtomicLong();
+
+	/** This stores the "synchronization order" for "move" type operations where we need to synchronize on multiple
+	 * filesystems at once. */
+	final long syncOrder = SYNC_ASSIGNMENT.getAndIncrement();
 
 	final Class<FS> filesystemClass;
 	final Class<P> pathClass;

diff --git a/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltMapFileSystem.java b/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltMapFileSystem.java
@@ -166,17 +166,19 @@ private <T extends Throwable> void addEntryRequiringParents0(QuiltUnifiedEntry n
 			}
 		}
 
-		QuiltUnifiedEntry entry = getEntry(parent);
-		if (entry instanceof QuiltUnifiedFolderWriteable) {
-			addEntryWithoutParents0(newEntry, execCtor);
-			((QuiltUnifiedFolderWriteable) entry).children.add(path);
-		} else if (entry == null) {
-			throw execCtor.apply("Cannot put entry " + path + " because the parent folder doesn't exist!");
-		} else {
-			throw execCtor.apply("Cannot put entry " + path + " because the parent is not a folder (was " + entry + ")");
-		}
+		synchronized (this) {
+			QuiltUnifiedEntry entry = getEntry(parent);
+			if (entry instanceof QuiltUnifiedFolderWriteable) {
+				addEntryWithoutParents0(newEntry, execCtor);
+				((QuiltUnifiedFolderWriteable) entry).children.add(path);
+			} else if (entry == null) {
+				throw execCtor.apply("Cannot put entry " + path + " because the parent folder doesn't exist!");
+			} else {
+				throw execCtor.apply("Cannot put entry " + path + " because the parent is not a folder (was " + entry + ")");
+			}
 
-		validate();
+			validate();
+		}
 	}
 
 	protected void addEntryAndParents(QuiltUnifiedEntry newEntry) throws IOException {
@@ -187,7 +189,7 @@ protected void addEntryAndParentsUnsafe(QuiltUnifiedEntry newEntry) {
 		addEntryAndParents0(newEntry, IllegalStateException::new);
 	}
 
-	private <T extends Throwable> void addEntryAndParents0(QuiltUnifiedEntry newEntry, Function<String, T> execCtor) throws T {
+	private synchronized <T extends Throwable> void addEntryAndParents0(QuiltUnifiedEntry newEntry, Function<String, T> execCtor) throws T {
 		P path = addEntryWithoutParents0(newEntry, execCtor);
 		P parent = path;
 		P previous = path;
@@ -235,7 +237,7 @@ private <T extends Throwable> P addEntryWithoutParents0(QuiltUnifiedEntry newEnt
 		}
 	}
 
-	protected boolean removeEntry(P path, boolean throwIfMissing) throws IOException {
+	protected synchronized boolean removeEntry(P path, boolean throwIfMissing) throws IOException {
 		path = path.toAbsolutePath().normalize();
 
 		QuiltUnifiedEntry current = getEntry(path);
@@ -321,7 +323,7 @@ public Stream<Path> list(Path dir) throws IOException {
 	}
 
 	@Override
-	public Path createDirectories(Path dir, FileAttribute<?>... attrs) throws IOException {
+	public synchronized Path createDirectories(Path dir, FileAttribute<?>... attrs) throws IOException {
 		dir = dir.toAbsolutePath().normalize();
 		Deque<Path> stack = new ArrayDeque<>();
 		Path p = dir;

diff --git a/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltMapFileSystemProvider.java b/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltMapFileSystemProvider.java
@@ -170,19 +170,24 @@ public OutputStream newOutputStream(Path pathIn, OpenOption... options) throws I
 		}
 
 		ensureWriteable(path);
-		QuiltUnifiedEntry current = path.fs.getEntry(path);
 		QuiltUnifiedFile targetFile = null;
 
 		if (create) {
-			if (current != null) {
-				delete(path);
-			}
+			synchronized (path.fs) {
+				QuiltUnifiedEntry current = path.fs.getEntry(path);
+				if (current != null) {
+					delete(path);
+				}
 
-			path.fs.addEntryRequiringParent(targetFile = new QuiltMemoryFile.ReadWrite(path));
-		} else if (current instanceof QuiltUnifiedFile) {
-			targetFile = (QuiltUnifiedFile) current;
+				path.fs.addEntryRequiringParent(targetFile = new QuiltMemoryFile.ReadWrite(path));
+			}
 		} else {
-			throw new IOException("Cannot open an OutputStream on " + current);
+			QuiltUnifiedEntry current = path.fs.getEntry(path);
+			if (current instanceof QuiltUnifiedFile) {
+				targetFile = (QuiltUnifiedFile) current;
+			} else {
+				throw new IOException("Cannot open an OutputStream on " + current);
+			}
 		}
 
 		OutputStream stream = targetFile.createOutputStream(append, truncate);
@@ -243,13 +248,15 @@ public SeekableByteChannel newByteChannel(Path pathIn, Set<? extends OpenOption>
 		throws IOException {
 
 		P path = toAbsolutePath(pathIn);
-		QuiltUnifiedEntry entry = path.fs.getEntry(path);
+		QuiltUnifiedEntry entry;
 
-		if (!path.getFileSystem().isReadOnly()) {
+		if (path.getFileSystem().isReadOnly()) {
+			entry = path.fs.getEntry(path);
+		} else {
 			boolean create = false;
 			for (OpenOption o : options) {
 				if (o == StandardOpenOption.CREATE_NEW) {
-					if (entry != null) {
+					if (path.fs.exists(path)) {
 						throw new IOException("File already exists: " + path);
 					}
 					create = true;
@@ -258,11 +265,18 @@ public SeekableByteChannel newByteChannel(Path pathIn, Set<? extends OpenOption>
 				}
 			}
 
-			if (create && entry == null) {
-				if (!path.isAbsolute()) {
-					throw new IOException("Cannot work above the root!");
+			if (create) {
+				synchronized (path.fs) {
+					entry = path.fs.getEntry(path);
+					if (entry == null) {
+						if (!path.isAbsolute()) {
+							throw new IOException("Cannot work above the root!");
+						}
+						path.fs.addEntryRequiringParent(entry = new QuiltMemoryFile.ReadWrite(path));
+					}
 				}
-				path.fs.addEntryRequiringParent(entry = new QuiltMemoryFile.ReadWrite(path));
+			} else {
+				entry = path.fs.getEntry(path);
 			}
 		}
 
@@ -359,11 +373,13 @@ public boolean hasNext() {
 	@Override
 	public void createDirectory(Path dir, FileAttribute<?>... attrs) throws IOException {
 		P path = toAbsolutePath(dir);
-		if (path.fs.exists(path)) {
-			throw new FileAlreadyExistsException(path.toString());
+		synchronized (path.fs) {
+			if (path.fs.exists(path)) {
+				throw new FileAlreadyExistsException(path.toString());
+			}
+			ensureWriteable(path);
+			path.fs.addEntryRequiringParent(new QuiltUnifiedFolderWriteable(path));
 		}
-		ensureWriteable(path);
-		path.fs.addEntryRequiringParent(new QuiltUnifiedFolderWriteable(path));
 	}
 
 	@Override
@@ -391,7 +407,9 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep
 			return;
 		}
 
-		copy0(src, dst, false, options);
+		synchronized (dst.fs) {
+			copy0(src, dst, false, options);
+		}
 	}
 
 	@Override
@@ -403,7 +421,18 @@ public void move(Path source, Path target, CopyOption... options) throws IOExcep
 			return;
 		}
 
-		copy0(src, dst, true, options);
+		FS first = src.fs;
+		FS second = dst.fs;
+		if (first.syncOrder > second.syncOrder) {
+			first = dst.fs;
+			second = src.fs;
+		}
+
+		synchronized (first) {
+			synchronized (second) {
+				copy0(src, dst, true, options);
+			}
+		}
 	}
 
 	private void copy0(P src, P dst, boolean isMove, CopyOption[] options) throws IOException {

diff --git a/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltMemoryFileSystem.java b/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltMemoryFileSystem.java
@@ -520,26 +520,6 @@ public Path createFile(Path path, FileAttribute<?>... attrs) throws IOException
 			return super.createFile(path, attrs);
 		}
 
-		@Override
-		public Path createDirectories(Path dir, FileAttribute<?>... attrs) throws IOException {
-			dir = dir.toAbsolutePath().normalize();
-			Deque<Path> stack = new ArrayDeque<>();
-			Path p = dir;
-			do {
-				if (isDirectory(p)) {
-					break;
-				} else {
-					stack.push(p);
-				}
-			} while ((p = p.getParent()) != null);
-
-			while (!stack.isEmpty()) {
-				provider().createDirectory(stack.pop(), attrs);
-			}
-
-			return dir;
-		}
-
 		@Override
 		public boolean isWritable(Path path) {
 			return exists(path);

diff --git a/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltUnifiedFileSystem.java b/src/main/java/org/quiltmc/loader/impl/filesystem/QuiltUnifiedFileSystem.java
@@ -103,8 +103,6 @@ public Path copyOnWrite(Path source, Path target, CopyOption... options) throws
 			return copy(source, target, options);
 		}
 		QuiltUnifiedPath dst = provider().toAbsolutePath(target);
-		QuiltUnifiedEntry dstEntry = getEntry(dst);
-
 		boolean canExist = false;
 
 		for (CopyOption option : options) {
@@ -113,20 +111,20 @@ public Path copyOnWrite(Path source, Path target, CopyOption... options) throws
 			}
 		}
 
-		if (canExist) {
-			provider().deleteIfExists(dst);
-		} else if (dstEntry != null) {
-			throw new FileAlreadyExistsException(dst.toString());
+		synchronized (this) {
+			if (canExist) {
+				provider().deleteIfExists(dst);
+			} else if (getEntry(dst) != null) {
+				throw new FileAlreadyExistsException(dst.toString());
+			}
+			addEntryRequiringParent(new QuiltUnifiedCopyOnWriteFile(dst, source));
 		}
-
-		addEntryRequiringParent(new QuiltUnifiedCopyOnWriteFile(dst, source));
 		return dst;
 	}
 
 	@Override
 	public Path mount(Path source, Path target, MountOption... options) throws IOException {
 		QuiltUnifiedPath dst = provider().toAbsolutePath(target);
-		QuiltUnifiedEntry dstEntry = getEntry(dst);
 
 		boolean canExist = false;
 		boolean readOnly = false;
@@ -156,20 +154,23 @@ public Path mount(Path source, Path target, MountOption... options) throws IOExc
 			throw new IllegalArgumentException("Can't specify both READ_ONLY and COPY_ON_WRITE : " + Arrays.toString(options));
 		}
 
+		synchronized (this) {
+			QuiltUnifiedEntry dstEntry = getEntry(dst);
 
-		if (canExist) {
-			provider().deleteIfExists(dst);
-		} else if (dstEntry != null) {
-			throw new FileAlreadyExistsException(dst.toString());
-		}
+			if (canExist) {
+				provider().deleteIfExists(dst);
+			} else if (dstEntry != null) {
+				throw new FileAlreadyExistsException(dst.toString());
+			}
 
-		if (copyOnWrite) {
-			dstEntry = new QuiltUnifiedCopyOnWriteFile(dst, source);
-		} else {
-			dstEntry = new QuiltUnifiedMountedFile(dst, source, readOnly);
+			if (copyOnWrite) {
+				dstEntry = new QuiltUnifiedCopyOnWriteFile(dst, source);
+			} else {
+				dstEntry = new QuiltUnifiedMountedFile(dst, source, readOnly);
+			}
+			addEntryRequiringParent(dstEntry);
+			return dst;
 		}
-		addEntryRequiringParent(dstEntry);
-		return dst;
 	}
 
 	@Override

diff --git a/src/main/resources/changelog/0.22.0.txt b/src/main/resources/changelog/0.22.0.txt
@@ -10,4 +10,9 @@ Features:
         - "loader.mod_solving.print_results" prints unsolved sub-problems, and the final chosen options.
         - This is an alternative to the very verbose "loader.debug.mod_solving" property.
     - Made the log line "Aborted mod solving optimisation due to timeout" always print when it happens.
+- Removed the "transformed-mod-" prefix from virtual mod filesystem names, as it confused a lot of people.
+- Added the mod version after the mod id to filesystem names.
 
+Bug Fixes:
+
+- [#382] Fixed the virtual file systems not being thread-safe