Remove FileSystemMount and rewrite JarMount

FileSystemMount was originally added to allow using ReadableByteChannels
instead of InputStreams. However, as zip files do not allow seeking,
there is no benefit of using them over the original JarMount (which we
need to preserve for backwards compatibility).

Instead of maintaining two near-identical mounts, we remove the
FileSystemMount and rewrite the JarMount implementation with several
improvements:

 - Rewrite the jar scanning algorithm to be closer to 1.13+'s data pack
   mount. This means we no longer require the jar file to have
   directories before the file (though this was not a problem in
   practice).
 - Add all JarMounts to a ReferenceQueue, closing up the ZipFile when
   they have been garbage collected (fixes #100).
 - Cache the contents of all files for 60 seconds (with some constraints
   on size). This allows us to seek on ROM files too (assuming they are
   small), by reading the whole thing into memory.
   The cache is shared across all mounts, and has a 64MiB limit, and
   thus should not have an adverse impact on memory.

src/main/java/dan200/computercraft/ComputerCraft.java

@@ -26,7 +26,7 @@ import dan200.computercraft.core.apis.ApiFactories;
 import dan200.computercraft.core.apis.http.websocket.Websocket;
 import dan200.computercraft.core.filesystem.ComboMount;
 import dan200.computercraft.core.filesystem.FileMount;
-import dan200.computercraft.core.filesystem.FileSystemMount;
+import dan200.computercraft.core.filesystem.JarMount;
 import dan200.computercraft.core.terminal.Terminal;
 import dan200.computercraft.core.tracking.Tracking;
 import dan200.computercraft.shared.*;
@@ -92,13 +92,9 @@ import java.io.*;
 import java.net.MalformedURLException;
 import java.net.URISyntaxException;
 import java.net.URL;
-import java.nio.file.FileSystem;
-import java.nio.file.FileSystems;
-import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.EnumSet;
 import java.util.List;
-import java.util.ServiceConfigurationError;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
 
@@ -498,13 +494,11 @@ public class ComputerCraft
         {
             try
             {
-                FileSystem fs = FileSystems.newFileSystem( modJar.toPath(), ComputerCraft.class.getClassLoader() );
+                mounts.add( new JarMount( modJar, subPath ) );
-                mounts.add( new FileSystemMount( fs, subPath ) );
             }
-            catch( IOException | RuntimeException | ServiceConfigurationError e )
+            catch( IOException | RuntimeException e )
             {
                 ComputerCraft.log.error( "Could not load mount from mod jar", e );
-                // Ignore
             }
         }
 
@@ -521,21 +515,16 @@ public class ComputerCraft
                     if( !resourcePack.isDirectory() )
                     {
                         // Mount a resource pack from a jar
-                        FileSystem fs = FileSystems.newFileSystem( resourcePack.toPath(), ComputerCraft.class.getClassLoader() );
+                        mounts.add( new JarMount( resourcePack, subPath ) );
-                        if( Files.exists( fs.getPath( subPath ) ) ) mounts.add( new FileSystemMount( fs, subPath ) );
                     }
                     else
                     {
                         // Mount a resource pack from a folder
                         File subResource = new File( resourcePack, subPath );
-                        if( subResource.exists() )
+                        if( subResource.exists() ) mounts.add( new FileMount( subResource, 0 ) );
-                        {
-                            IMount resourcePackMount = new FileMount( subResource, 0 );
-                            mounts.add( resourcePackMount );
-                        }
                     }
                 }
-                catch( IOException | RuntimeException | ServiceConfigurationError e )
+                catch( IOException | RuntimeException e )
                 {
                     ComputerCraft.log.error( "Could not load resource pack '" + resourcePackName + "'", e );
                 }

src/main/java/dan200/computercraft/core/filesystem/FileSystemMount.java

@@ -1,144 +0,0 @@
-/*
- * This file is part of ComputerCraft - http://www.computercraft.info
- * Copyright Daniel Ratcliffe, 2011-2019. Do not distribute without permission.
- * Send enquiries to dratcliffe@gmail.com
- */
-
-package dan200.computercraft.core.filesystem;
-
-import dan200.computercraft.api.filesystem.IMount;
-
-import javax.annotation.Nonnull;
-import java.io.IOException;
-import java.io.InputStream;
-import java.nio.channels.ReadableByteChannel;
-import java.nio.file.FileSystem;
-import java.nio.file.*;
-import java.nio.file.attribute.BasicFileAttributes;
-import java.util.*;
-import java.util.stream.Stream;
-
-public class FileSystemMount implements IMount
-{
-    private final Entry rootEntry;
-
-    public FileSystemMount( FileSystem fileSystem, String root ) throws IOException
-    {
-        Path rootPath = fileSystem.getPath( root );
-        rootEntry = new Entry( "", rootPath );
-        populate( rootEntry );
-    }
-
-    private void populate( Entry root ) throws IOException
-    {
-        if( !root.directory ) return;
-
-        Queue<Entry> entries = new ArrayDeque<>();
-        entries.add( root );
-        while( !entries.isEmpty() )
-        {
-            Entry entry = entries.remove();
-            try( Stream<Path> childStream = Files.list( entry.path ) )
-            {
-                Iterator<Path> children = childStream.iterator();
-                while( children.hasNext() )
-                {
-                    Path childPath = children.next();
-                    Entry child = new Entry( childPath.getFileName().toString(), childPath );
-                    entry.children.put( child.name, child );
-                    if( child.directory ) entries.add( child );
-                }
-            }
-        }
-    }
-
-    @Override
-    public boolean exists( @Nonnull String path )
-    {
-        return getFile( path ) != null;
-    }
-
-    @Override
-    public boolean isDirectory( @Nonnull String path )
-    {
-        Entry entry = getFile( path );
-        return entry != null && entry.directory;
-    }
-
-    @Override
-    public void list( @Nonnull String path, @Nonnull List<String> contents ) throws IOException
-    {
-        Entry entry = getFile( path );
-        if( entry == null || !entry.directory ) throw new IOException( "/" + path + ": Not a directory" );
-
-        contents.addAll( entry.children.keySet() );
-    }
-
-    @Override
-    public long getSize( @Nonnull String path ) throws IOException
-    {
-        Entry file = getFile( path );
-        if( file == null ) throw new IOException( "/" + path + ": No such file" );
-        return file.size;
-    }
-
-    @Nonnull
-    @Override
-    @Deprecated
-    public InputStream openForRead( @Nonnull String path ) throws IOException
-    {
-        Entry file = getFile( path );
-        if( file == null || file.directory ) throw new IOException( "/" + path + ": No such file" );
-
-        return Files.newInputStream( file.path, StandardOpenOption.READ );
-    }
-
-    @Nonnull
-    @Override
-    public ReadableByteChannel openChannelForRead( @Nonnull String path ) throws IOException
-    {
-        Entry file = getFile( path );
-        if( file == null || file.directory ) throw new IOException( "/" + path + ": No such file" );
-
-        return Files.newByteChannel( file.path, StandardOpenOption.READ );
-    }
-
-    private Entry getFile( String path )
-    {
-        if( path.equals( "" ) ) return rootEntry;
-        if( !path.contains( "/" ) ) return rootEntry.children.get( path );
-
-        String[] components = path.split( "/" );
-        Entry entry = rootEntry;
-        for( String component : components )
-        {
-            if( entry == null || entry.children == null ) return null;
-            entry = entry.children.get( component );
-        }
-
-        return entry;
-    }
-
-    private static class Entry
-    {
-        final String name;
-        final Path path;
-
-        final boolean directory;
-        final long size;
-        final Map<String, Entry> children;
-
-        private Entry( String name, Path path ) throws IOException
-        {
-            if( name.endsWith( "/" ) || name.endsWith( "\\" ) ) name = name.substring( 0, name.length() - 1 );
-
-            this.name = name;
-            this.path = path;
-
-            BasicFileAttributes attributes = Files.readAttributes( path, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS );
-            this.directory = attributes.isDirectory();
-            this.size = directory ? 0 : attributes.size();
-            this.children = directory ? new HashMap<>() : null;
-        }
-    }
-}

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

@@ -6,242 +6,266 @@
 
 package dan200.computercraft.core.filesystem;
 
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.io.ByteStreams;
 import dan200.computercraft.api.filesystem.IMount;
+import dan200.computercraft.core.apis.handles.ArrayByteChannel;
+import dan200.computercraft.shared.util.IoUtil;
 
 import javax.annotation.Nonnull;
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
+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.util.Enumeration;
-import java.util.LinkedHashMap;
+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;
 
-@Deprecated
 public class JarMount implements IMount
 {
-    private static class FileInZip
+    /**
-    {
+     * Only cache files smaller than 1MiB.
-        private String m_path;
+     */
-        private boolean m_directory;
+    private static final int MAX_CACHED_SIZE = 1 << 20;
-        private long m_size;
-        private Map<String, FileInZip> m_children;
 
-        public FileInZip( String path, boolean directory, long size )
+    /**
-        {
+     * Limit the entire cache to 64MiB.
-            m_path = path;
+     */
-            m_directory = directory;
+    private static final int MAX_CACHE_SIZE = 64 << 20;
-            m_size = m_directory ? 0 : size;
-            m_children = new LinkedHashMap<>();
-        }
 
-        public String getPath()
+    /**
-        {
+     * We maintain a cache of the contents of all files in the mount. This allows us to allow
-            return m_path;
+     * 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();
 
-        public boolean isDirectory()
+    /**
-        {
+     * We have a {@link ReferenceQueue} of all mounts, a long with their corresponding {@link ZipFile}. If
-            return m_directory;
+     * the mount has been destroyed, we clean up after it.
-        }
+     */
+    private static final ReferenceQueue<JarMount> MOUNT_QUEUE = new ReferenceQueue<>();
 
-        public long getSize()
+    private final ZipFile zip;
-        {
+    private final FileEntry root;
-            return m_size;
-        }
 
-        public void list( List<String> contents )
-        {
-            contents.addAll( m_children.keySet() );
-        }
-
-        public void insertChild( FileInZip child )
-        {
-            String localPath = FileSystem.toLocal( child.getPath(), m_path );
-            m_children.put( localPath, child );
-        }
-
-        public FileInZip getFile( String path )
-        {
-            // If we've reached the target, return this
-            if( path.equals( m_path ) )
-            {
-                return this;
-            }
-
-            // Otherwise, get the next component of the path
-            String localPath = FileSystem.toLocal( path, m_path );
-            int slash = localPath.indexOf( "/" );
-            if( slash >= 0 )
-            {
-                localPath = localPath.substring( 0, slash );
-            }
-
-            // And recurse down using it
-            FileInZip subFile = m_children.get( localPath );
-            if( subFile != null )
-            {
-                return subFile.getFile( path );
-            }
-
-            return null;
-        }
-
-        public FileInZip getParent( String path )
-        {
-            if( path.length() == 0 )
-            {
-                return null;
-            }
-
-            FileInZip file = getFile( FileSystem.getDirectory( path ) );
-            if( file.isDirectory() )
-            {
-                return file;
-            }
-            return null;
-        }
-    }
-
-    private ZipFile m_zipFile;
-    private FileInZip m_root;
-    private String m_rootPath;
-
-    @Deprecated
     public JarMount( File jarFile, String subPath ) throws IOException
     {
-        if( !jarFile.exists() || jarFile.isDirectory() )
+        // Cleanup any old mounts. It's unlikely that there will be any, but it's best to be safe.
-        {
+        cleanup();
-            throw new FileNotFoundException();
+
-        }
+        if( !jarFile.exists() || jarFile.isDirectory() ) throw new FileNotFoundException();
 
         // Open the zip file
         try
         {
-            m_zipFile = new ZipFile( jarFile );
+            zip = new ZipFile( jarFile );
         }
         catch( Exception e )
         {
             throw new IOException( "Error loading zip file" );
         }
 
-        if( m_zipFile.getEntry( subPath ) == null )
+        // Ensure the root entry exists.
+        if( zip.getEntry( subPath ) == null )
         {
-            m_zipFile.close();
+            zip.close();
             throw new IOException( "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
-        Enumeration<? extends ZipEntry> zipEntries = m_zipFile.entries();
+        root = new FileEntry( "" );
+        Enumeration<? extends ZipEntry> zipEntries = zip.entries();
         while( zipEntries.hasMoreElements() )
         {
             ZipEntry entry = zipEntries.nextElement();
-            String entryName = entry.getName();
+
-            if( entryName.startsWith( subPath ) )
+            String entryPath = entry.getName();
-            {
+            if( !entryPath.startsWith( subPath ) ) continue;
-                entryName = FileSystem.toLocal( entryName, subPath );
+
-                if( m_root == null )
+            String localPath = FileSystem.toLocal( entryPath, subPath );
-                {
+            create( entry, localPath );
-                    if( entryName.equals( "" ) )
-                    {
-                        m_root = new FileInZip( entryName, entry.isDirectory(), entry.getSize() );
-                        m_rootPath = subPath;
-                        if( !m_root.isDirectory() ) break;
-                    }
-                    else
-                    {
-                        // TODO: handle this case. The code currently assumes we find the root before anything else
-                    }
-                }
-                else
-                {
-                    FileInZip parent = m_root.getParent( entryName );
-                    if( parent != null )
-                    {
-                        parent.insertChild( new FileInZip( entryName, entry.isDirectory(), entry.getSize() ) );
-                    }
-                    else
-                    {
-                        // TODO: handle this case. The code currently assumes we find folders before their contents
-                    }
-                }
-            }
         }
     }
 
-    // IMount implementation
+    private FileEntry get( String path )
+    {
+        FileEntry lastEntry = root;
+        int lastIndex = 0;
+
+        while( lastEntry != null && lastIndex < path.length() )
+        {
+            int 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 )
+    {
+        FileEntry lastEntry = root;
+
+        int lastIndex = 0;
+        while( lastIndex < localPath.length() )
+        {
+            int nextIndex = localPath.indexOf( '/', lastIndex );
+            if( nextIndex < 0 ) nextIndex = localPath.length();
+
+            String part = localPath.substring( lastIndex, nextIndex );
+            if( lastEntry.children == null ) lastEntry.children = new HashMap<>( 0 );
+
+            FileEntry nextEntry = lastEntry.children.get( part );
+            if( nextEntry == null || !nextEntry.isDirectory() )
+            {
+                lastEntry.children.put( part, nextEntry = new FileEntry( part ) );
+            }
+
+            lastEntry = nextEntry;
+            lastIndex = nextIndex + 1;
+        }
+
+        lastEntry.setup( entry );
+    }
 
     @Override
     public boolean exists( @Nonnull String path )
     {
-        FileInZip file = m_root.getFile( path );
+        return get( path ) != null;
-        return file != null;
     }
 
     @Override
     public boolean isDirectory( @Nonnull String path )
     {
-        FileInZip file = m_root.getFile( path );
+        FileEntry file = get( path );
-        if( file != null )
+        return file != null && file.isDirectory();
-        {
-            return file.isDirectory();
-        }
-        return false;
     }
 
     @Override
     public void list( @Nonnull String path, @Nonnull List<String> contents ) throws IOException
     {
-        FileInZip file = m_root.getFile( path );
+        FileEntry file = get( path );
-        if( file != null && file.isDirectory() )
+        if( file == null || !file.isDirectory() ) throw new IOException( "/" + path + ": Not a directory" );
-        {
+
-            file.list( contents );
+        file.list( contents );
-        }
-        else
-        {
-            throw new IOException( "/" + path + ": Not a directory" );
-        }
     }
 
     @Override
     public long getSize( @Nonnull String path ) throws IOException
     {
-        FileInZip file = m_root.getFile( path );
+        FileEntry file = get( path );
-        if( file != null )
+        if( file != null ) return file.size;
-        {
-            return file.getSize();
-        }
         throw new IOException( "/" + path + ": No such file" );
     }
 
     @Nonnull
     @Override
+    @Deprecated
     public InputStream openForRead( @Nonnull String path ) throws IOException
     {
-        FileInZip file = m_root.getFile( path );
+        return Channels.newInputStream( openChannelForRead( path ) );
+    }
+
+    @Nonnull
+    @Override
+    public ReadableByteChannel openChannelForRead( @Nonnull String path ) throws IOException
+    {
+        FileEntry file = get( path );
         if( file != null && !file.isDirectory() )
         {
+            byte[] contents = CONTENTS_CACHE.getIfPresent( file );
+            if( contents != null ) return new ArrayByteChannel( contents );
+
             try
             {
-                String fullPath = m_rootPath;
+                ZipEntry entry = zip.getEntry( file.path );
-                if( path.length() > 0 )
-                {
-                    fullPath = fullPath + "/" + path;
-                }
-                ZipEntry entry = m_zipFile.getEntry( fullPath );
                 if( entry != null )
                 {
-                    return m_zipFile.getInputStream( entry );
+                    try( InputStream 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( Exception e )
             {
-                // treat errors as non-existance of file
+                // Treat errors as non-existence of file
             }
         }
+
         throw new IOException( "/" + path + ": No such file" );
     }
+
+    private static class FileEntry
+    {
+        final String name;
+
+        String path;
+        long size;
+        Map<String, FileEntry> children;
+
+        FileEntry( String name )
+        {
+            this.name = name;
+        }
+
+        void setup( ZipEntry entry )
+        {
+            path = entry.getName();
+            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 );
+    }
 }

src/test/java/dan200/computercraft/core/filesystem/FilesystemMountTest.java

@@ -1,63 +0,0 @@
-/*
- * This file is part of ComputerCraft - http://www.computercraft.info
- * Copyright Daniel Ratcliffe, 2011-2019. Do not distribute without permission.
- * Send enquiries to dratcliffe@gmail.com
- */
-
-package dan200.computercraft.core.filesystem;
-
-import dan200.computercraft.api.filesystem.IMount;
-import org.junit.BeforeClass;
-import org.junit.Test;
-
-import java.io.File;
-import java.io.FileOutputStream;
-import java.io.IOException;
-import java.nio.charset.StandardCharsets;
-import java.nio.file.FileSystem;
-import java.nio.file.FileSystems;
-import java.util.zip.ZipEntry;
-import java.util.zip.ZipOutputStream;
-
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
-public class FilesystemMountTest
-{
-    private static final File ZIP_FILE = new File( "test-files/filesystem-mount.zip" );
-
-    @BeforeClass
-    public static void before() throws IOException
-    {
-        if( ZIP_FILE.exists() ) return;
-        ZIP_FILE.getParentFile().mkdirs();
-
-        try( ZipOutputStream stream = new ZipOutputStream( new FileOutputStream( ZIP_FILE ) ) )
-        {
-            stream.putNextEntry( new ZipEntry( "dir/" ) );
-            stream.closeEntry();
-
-            stream.putNextEntry( new ZipEntry( "dir/file.lua" ) );
-            stream.write( "print('testing')".getBytes( StandardCharsets.UTF_8 ) );
-            stream.closeEntry();
-        }
-    }
-
-    @Test
-    public void mountsDir() throws IOException
-    {
-        FileSystem fs = FileSystems.newFileSystem( ZIP_FILE.toPath(), getClass().getClassLoader() );
-        IMount mount = new FileSystemMount( fs, "dir" );
-        assertTrue( "Root should be directory", mount.isDirectory( "" ) );
-        assertTrue( "File should exist", mount.exists( "file.lua" ) );
-    }
-
-    @Test
-    public void mountsFile() throws IOException
-    {
-        FileSystem fs = FileSystems.newFileSystem( ZIP_FILE.toPath(), getClass().getClassLoader() );
-        IMount mount = new FileSystemMount( fs, "dir/file.lua" );
-        assertTrue( "Root should exist", mount.exists( "" ) );
-        assertFalse( "Root should be a file", mount.isDirectory( "" ) );
-    }
-}

src/test/java/dan200/computercraft/core/filesystem/JarMountTest.java

@@ -6,6 +6,7 @@
 
 package dan200.computercraft.core.filesystem;
 
+import com.google.common.io.ByteStreams;
 import dan200.computercraft.api.filesystem.IMount;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -13,12 +14,12 @@ import org.junit.Test;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipOutputStream;
 
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.*;
-import static org.junit.Assert.assertTrue;
 
 @SuppressWarnings( "deprecation" )
 public class JarMountTest
@@ -57,4 +58,30 @@ public class JarMountTest
         assertTrue( "Root should exist", mount.exists( "" ) );
         assertFalse( "Root should be a file", mount.isDirectory( "" ) );
     }
+
+    @Test
+    public void opensFileFromFile() throws IOException
+    {
+        IMount mount = new JarMount( ZIP_FILE, "dir/file.lua" );
+        byte[] contents;
+        try( InputStream stream = mount.openForRead( "" ) )
+        {
+            contents = ByteStreams.toByteArray( stream );
+        }
+
+        assertEquals( "print('testing')", new String( contents, StandardCharsets.UTF_8 ) );
+    }
+
+    @Test
+    public void opensFileFromDir() throws IOException
+    {
+        IMount mount = new JarMount( ZIP_FILE, "dir" );
+        byte[] contents;
+        try( InputStream stream = mount.openForRead( "file.lua" ) )
+        {
+            contents = ByteStreams.toByteArray( stream );
+        }
+
+        assertEquals( "print('testing')", new String( contents, StandardCharsets.UTF_8 ) );
+    }
 }