From 443e0f8f763ee779f5256253543f9c1acf34fc28 Mon Sep 17 00:00:00 2001 From: SquidDev Date: Wed, 16 Jan 2019 17:25:46 +0000 Subject: [PATCH] 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. --- .../dan200/computercraft/ComputerCraft.java | 23 +- .../core/filesystem/FileSystemMount.java | 144 -------- .../core/filesystem/JarMount.java | 334 ++++++++++-------- .../core/filesystem/FilesystemMountTest.java | 63 ---- .../core/filesystem/JarMountTest.java | 31 +- 5 files changed, 214 insertions(+), 381 deletions(-) delete mode 100644 src/main/java/dan200/computercraft/core/filesystem/FileSystemMount.java delete mode 100644 src/test/java/dan200/computercraft/core/filesystem/FilesystemMountTest.java diff --git a/src/main/java/dan200/computercraft/ComputerCraft.java b/src/main/java/dan200/computercraft/ComputerCraft.java index 580d4daed..f7162542c 100644 --- a/src/main/java/dan200/computercraft/ComputerCraft.java +++ b/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 FileSystemMount( fs, subPath ) ); + mounts.add( new JarMount( modJar, 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() ); - if( Files.exists( fs.getPath( subPath ) ) ) mounts.add( new FileSystemMount( fs, subPath ) ); + mounts.add( new JarMount( resourcePack, subPath ) ); } else { // Mount a resource pack from a folder File subResource = new File( resourcePack, subPath ); - if( subResource.exists() ) - { - IMount resourcePackMount = new FileMount( subResource, 0 ); - mounts.add( resourcePackMount ); - } + if( subResource.exists() ) mounts.add( new FileMount( subResource, 0 ) ); } } - catch( IOException | RuntimeException | ServiceConfigurationError e ) + catch( IOException | RuntimeException e ) { ComputerCraft.log.error( "Could not load resource pack '" + resourcePackName + "'", e ); } diff --git a/src/main/java/dan200/computercraft/core/filesystem/FileSystemMount.java b/src/main/java/dan200/computercraft/core/filesystem/FileSystemMount.java deleted file mode 100644 index ff83e8163..000000000 --- a/src/main/java/dan200/computercraft/core/filesystem/FileSystemMount.java +++ /dev/null @@ -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 entries = new ArrayDeque<>(); - entries.add( root ); - while( !entries.isEmpty() ) - { - Entry entry = entries.remove(); - try( Stream childStream = Files.list( entry.path ) ) - { - Iterator 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 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 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; - } - } -} diff --git a/src/main/java/dan200/computercraft/core/filesystem/JarMount.java b/src/main/java/dan200/computercraft/core/filesystem/JarMount.java index 8ec930462..dcda7e50b 100644 --- a/src/main/java/dan200/computercraft/core/filesystem/JarMount.java +++ b/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 - { - private String m_path; - private boolean m_directory; - private long m_size; - private Map m_children; + /** + * Only cache files smaller than 1MiB. + */ + private static final int MAX_CACHED_SIZE = 1 << 20; - public FileInZip( String path, boolean directory, long size ) - { - m_path = path; - m_directory = directory; - m_size = m_directory ? 0 : size; - m_children = new LinkedHashMap<>(); - } + /** + * Limit the entire cache to 64MiB. + */ + private static final int MAX_CACHE_SIZE = 64 << 20; - public String getPath() - { - return m_path; - } + /** + * 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 CONTENTS_CACHE = CacheBuilder.newBuilder() + .concurrencyLevel( 4 ) + .expireAfterAccess( 60, TimeUnit.SECONDS ) + .maximumWeight( MAX_CACHE_SIZE ) + .weakKeys() + .weigher( ( k, v ) -> v.length ) + .build(); - public boolean isDirectory() - { - return m_directory; - } + /** + * 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 MOUNT_QUEUE = new ReferenceQueue<>(); - public long getSize() - { - return m_size; - } + private final ZipFile zip; + private final FileEntry root; - public void list( List 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() ) - { - throw new FileNotFoundException(); - } + // 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(); // 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 zipEntries = m_zipFile.entries(); + root = new FileEntry( "" ); + Enumeration zipEntries = zip.entries(); while( zipEntries.hasMoreElements() ) { ZipEntry entry = zipEntries.nextElement(); - String entryName = entry.getName(); - if( entryName.startsWith( subPath ) ) - { - entryName = FileSystem.toLocal( entryName, subPath ); - if( m_root == null ) - { - 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 - } - } - } + + String entryPath = entry.getName(); + if( !entryPath.startsWith( subPath ) ) continue; + + String localPath = FileSystem.toLocal( entryPath, subPath ); + create( entry, localPath ); } } - // 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 file != null; + return get( path ) != null; } @Override public boolean isDirectory( @Nonnull String path ) { - FileInZip file = m_root.getFile( path ); - if( file != null ) - { - return file.isDirectory(); - } - return false; + FileEntry file = get( path ); + return file != null && file.isDirectory(); } @Override public void list( @Nonnull String path, @Nonnull List contents ) throws IOException { - FileInZip file = m_root.getFile( path ); - if( file != null && file.isDirectory() ) - { - file.list( contents ); - } - else - { - throw new IOException( "/" + path + ": Not a directory" ); - } + FileEntry file = get( path ); + if( file == null || !file.isDirectory() ) throw new IOException( "/" + path + ": Not a directory" ); + + file.list( contents ); } @Override public long getSize( @Nonnull String path ) throws IOException { - FileInZip file = m_root.getFile( path ); - if( file != null ) - { - return file.getSize(); - } + FileEntry file = get( path ); + if( file != null ) return file.size; 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; - if( path.length() > 0 ) - { - fullPath = fullPath + "/" + path; - } - ZipEntry entry = m_zipFile.getEntry( fullPath ); + ZipEntry entry = zip.getEntry( file.path ); 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 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 contents ) + { + if( children != null ) contents.addAll( children.keySet() ); + } + } + + private static class MountReference extends WeakReference + { + final ZipFile file; + + MountReference( JarMount file ) + { + super( file, MOUNT_QUEUE ); + this.file = file.zip; + } + } + + private static void cleanup() + { + Reference next; + while( (next = MOUNT_QUEUE.poll()) != null ) IoUtil.closeQuietly( ((MountReference) next).file ); + } } diff --git a/src/test/java/dan200/computercraft/core/filesystem/FilesystemMountTest.java b/src/test/java/dan200/computercraft/core/filesystem/FilesystemMountTest.java deleted file mode 100644 index 9028d4d21..000000000 --- a/src/test/java/dan200/computercraft/core/filesystem/FilesystemMountTest.java +++ /dev/null @@ -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( "" ) ); - } -} diff --git a/src/test/java/dan200/computercraft/core/filesystem/JarMountTest.java b/src/test/java/dan200/computercraft/core/filesystem/JarMountTest.java index 9af07adff..824a5b294 100644 --- a/src/test/java/dan200/computercraft/core/filesystem/JarMountTest.java +++ b/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.assertTrue; +import static org.junit.Assert.*; @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 ) ); + } }