#438 CompoundDocument file descriptor fix

common/common-io/src/main/java/com/twelvemonkeys/io/AbstractCachedSeekableStream.java

@@ -186,7 +186,7 @@ abstract class AbstractCachedSeekableStream extends SeekableInputStream {
     }
 
     protected void closeImpl() throws IOException {
-        cache.flush(position);
+        cache.close();
         cache = null;
         stream.close();
     }
@@ -198,12 +198,12 @@ abstract class AbstractCachedSeekableStream extends SeekableInputStream {
      * @author last modified by $Author: haku $
      * @version $Id: //depot/branches/personal/haraldk/twelvemonkeys/release-2/twelvemonkeys-core/src/main/java/com/twelvemonkeys/io/AbstractCachedSeekableStream.java#2 $
      */
-    public static abstract class StreamCache {
+    static abstract class StreamCache {
 
         /**
          * Creates a {@code StreamCache}.
          */
-        protected StreamCache() {
+        StreamCache() {
         }
 
         /**
@@ -303,5 +303,7 @@ abstract class AbstractCachedSeekableStream extends SeekableInputStream {
          * @throws IOException if the position can't be determined because of a problem in the cache backing mechanism.
          */
         abstract long getPosition() throws IOException;
+
+        abstract void close() throws IOException;
     }
 }

common/common-io/src/main/java/com/twelvemonkeys/io/FileCacheSeekableStream.java

@@ -124,7 +124,9 @@ public final class FileCacheSeekableStream extends AbstractCachedSeekableStream
 
     @Override
     protected void closeImpl() throws IOException {
+        // TODO: Close cache file
         super.closeImpl();
+
         buffer = null;
     }
 
@@ -194,39 +196,45 @@ public final class FileCacheSeekableStream extends AbstractCachedSeekableStream
         return length;
     }
 
+    // TODO: We need to close the cache file!!! Otherwise we are leaking file descriptors
+
     final static class FileCache extends StreamCache {
-        private RandomAccessFile mCacheFile;
+        private RandomAccessFile cacheFile;
 
         public FileCache(final File pFile) throws FileNotFoundException {
             Validate.notNull(pFile, "file");
-            mCacheFile = new RandomAccessFile(pFile, "rw");
+            cacheFile = new RandomAccessFile(pFile, "rw");
         }
 
         public void write(final int pByte) throws IOException {
-            mCacheFile.write(pByte);
+            cacheFile.write(pByte);
         }
 
         @Override
         public void write(final byte[] pBuffer, final int pOffset, final int pLength) throws IOException {
-            mCacheFile.write(pBuffer, pOffset, pLength);
+            cacheFile.write(pBuffer, pOffset, pLength);
         }
 
         public int read() throws IOException {
-            return mCacheFile.read();
+            return cacheFile.read();
         }
 
         @Override
         public int read(final byte[] pBuffer, final int pOffset, final int pLength) throws IOException {
-            return mCacheFile.read(pBuffer, pOffset, pLength);
+            return cacheFile.read(pBuffer, pOffset, pLength);
         }
 
         public void seek(final long pPosition) throws IOException {
-            mCacheFile.seek(pPosition);
+            cacheFile.seek(pPosition);
         }
 
         public long getPosition() throws IOException {
-            return mCacheFile.getFilePointer();
+            return cacheFile.getFilePointer();
-        }
         }
 
+        @Override
+        void close() throws IOException {
+            cacheFile.close();
+        }
+    }
 }

common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataInputStream.java

@@ -53,7 +53,6 @@ import java.io.*;
  * which this class imitates reads big endian quantities.
  * <p/>
  * <em>Warning:
- * <!-- Beware of little indians! -->
  * The {@code DataInput} and {@code DataOutput} interfaces
  * specifies big endian byte order in their documentation.
  * This means that this class is, strictly speaking, not a proper

common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataOutputStream.java

@@ -52,7 +52,6 @@ import java.io.*;
  * imitates uses big endian integers.
  * <p/>
  * <em>Warning:
- * <!-- Beware of little indians! -->
  * The {@code DataInput} and {@code DataOutput} interfaces
  * specifies big endian byte order in their documentation.
  * This means that this class is, strictly speaking, not a proper

common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianRandomAccessFile.java

@@ -38,7 +38,6 @@ import java.nio.channels.FileChannel;
  * and writing data in little endian byte order.
  * <p/>
  * <em>Warning:
- * <!-- Beware of little indians! -->
  * The {@code DataInput} and {@code DataOutput} interfaces
  * specifies big endian byte order in their documentation.
  * This means that this class is, strictly speaking, not a proper

common/common-io/src/main/java/com/twelvemonkeys/io/MemoryCacheSeekableStream.java

@@ -67,7 +67,7 @@ public final class MemoryCacheSeekableStream extends AbstractCachedSeekableStrea
     final static class MemoryCache extends StreamCache {
         final static int BLOCK_SIZE = 1 << 13;
 
-        private final List<byte[]> cache = new ArrayList<byte[]>();
+        private final List<byte[]> cache = new ArrayList<>();
         private long length;
         private long position;
         private long start;
@@ -186,6 +186,11 @@ public final class MemoryCacheSeekableStream extends AbstractCachedSeekableStrea
             start = pPosition;
         }
 
+        @Override
+        void close() throws IOException {
+            cache.clear();
+        }
+
         public long getPosition() {
             return position;
         }

common/common-io/src/main/java/com/twelvemonkeys/io/ole2/CompoundDocument.java

@@ -30,11 +30,14 @@
 
 package com.twelvemonkeys.io.ole2;
 
+import static com.twelvemonkeys.lang.Validate.notNull;
+
 import com.twelvemonkeys.io.*;
 import com.twelvemonkeys.lang.StringUtil;
 
 import javax.imageio.stream.ImageInputStream;
 import java.io.*;
+import java.nio.ByteOrder;
 import java.util.Arrays;
 import java.util.SortedSet;
 import java.util.TreeSet;
@@ -52,7 +55,7 @@ import java.util.UUID;
  * @author last modified by $Author: haku $
  * @version $Id: //depot/branches/personal/haraldk/twelvemonkeys/release-2/twelvemonkeys-core/src/main/java/com/twelvemonkeys/io/ole2/CompoundDocument.java#4 $
  */
-public final class CompoundDocument {
+public final class CompoundDocument implements AutoCloseable {
     // TODO: Write support...
     // TODO: Properties: http://support.microsoft.com/kb/186898
     
@@ -96,13 +99,18 @@ public final class CompoundDocument {
 
     /**
      * Creates a (for now) read only {@code CompoundDocument}.
+     * <p/>
+     * <em>Warning! You must invoke {@link #close()} on the compound document
+     * created from this constructor when done, to avoid leaking file
+     * descriptors.</em>
      *
-     * @param pFile the file to read from
+     * @param file the file to read from
      *
      * @throws IOException if an I/O exception occurs while reading the header
      */
-    public CompoundDocument(final File pFile) throws IOException {
+    public CompoundDocument(final File file) throws IOException {
-        input = new LittleEndianRandomAccessFile(FileUtil.resolve(pFile), "r");
+        // TODO: We need to close this (or it's underlying RAF)! Otherwise we're leaking file descriptors!
+        input = new LittleEndianRandomAccessFile(FileUtil.resolve(file), "r");
 
         // TODO: Might be better to read header on first read operation?!
         // OTOH: It's also good to be fail-fast, so at least we should make
@@ -113,17 +121,17 @@ public final class CompoundDocument {
     /**
      * Creates a read only {@code CompoundDocument}.
      *
-     * @param pInput the input to read from
+     * @param pInput the input to read from.
      *
      * @throws IOException if an I/O exception occurs while reading the header
      */
     public CompoundDocument(final InputStream pInput) throws IOException {
-        this(new FileCacheSeekableStream(pInput));
+        this(new MemoryCacheSeekableStream(pInput));
     }
 
     // For testing only, consider exposing later
-    CompoundDocument(final SeekableInputStream pInput) throws IOException {
+    CompoundDocument(final SeekableInputStream stream) throws IOException {
-        input = new SeekableLittleEndianDataInputStream(pInput);
+        input = new SeekableLittleEndianDataInputStream(stream);
 
         // TODO: Might be better to read header on first read operation?!
         // OTOH: It's also good to be fail-fast, so at least we should make
@@ -134,12 +142,15 @@ public final class CompoundDocument {
     /**
      * Creates a read only {@code CompoundDocument}.
      *
-     * @param pInput the input to read from
+     * @param input the input to read from
      *
      * @throws IOException if an I/O exception occurs while reading the header
      */
-    public CompoundDocument(final ImageInputStream pInput) throws IOException {
+    public CompoundDocument(final ImageInputStream input) throws IOException {
-        input = pInput;
+        this.input = notNull(input, "input");
+
+        // This implementation only supports little endian (Intel) CompoundDocuments
+        input.setByteOrder(ByteOrder.LITTLE_ENDIAN);
 
         // TODO: Might be better to read header on first read operation?!
         // OTOH: It's also good to be fail-fast, so at least we should make
@@ -147,6 +158,27 @@ public final class CompoundDocument {
         readHeader();
     }
 
+    /**
+     * This method will close the underlying {@link RandomAccessFile} if any,
+     * but will leave any stream created outside the document open.
+     *
+     * @see #CompoundDocument(File)
+     * @see RandomAccessFile#close()
+     *
+     * @throws IOException if an I/O error occurs.
+     */
+    @Override
+    public void close() throws IOException {
+        if (input instanceof RandomAccessFile) {
+            ((RandomAccessFile) input).close();
+        }
+        else if (input instanceof LittleEndianRandomAccessFile) {
+            ((LittleEndianRandomAccessFile) input).close();
+        }
+
+        // Other streams are left open
+    }
+
     public static boolean canRead(final DataInput pInput) {
         return canRead(pInput, true);
     }

common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocumentTest.java

@@ -72,8 +72,7 @@ public class CompoundDocumentTest {
 
     @Test
     public void testRoot() throws IOException {
-        CompoundDocument document = createTestDocument();
+        try (CompoundDocument document = createTestDocument()) {
-
             Entry root = document.getRootEntry();
 
             assertNotNull(root);
@@ -84,11 +83,11 @@ public class CompoundDocumentTest {
             assertEquals(0, root.length());
             assertNull(root.getInputStream());
         }
+    }
 
     @Test
     public void testContents() throws IOException {
-        CompoundDocument document = createTestDocument();
+        try (CompoundDocument document = createTestDocument()) {
-
             Entry root = document.getRootEntry();
 
             assertNotNull(root);
@@ -102,11 +101,11 @@ public class CompoundDocumentTest {
                 children.remove(children.first());
             }
         }
+    }
 
     @Test(expected = UnsupportedOperationException.class)
     public void testChildEntriesUnmodifiable() throws IOException {
-        CompoundDocument document = createTestDocument();
+        try (CompoundDocument document = createTestDocument()) {
-
             Entry root = document.getRootEntry();
 
             assertNotNull(root);
@@ -116,11 +115,11 @@ public class CompoundDocumentTest {
             // Should not be allowed, as it modifies the internal structure
             children.remove(children.first());
         }
+    }
 
     @Test
     public void testReadThumbsCatalogFile() throws IOException {
-        CompoundDocument document = createTestDocument();
+        try (CompoundDocument document = createTestDocument()) {
-
             Entry root = document.getRootEntry();
 
             assertNotNull(root);
@@ -131,6 +130,7 @@ public class CompoundDocumentTest {
             assertNotNull(catalog);
             assertNotNull("Input stream may not be null", catalog.getInputStream());
         }
+    }
 
     @Test
     public void testReadCatalogInputStream() throws IOException {

common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocument_StreamTest.java

@@ -51,35 +51,7 @@ import static org.junit.Assert.*;
  * @author last modified by $Author: haraldk$
  * @version $Id: CompoundDocument_StreamTestCase.java,v 1.0 13.10.11 12:01 haraldk Exp$
  */
-//@Ignore("Need proper in-memory creation of CompoundDocuments")
 public class CompoundDocument_StreamTest extends InputStreamAbstractTest {
-    private static final String SAMPLE_DATA = "/Thumbs-camera.db";
-
-    protected final CompoundDocument createTestDocument() throws IOException {
-        URL input = getClass().getResource(SAMPLE_DATA);
-
-        assertNotNull("Missing test resource!", input);
-        assertEquals("Test resource not a file:// resource", "file", input.getProtocol());
-
-        try {
-            return new CompoundDocument(new File(input.toURI()));
-        }
-        catch (URISyntaxException e) {
-            throw new AssertionError(e);
-        }
-    }
-
-    private SeekableInputStream createRealInputStream() {
-        try {
-            Entry first = createTestDocument().getRootEntry().getChildEntries().first();
-            assertNotNull(first);
-            return first.getInputStream();
-        }
-        catch (IOException e) {
-            throw new AssertionError(e);
-        }
-    }
-
     @Override
     protected InputStream makeInputStream(byte[] data) {
         try {
@@ -182,15 +154,13 @@ public class CompoundDocument_StreamTest extends InputStreamAbstractTest {
         return pad;
     }
 
-//    @Ignore
     @Test
-    public void testDev() throws IOException {
+    public void testStreamRead() throws IOException {
         InputStream stream = makeInputStream(makeOrderedArray(32));
 
         int read;
         int count = 0;
         while ((read = stream.read()) >= 0) {
-//            System.out.printf("read %02d: 0x%02x%n", count, read & 0xFF);
             assertEquals(count, read);
             count++;
         }