Refactor common {Jar,Resource}Mount code into a parent class

projects/common/src/main/java/dan200/computercraft/shared/computer/core/ResourceMount.java

@@ -5,57 +5,32 @@
  */
 package dan200.computercraft.shared.computer.core;
 
-import com.google.common.cache.Cache;
+import dan200.computercraft.core.filesystem.ArchiveMount;
-import com.google.common.cache.CacheBuilder;
-import com.google.common.io.ByteStreams;
-import dan200.computercraft.api.filesystem.Mount;
-import dan200.computercraft.core.apis.handles.ArrayByteChannel;
 import dan200.computercraft.core.filesystem.FileSystem;
-import dan200.computercraft.core.util.IoUtil;
 import net.minecraft.ResourceLocationException;
 import net.minecraft.resources.ResourceLocation;
+import net.minecraft.server.MinecraftServer;
 import net.minecraft.server.packs.resources.ResourceManager;
 import net.minecraft.server.packs.resources.SimplePreparableReloadListener;
 import net.minecraft.util.profiling.ProfilerFiller;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import javax.annotation.Nullable;
+import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.nio.channels.Channels;
-import java.nio.channels.ReadableByteChannel;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
-import java.util.concurrent.TimeUnit;
 
-public final class ResourceMount implements Mount {
+/**
+ * A mount backed by Minecraft's {@link ResourceManager}.
+ *
+ * @see dan200.computercraft.api.ComputerCraftAPI#createResourceMount(MinecraftServer, String, String)
+ */
+public final class ResourceMount extends ArchiveMount<ResourceMount.FileEntry> {
     private static final Logger LOG = LoggerFactory.getLogger(ResourceMount.class);
 
-    /**
-     * Only cache files smaller than 1MiB.
-     */
-    private static final int MAX_CACHED_SIZE = 1 << 20;
-
-    /**
-     * Limit the entire cache to 64MiB.
-     */
-    private static final int MAX_CACHE_SIZE = 64 << 20;
-
     private static final byte[] TEMP_BUFFER = new byte[8192];
 
-    /**
-     * We maintain a cache of the contents of all files in the mount. This allows us to allow
-     * seeking within ROM files, and reduces the amount we need to access disk for computer startup.
-     */
-    private static final Cache<FileEntry, byte[]> CONTENTS_CACHE = CacheBuilder.newBuilder()
-        .concurrencyLevel(4)
-        .expireAfterAccess(60, TimeUnit.SECONDS)
-        .maximumWeight(MAX_CACHE_SIZE)
-        .weakKeys()
-        .<FileEntry, byte[]>weigher((k, v) -> v.length)
-        .build();
-
     /**
      * Maintain a cache of currently loaded resource mounts. This cache is invalidated when currentManager changes.
      */
@@ -65,9 +40,6 @@ public final class ResourceMount implements Mount {
     private final String subPath;
     private ResourceManager manager;
 
-    @Nullable
-    private FileEntry root;
-
     public static ResourceMount get(String namespace, String subPath, ResourceManager manager) {
         var path = new ResourceLocation(namespace, subPath);
         synchronized (MOUNT_CACHE) {
@@ -110,21 +82,6 @@ public final class ResourceMount implements Mount {
         }
     }
 
-    private @Nullable FileEntry get(String path) {
-        var lastEntry = root;
-        var lastIndex = 0;
-
-        while (lastEntry != null && lastIndex < path.length()) {
-            var nextIndex = path.indexOf('/', lastIndex);
-            if (nextIndex < 0) nextIndex = path.length();
-
-            lastEntry = lastEntry.children == null ? null : lastEntry.children.get(path.substring(lastIndex, nextIndex));
-            lastIndex = nextIndex + 1;
-        }
-
-        return lastEntry;
-    }
-
     private void create(FileEntry lastEntry, String path) {
         var lastIndex = 0;
         while (lastIndex < path.length()) {
@@ -152,96 +109,39 @@ public final class ResourceMount implements Mount {
     }
 
     @Override
-    public boolean exists(String path) {
+    public long getSize(FileEntry file) {
-        return get(path) != null;
+        var resource = manager.getResource(file.identifier).orElse(null);
-    }
+        if (resource == null) return 0;
 
-    @Override
+        try (var stream = resource.open()) {
-    public boolean isDirectory(String path) {
+            int total = 0, read = 0;
-        var file = get(path);
+            do {
-        return file != null && file.isDirectory();
+                total += read;
-    }
+                read = stream.read(TEMP_BUFFER);
+            } while (read > 0);
 
-    @Override
+            return total;
-    public void list(String path, List<String> contents) throws IOException {
+        } catch (IOException e) {
-        var file = get(path);
+            return 0;
-        if (file == null || !file.isDirectory()) throw new IOException("/" + path + ": Not a directory");
-
-        file.list(contents);
-    }
-
-    @Override
-    public long getSize(String path) throws IOException {
-        var file = get(path);
-        if (file != null) {
-            if (file.size != -1) return file.size;
-            if (file.isDirectory()) return file.size = 0;
-
-            var contents = CONTENTS_CACHE.getIfPresent(file);
-            if (contents != null) return file.size = contents.length;
-
-            var resource = manager.getResource(file.identifier).orElse(null);
-            if (resource == null) return file.size = 0;
-
-            try (var s = resource.open()) {
-                int total = 0, read = 0;
-                do {
-                    total += read;
-                    read = s.read(TEMP_BUFFER);
-                } while (read > 0);
-
-                return file.size = total;
-            } catch (IOException e) {
-                return file.size = 0;
-            }
         }
-
-        throw new IOException("/" + path + ": No such file");
     }
 
     @Override
-    public ReadableByteChannel openForRead(String path) throws IOException {
+    public byte[] getContents(FileEntry file) throws IOException {
-        var file = get(path);
+        var resource = manager.getResource(file.identifier).orElse(null);
-        if (file != null && !file.isDirectory()) {
+        if (resource == null) throw new FileNotFoundException(NO_SUCH_FILE);
-            var contents = CONTENTS_CACHE.getIfPresent(file);
-            if (contents != null) return new ArrayByteChannel(contents);
 
-            var resource = manager.getResource(file.identifier).orElse(null);
+        try (var stream = resource.open()) {
-            if (resource != null) {
+            return stream.readAllBytes();
-                var stream = resource.open();
-                if (stream.available() > MAX_CACHED_SIZE) return Channels.newChannel(stream);
-
-                try {
-                    contents = ByteStreams.toByteArray(stream);
-                } finally {
-                    IoUtil.closeQuietly(stream);
-                }
-
-                CONTENTS_CACHE.put(file, contents);
-                return new ArrayByteChannel(contents);
-            }
         }
-
-        throw new IOException("/" + path + ": No such file");
     }
 
-    private static class FileEntry {
+    protected static class FileEntry extends ArchiveMount.FileEntry<FileEntry> {
         final ResourceLocation identifier;
-        @Nullable
-        Map<String, FileEntry> children;
-        long size = -1;
 
         FileEntry(ResourceLocation identifier) {
             this.identifier = identifier;
         }
-
-        boolean isDirectory() {
-            return children != null;
-        }
-
-        void list(List<String> contents) {
-            if (children != null) contents.addAll(children.keySet());
-        }
     }
 
     /**
@@ -254,7 +154,6 @@ public final class ResourceMount implements Mount {
             profiler.push("Reloading ComputerCraft mounts");
             try {
                 for (var mount : MOUNT_CACHE.values()) mount.load(manager);
-                CONTENTS_CACHE.invalidateAll();
             } finally {
                 profiler.pop();
             }

projects/core-api/src/main/java/dan200/computercraft/api/filesystem/FileAttributes.java

@@ -16,7 +16,7 @@ import java.time.Instant;
  * @param isDirectory Whether this filesystem entry is a directory.
  * @param size        The size of the file.
  */
-record FileAttributes(boolean isDirectory, long size) implements BasicFileAttributes {
+public record FileAttributes(boolean isDirectory, long size) implements BasicFileAttributes {
     private static final FileTime EPOCH = FileTime.from(Instant.EPOCH);
 
     @Override

projects/core/src/main/java/dan200/computercraft/core/filesystem/ArchiveMount.java

@@ -0,0 +1,166 @@
+/*
+ * This file is part of ComputerCraft - http://www.computercraft.info
+ * Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission.
+ * Send enquiries to dratcliffe@gmail.com
+ */
+package dan200.computercraft.core.filesystem;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import dan200.computercraft.api.filesystem.FileAttributes;
+import dan200.computercraft.api.filesystem.FileOperationException;
+import dan200.computercraft.api.filesystem.Mount;
+import dan200.computercraft.core.apis.handles.ArrayByteChannel;
+
+import javax.annotation.Nullable;
+import java.io.IOException;
+import java.nio.channels.ReadableByteChannel;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * An abstract mount based on some archive of files, such as a Zip or Minecraft's resources.
+ *
+ * @param <T> The type of file.
+ */
+public abstract class ArchiveMount<T extends ArchiveMount.FileEntry<T>> implements Mount {
+    protected static final String NO_SUCH_FILE = "No such file";
+    /**
+     * Limit the entire cache to 64MiB.
+     */
+    private static final int MAX_CACHE_SIZE = 64 << 20;
+
+    /**
+     * We maintain a cache of the contents of all files in the mount. This allows us to allow
+     * seeking within ROM files, and reduces the amount we need to access disk for computer startup.
+     */
+    private static final Cache<FileEntry<?>, byte[]> CONTENTS_CACHE = CacheBuilder.newBuilder()
+        .concurrencyLevel(4)
+        .expireAfterAccess(60, TimeUnit.SECONDS)
+        .maximumWeight(MAX_CACHE_SIZE)
+        .weakKeys()
+        .<FileEntry<?>, byte[]>weigher((k, v) -> v.length)
+        .build();
+
+    @Nullable
+    protected T root;
+
+    private @Nullable T get(String path) {
+        var lastEntry = root;
+        var lastIndex = 0;
+
+        while (lastEntry != null && lastIndex < path.length()) {
+            var nextIndex = path.indexOf('/', lastIndex);
+            if (nextIndex < 0) nextIndex = path.length();
+
+            lastEntry = lastEntry.children == null ? null : lastEntry.children.get(path.substring(lastIndex, nextIndex));
+            lastIndex = nextIndex + 1;
+        }
+
+        return lastEntry;
+    }
+
+    @Override
+    public final boolean exists(String path) {
+        return get(path) != null;
+    }
+
+    @Override
+    public final boolean isDirectory(String path) {
+        var file = get(path);
+        return file != null && file.isDirectory();
+    }
+
+    @Override
+    public final void list(String path, List<String> contents) throws IOException {
+        var file = get(path);
+        if (file == null || !file.isDirectory()) throw new FileOperationException(path, "Not a directory");
+
+        file.list(contents);
+    }
+
+    @Override
+    public final long getSize(String path) throws IOException {
+        var file = get(path);
+        if (file == null) throw new FileOperationException(path, NO_SUCH_FILE);
+        return getCachedSize(file);
+    }
+
+    private long getCachedSize(T file) throws IOException {
+        if (file.size != -1) return file.size;
+        if (file.isDirectory()) return file.size = 0;
+
+        var contents = CONTENTS_CACHE.getIfPresent(file);
+        if (contents != null) return file.size = contents.length;
+
+        return file.size = getSize(file);
+    }
+
+    /**
+     * Get the size of a file.
+     * <p>
+     * This should only be called once per file, as the result is cached in {@link #getSize(String)}.
+     *
+     * @param file The file to compute the size of.
+     * @return The size of the file.
+     * @throws IOException If the size could not be read.
+     */
+    protected abstract long getSize(T file) throws IOException;
+
+    @Override
+    public ReadableByteChannel openForRead(String path) throws IOException {
+        var file = get(path);
+        if (file == null || file.isDirectory()) throw new FileOperationException(path, NO_SUCH_FILE);
+
+        var cachedContents = CONTENTS_CACHE.getIfPresent(file);
+        if (cachedContents != null) return new ArrayByteChannel(cachedContents);
+
+        var contents = getContents(file);
+        CONTENTS_CACHE.put(file, contents);
+        return new ArrayByteChannel(contents);
+    }
+
+    /**
+     * Read the entirety of a file into memory.
+     *
+     * @param file The file to read into memory.
+     * @return The contents of the file.
+     */
+    protected abstract byte[] getContents(T file) throws IOException;
+
+    @Override
+    public final BasicFileAttributes getAttributes(String path) throws IOException {
+        var file = get(path);
+        if (file == null) throw new FileOperationException(path, NO_SUCH_FILE);
+
+        return getAttributes(file);
+    }
+
+    /**
+     * Get all attributes of the file.
+     *
+     * @param file The file to compute attributes for.
+     * @return The file's attributes.
+     * @throws IOException If the attributes could not be read.
+     */
+    protected BasicFileAttributes getAttributes(T file) throws IOException {
+        return new FileAttributes(file.isDirectory(), getCachedSize(file));
+    }
+
+    protected static class FileEntry<T extends ArchiveMount.FileEntry<T>> {
+        @Nullable
+        public Map<String, T> children;
+
+        long size = -1;
+
+        protected boolean isDirectory() {
+            return children != null;
+        }
+
+        protected void list(List<String> contents) {
+            if (children != null) contents.addAll(children.keySet());
+        }
+    }
+}

projects/core/src/main/java/dan200/computercraft/core/filesystem/JarMount.java

@@ -5,70 +5,27 @@
  */
 package dan200.computercraft.core.filesystem;
 
-import com.google.common.cache.Cache;
+import dan200.computercraft.core.util.Nullability;
-import com.google.common.cache.CacheBuilder;
-import com.google.common.io.ByteStreams;
-import com.google.errorprone.annotations.concurrent.LazyInit;
-import dan200.computercraft.api.filesystem.FileOperationException;
-import dan200.computercraft.api.filesystem.Mount;
-import dan200.computercraft.core.apis.handles.ArrayByteChannel;
-import dan200.computercraft.core.util.IoUtil;
 
 import javax.annotation.Nullable;
+import java.io.Closeable;
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.lang.ref.Reference;
-import java.lang.ref.ReferenceQueue;
-import java.lang.ref.WeakReference;
-import java.nio.channels.Channels;
-import java.nio.channels.ReadableByteChannel;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.attribute.FileTime;
 import java.time.Instant;
 import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.TimeUnit;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
 
-public class JarMount implements Mount {
+/**
-    /**
+ * A mount which reads zip/jar files.
-     * Only cache files smaller than 1MiB.
+ */
-     */
+public class JarMount extends ArchiveMount<JarMount.FileEntry> implements Closeable {
-    private static final int MAX_CACHED_SIZE = 1 << 20;
-
-    /**
-     * Limit the entire cache to 64MiB.
-     */
-    private static final int MAX_CACHE_SIZE = 64 << 20;
-
-    /**
-     * We maintain a cache of the contents of all files in the mount. This allows us to allow
-     * seeking within ROM files, and reduces the amount we need to access disk for computer startup.
-     */
-    private static final Cache<FileEntry, byte[]> CONTENTS_CACHE = CacheBuilder.newBuilder()
-        .concurrencyLevel(4)
-        .expireAfterAccess(60, TimeUnit.SECONDS)
-        .maximumWeight(MAX_CACHE_SIZE)
-        .weakKeys()
-        .<FileEntry, byte[]>weigher((k, v) -> v.length)
-        .build();
-
-    /**
-     * We have a {@link ReferenceQueue} of all mounts, a long with their corresponding {@link ZipFile}. If
-     * the mount has been destroyed, we clean up after it.
-     */
-    private static final ReferenceQueue<JarMount> MOUNT_QUEUE = new ReferenceQueue<>();
-
     private final ZipFile zip;
-    private final FileEntry root;
 
     public JarMount(File jarFile, String subPath) throws IOException {
-        // Cleanup any old mounts. It's unlikely that there will be any, but it's best to be safe.
-        cleanup();
-
         if (!jarFile.exists() || jarFile.isDirectory()) throw new FileNotFoundException("Cannot find " + jarFile);
 
         // Open the zip file
@@ -84,9 +41,6 @@ public class JarMount implements Mount {
             throw new FileNotFoundException("Zip does not contain path");
         }
 
-        // We now create a weak reference to this mount. This is automatically added to the appropriate queue.
-        new MountReference(this);
-
         // Read in all the entries
         root = new FileEntry();
         var zipEntries = zip.entries();
@@ -101,24 +55,8 @@ public class JarMount implements Mount {
         }
     }
 
-    @Nullable
-    private FileEntry get(String path) {
-        var lastEntry = root;
-        var lastIndex = 0;
-
-        while (lastEntry != null && lastIndex < path.length()) {
-            var nextIndex = path.indexOf('/', lastIndex);
-            if (nextIndex < 0) nextIndex = path.length();
-
-            lastEntry = lastEntry.children == null ? null : lastEntry.children.get(path.substring(lastIndex, nextIndex));
-            lastIndex = nextIndex + 1;
-        }
-
-        return lastEntry;
-    }
-
     private void create(ZipEntry entry, String localPath) {
-        var lastEntry = root;
+        var lastEntry = Nullability.assertNonNull(root);
 
         var lastIndex = 0;
         while (lastIndex < localPath.length()) {
@@ -141,104 +79,43 @@ public class JarMount implements Mount {
     }
 
     @Override
-    public boolean exists(String path) {
+    protected long getSize(FileEntry file) throws IOException {
-        return get(path) != null;
+        if (file.zipEntry == null) throw new FileNotFoundException(NO_SUCH_FILE);
+        return file.zipEntry.getSize();
     }
 
     @Override
-    public boolean isDirectory(String path) {
+    protected byte[] getContents(FileEntry file) throws IOException {
-        var file = get(path);
+        if (file.zipEntry == null) throw new FileNotFoundException(NO_SUCH_FILE);
-        return file != null && file.isDirectory();
-    }
 
-    @Override
+        try (var stream = zip.getInputStream(file.zipEntry)) {
-    public void list(String path, List<String> contents) throws IOException {
+            return stream.readAllBytes();
-        var file = get(path);
+        } catch (IOException e) {
-        if (file == null || !file.isDirectory()) throw new FileOperationException(path, "Not a directory");
+            // Mask other IO exceptions as a non-existent file.
-
+            throw new FileNotFoundException(NO_SUCH_FILE);
-        file.list(contents);
-    }
-
-    @Override
-    public long getSize(String path) throws IOException {
-        var file = get(path);
-        if (file != null) return file.size;
-        throw new FileOperationException(path, "No such file");
-    }
-
-    @Override
-    public ReadableByteChannel openForRead(String path) throws IOException {
-        var file = get(path);
-        if (file != null && !file.isDirectory()) {
-            var contents = CONTENTS_CACHE.getIfPresent(file);
-            if (contents != null) return new ArrayByteChannel(contents);
-
-            try {
-                var entry = zip.getEntry(file.path);
-                if (entry != null) {
-                    try (var stream = zip.getInputStream(entry)) {
-                        if (stream.available() > MAX_CACHED_SIZE) return Channels.newChannel(stream);
-
-                        contents = ByteStreams.toByteArray(stream);
-                        CONTENTS_CACHE.put(file, contents);
-                        return new ArrayByteChannel(contents);
-                    }
-                }
-            } catch (IOException e) {
-                // Treat errors as non-existence of file
-            }
         }
-
-        throw new FileOperationException(path, "No such file");
     }
 
     @Override
-    public BasicFileAttributes getAttributes(String path) throws IOException {
+    public BasicFileAttributes getAttributes(FileEntry file) throws IOException {
-        var file = get(path);
+        if (file.zipEntry == null) throw new FileNotFoundException(NO_SUCH_FILE);
-        if (file != null) {
+        return new ZipEntryAttributes(file.zipEntry);
-            var entry = zip.getEntry(file.path);
-            if (entry != null) return new ZipEntryAttributes(entry);
-        }
-
-        throw new FileOperationException(path, "No such file");
     }
 
-    private static class FileEntry {
+    @Override
-        @LazyInit // TODO: Might be nicer to use @Initializer on setup(...)
+    public void close() throws IOException {
-        String path;
+        zip.close();
-
+    }
-        long size;
 
+    protected static class FileEntry extends ArchiveMount.FileEntry<FileEntry> {
         @Nullable
-        Map<String, FileEntry> children;
+        ZipEntry zipEntry;
 
         void setup(ZipEntry entry) {
-            path = entry.getName();
+            zipEntry = entry;
             size = entry.getSize();
             if (children == null && entry.isDirectory()) children = new HashMap<>(0);
         }
-
-        boolean isDirectory() {
-            return children != null;
-        }
-
-        void list(List<String> contents) {
-            if (children != null) contents.addAll(children.keySet());
-        }
-    }
-
-    private static class MountReference extends WeakReference<JarMount> {
-        final ZipFile file;
-
-        MountReference(JarMount file) {
-            super(file, MOUNT_QUEUE);
-            this.file = file.zip;
-        }
-    }
-
-    private static void cleanup() {
-        Reference<? extends JarMount> next;
-        while ((next = MOUNT_QUEUE.poll()) != null) IoUtil.closeQuietly(((MountReference) next).file);
     }
 
     private static class ZipEntryAttributes implements BasicFileAttributes {

projects/core/src/testFixtures/java/dan200/computercraft/test/core/computer/BasicEnvironment.java

@@ -104,7 +104,6 @@ public class BasicEnvironment implements ComputerEnvironment, GlobalEnvironment,
         }
     }
 
-
     private static File getContainingFile(Class<?> klass) {
         var path = klass.getProtectionDomain().getCodeSource().getLocation().getPath();
         var bangIndex = path.indexOf("!");