Change SimpleFSDirectory to use newByteChannel

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1624868 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -9,6 +9,11 @@ New Features
 
 * LUCENE-5945: All file handling converted to NIO.2 apis. (Robert Muir)
 
+* LUCENE-5946: SimpleFSDirectory now uses Files.newByteChannel, for 
+  portability with custom FileSystemProviders. If you want the old
+  non-interruptible behavior of RandomAccessFile, use RAFDirectory
+  in the misc/ module. (Uwe Schindler, Robert Muir)
+
 * SOLR-3359: Added analyzer attribute/property to SynonymFilterFactory.
   (Ryo Onodera via Koji Sekiguchi)
 

lucene/core/build.xml

@@ -35,12 +35,6 @@
     org/apache/lucene/index/TestIndexWriterOnJRECrash.class
   "/>
 
-  <!-- TODO: maybe let people get closedchannel if they cancel(true) -->
-  <property name="forbidden-base-excludes" value="
-    org/apache/lucene/store/SimpleFSDirectory.class
-    org/apache/lucene/store/SimpleFSDirectory$SimpleFSIndexInput.class
-  "/>
-
   <import file="../common-build.xml"/>
 
   <property name="moman.commit-hash" value="5c5c2a1e4dea" />

lucene/core/src/java/org/apache/lucene/store/FSDirectory.java

@@ -46,7 +46,7 @@ import static java.util.Collections.synchronizedSet;
  * <ul>
  *
  *  <li> {@link SimpleFSDirectory} is a straightforward
- *       implementation using java.io.RandomAccessFile.
+ *       implementation using Files.newByteChannel.
  *       However, it has poor concurrent performance
  *       (multiple threads will bottleneck) as it
  *       synchronizes when multiple threads read from the
@@ -61,7 +61,7 @@ import static java.util.Collections.synchronizedSet;
  *       on all other platforms this is the preferred
  *       choice. Applications using {@link Thread#interrupt()} or
  *       {@link Future#cancel(boolean)} should use
- *       {@link SimpleFSDirectory} instead. See {@link NIOFSDirectory} java doc
+ *       {@code RAFDirectory} instead. See {@link NIOFSDirectory} java doc
  *       for details.
  *        
  *        

lucene/core/src/java/org/apache/lucene/store/NIOFSDirectory.java

@@ -47,7 +47,7 @@ import java.util.concurrent.Future; // javadoc
  * blocked on IO. The file descriptor will remain closed and subsequent access
  * to {@link NIOFSDirectory} will throw a {@link ClosedChannelException}. If
  * your application uses either {@link Thread#interrupt()} or
- * {@link Future#cancel(boolean)} you should use {@link SimpleFSDirectory} in
+ * {@link Future#cancel(boolean)} you should use {@code RAFDirectory} in
  * favor of {@link NIOFSDirectory}.</font>
  * </p>
  */

lucene/core/src/java/org/apache/lucene/store/SimpleFSDirectory.java

@@ -18,19 +18,20 @@ package org.apache.lucene.store;
  */
 
 import java.io.EOFException;
-import java.io.File;
 import java.io.IOException;
-import java.io.RandomAccessFile;
+import java.nio.ByteBuffer;
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.StandardOpenOption;
 
 /** A straightforward implementation of {@link FSDirectory}
- *  using java.io.RandomAccessFile.  However, this class has
+ *  using {@link Files#newByteChannel(Path, java.nio.file.OpenOption...)}.  
+ *  However, this class has
  *  poor concurrent performance (multiple threads will
  *  bottleneck) as it synchronizes when multiple threads
  *  read from the same file.  It's usually better to use
  *  {@link NIOFSDirectory} or {@link MMapDirectory} instead. */
-// TODO: we currently mandate .toFile to still use RandomAccessFile, to avoid ClosedByInterruptException.
-// should we change to SeekableByteChannel instead?
 public class SimpleFSDirectory extends FSDirectory {
     
   /** Create a new SimpleFSDirectory for the named location.
@@ -42,7 +43,6 @@ public class SimpleFSDirectory extends FSDirectory {
    */
   public SimpleFSDirectory(Path path, LockFactory lockFactory) throws IOException {
     super(path, lockFactory);
-    path.toFile(); // throw exception if we can't get a File for now
   }
   
   /** Create a new SimpleFSDirectory for the named location and {@link NativeFSLockFactory}.
@@ -52,31 +52,28 @@ public class SimpleFSDirectory extends FSDirectory {
    */
   public SimpleFSDirectory(Path path) throws IOException {
     super(path, null);
-    path.toFile(); // throw exception if we can't get a File for now
   }
 
   /** Creates an IndexInput for the file with the given name. */
   @Override
   public IndexInput openInput(String name, IOContext context) throws IOException {
     ensureOpen();
-    final File path = directory.resolve(name).toFile();
-    RandomAccessFile raf = new RandomAccessFile(path, "r");
-    return new SimpleFSIndexInput("SimpleFSIndexInput(path=\"" + path.getPath() + "\")", raf, context);
+    Path path = directory.resolve(name);
+    SeekableByteChannel channel = Files.newByteChannel(path, StandardOpenOption.READ);
+    return new SimpleFSIndexInput("SimpleFSIndexInput(path=\"" + path + "\")", channel, context);
   }
 
   /**
-   * Reads bytes with {@link RandomAccessFile#seek(long)} followed by
-   * {@link RandomAccessFile#read(byte[], int, int)}.  
+   * Reads bytes with {@link SeekableByteChannel#read(ByteBuffer)}
    */
   static final class SimpleFSIndexInput extends BufferedIndexInput {
     /**
-     * The maximum chunk size is 8192 bytes, because {@link RandomAccessFile} mallocs
-     * a native buffer outside of stack if the read buffer size is larger.
+     * The maximum chunk size for reads of 16384 bytes.
      */
-    private static final int CHUNK_SIZE = 8192;
+    private static final int CHUNK_SIZE = 16384;
     
-    /** the file channel we will read from */
-    protected final RandomAccessFile file;
+    /** the channel we will read from */
+    protected final SeekableByteChannel channel;
     /** is this instance a clone and hence does not own the file to close it */
     boolean isClone = false;
     /** start offset: non-zero in the slice case */
@@ -84,16 +81,18 @@ public class SimpleFSDirectory extends FSDirectory {
     /** end offset (start+length) */
     protected final long end;
     
-    public SimpleFSIndexInput(String resourceDesc, RandomAccessFile file, IOContext context) throws IOException {
+    private ByteBuffer byteBuf; // wraps the buffer for NIO
+
+    public SimpleFSIndexInput(String resourceDesc, SeekableByteChannel channel, IOContext context) throws IOException {
       super(resourceDesc, context);
-      this.file = file; 
+      this.channel = channel; 
       this.off = 0L;
-      this.end = file.length();
+      this.end = channel.size();
     }
     
-    public SimpleFSIndexInput(String resourceDesc, RandomAccessFile file, long off, long length, int bufferSize) {
+    public SimpleFSIndexInput(String resourceDesc, SeekableByteChannel channel, long off, long length, int bufferSize) {
       super(resourceDesc, bufferSize);
-      this.file = file;
+      this.channel = channel;
       this.off = off;
       this.end = off + length;
       this.isClone = true;
@@ -102,7 +101,7 @@ public class SimpleFSDirectory extends FSDirectory {
     @Override
     public void close() throws IOException {
       if (!isClone) {
-        file.close();
+        channel.close();
       }
     }
     
@@ -118,50 +117,65 @@ public class SimpleFSDirectory extends FSDirectory {
       if (offset < 0 || length < 0 || offset + length > this.length()) {
         throw new IllegalArgumentException("slice() " + sliceDescription + " out of bounds: "  + this);
       }
-      return new SimpleFSIndexInput(sliceDescription, file, off + offset, length, getBufferSize());
+      return new SimpleFSIndexInput(sliceDescription, channel, off + offset, length, getBufferSize());
     }
 
     @Override
     public final long length() {
       return end - off;
     }
-  
-    /** IndexInput methods */
-    @Override
-    protected void readInternal(byte[] b, int offset, int len)
-         throws IOException {
-      synchronized (file) {
-        long position = off + getFilePointer();
-        file.seek(position);
-        int total = 0;
 
-        if (position + len > end) {
+    @Override
+    protected void newBuffer(byte[] newBuffer) {
+      super.newBuffer(newBuffer);
+      byteBuf = ByteBuffer.wrap(newBuffer);
+    }
+
+    @Override
+    protected void readInternal(byte[] b, int offset, int len) throws IOException {
+      final ByteBuffer bb;
+
+      // Determine the ByteBuffer we should use
+      if (b == buffer) {
+        // Use our own pre-wrapped byteBuf:
+        assert byteBuf != null;
+        bb = byteBuf;
+        byteBuf.clear().position(offset);
+      } else {
+        bb = ByteBuffer.wrap(b, offset, len);
+      }
+
+      synchronized(channel) {
+        long pos = getFilePointer() + off;
+        
+        if (pos + len > end) {
           throw new EOFException("read past EOF: " + this);
         }
-
+               
         try {
-          while (total < len) {
-            final int toRead = Math.min(CHUNK_SIZE, len - total);
-            final int i = file.read(b, offset + total, toRead);
+          channel.position(pos);
+
+          int readLength = len;
+          while (readLength > 0) {
+            final int toRead = Math.min(CHUNK_SIZE, readLength);
+            bb.limit(bb.position() + toRead);
+            assert bb.remaining() == toRead;
+            final int i = channel.read(bb);
             if (i < 0) { // be defensive here, even though we checked before hand, something could have changed
-             throw new EOFException("read past EOF: " + this + " off: " + offset + " len: " + len + " total: " + total + " chunkLen: " + toRead + " end: " + end);
+              throw new EOFException("read past EOF: " + this + " off: " + offset + " len: " + len + " pos: " + pos + " chunkLen: " + toRead + " end: " + end);
             }
-            assert i > 0 : "RandomAccessFile.read with non zero-length toRead must always read at least one byte";
-            total += i;
+            assert i > 0 : "SeekableByteChannel.read with non zero-length bb.remaining() must always read at least one byte (Channel is in blocking mode, see spec of ReadableByteChannel)";
+            pos += i;
+            readLength -= i;
           }
-          assert total == len;
+          assert readLength == 0;
         } catch (IOException ioe) {
           throw new IOException(ioe.getMessage() + ": " + this, ioe);
         }
       }
     }
-  
+
     @Override
-    protected void seekInternal(long position) {
-    }
-    
-    boolean isFDValid() throws IOException {
-      return file.getFD().valid();
-    }
+    protected void seekInternal(long pos) throws IOException {}
   }
 }

lucene/core/src/test/org/apache/lucene/index/TestCompoundFile.java

@@ -33,9 +33,6 @@ import org.apache.lucene.util.TestUtil;
 import java.io.IOException;
 import java.nio.file.Path;
 
-import static org.apache.lucene.store.TestHelper.isSimpleFSIndexInput;
-import static org.apache.lucene.store.TestHelper.isSimpleFSIndexInputOpen;
-
 public class TestCompoundFile extends LuceneTestCase
 {
     private Directory dir;
@@ -345,10 +342,6 @@ public class TestCompoundFile extends LuceneTestCase
         // basic clone
         IndexInput expected = dir.openInput("f11", newIOContext(random()));
 
-        // this test only works for FSIndexInput
-        assertTrue(isSimpleFSIndexInput(expected));
-        assertTrue(isSimpleFSIndexInputOpen(expected));
-
         IndexInput one = cr.openInput("f11", newIOContext(random()));
 
         IndexInput two = one.clone();

lucene/misc/build.xml

@@ -26,6 +26,9 @@
   <property name="forbidden-base-excludes" value="
     org/apache/lucene/store/NativeUnixDirectory$NativeUnixIndexInput.class
     org/apache/lucene/store/NativeUnixDirectory$NativeUnixIndexOutput.class
+    org/apache/lucene/store/RAFDirectory.class
+    org/apache/lucene/store/RAFDirectory$RAFIndexInput.class
+    org/apache/lucene/store/RAFDirectory$RAFIndexOutput.class
   "/>
 
   <property name="forbidden-sysout-excludes" value="

lucene/misc/src/java/org/apache/lucene/store/RAFDirectory.java

@@ -0,0 +1,171 @@
+package org.apache.lucene.store;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.io.EOFException;
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.file.Path;
+
+/** A straightforward implementation of {@link FSDirectory}
+ *  using java.io.RandomAccessFile.  However, this class has
+ *  poor concurrent performance (multiple threads will
+ *  bottleneck) as it synchronizes when multiple threads
+ *  read from the same file.  It's usually better to use
+ *  {@link NIOFSDirectory} or {@link MMapDirectory} instead. 
+ *  <p>
+ *  NOTE: Because this uses RandomAccessFile, it will generally
+ *  not work with non-default filesystem providers. It is only
+ *  provided for applications that relied on the fact that 
+ *  RandomAccessFile's IO was not interruptible.
+ */
+public class RAFDirectory extends FSDirectory {
+    
+  /** Create a new RAFDirectory for the named location.
+   *
+   * @param path the path of the directory
+   * @param lockFactory the lock factory to use, or null for the default
+   * ({@link NativeFSLockFactory});
+   * @throws IOException if there is a low-level I/O error
+   */
+  public RAFDirectory(Path path, LockFactory lockFactory) throws IOException {
+    super(path, lockFactory);
+    path.toFile(); // throw exception if we can't get a File
+  }
+  
+  /** Create a new SimpleFSDirectory for the named location and {@link NativeFSLockFactory}.
+   *
+   * @param path the path of the directory
+   * @throws IOException if there is a low-level I/O error
+   */
+  public RAFDirectory(Path path) throws IOException {
+    super(path, null);
+    path.toFile(); // throw exception if we can't get a File
+  }
+
+  /** Creates an IndexInput for the file with the given name. */
+  @Override
+  public IndexInput openInput(String name, IOContext context) throws IOException {
+    ensureOpen();
+    final File path = directory.resolve(name).toFile();
+    RandomAccessFile raf = new RandomAccessFile(path, "r");
+    return new RAFIndexInput("SimpleFSIndexInput(path=\"" + path.getPath() + "\")", raf, context);
+  }
+
+  /**
+   * Reads bytes with {@link RandomAccessFile#seek(long)} followed by
+   * {@link RandomAccessFile#read(byte[], int, int)}.  
+   */
+  static final class RAFIndexInput extends BufferedIndexInput {
+    /**
+     * The maximum chunk size is 8192 bytes, because {@link RandomAccessFile} mallocs
+     * a native buffer outside of stack if the read buffer size is larger.
+     */
+    private static final int CHUNK_SIZE = 8192;
+    
+    /** the file channel we will read from */
+    protected final RandomAccessFile file;
+    /** is this instance a clone and hence does not own the file to close it */
+    boolean isClone = false;
+    /** start offset: non-zero in the slice case */
+    protected final long off;
+    /** end offset (start+length) */
+    protected final long end;
+    
+    public RAFIndexInput(String resourceDesc, RandomAccessFile file, IOContext context) throws IOException {
+      super(resourceDesc, context);
+      this.file = file; 
+      this.off = 0L;
+      this.end = file.length();
+    }
+    
+    public RAFIndexInput(String resourceDesc, RandomAccessFile file, long off, long length, int bufferSize) {
+      super(resourceDesc, bufferSize);
+      this.file = file;
+      this.off = off;
+      this.end = off + length;
+      this.isClone = true;
+    }
+    
+    @Override
+    public void close() throws IOException {
+      if (!isClone) {
+        file.close();
+      }
+    }
+    
+    @Override
+    public RAFIndexInput clone() {
+      RAFIndexInput clone = (RAFIndexInput)super.clone();
+      clone.isClone = true;
+      return clone;
+    }
+    
+    @Override
+    public IndexInput slice(String sliceDescription, long offset, long length) throws IOException {
+      if (offset < 0 || length < 0 || offset + length > this.length()) {
+        throw new IllegalArgumentException("slice() " + sliceDescription + " out of bounds: "  + this);
+      }
+      return new RAFIndexInput(sliceDescription, file, off + offset, length, getBufferSize());
+    }
+
+    @Override
+    public final long length() {
+      return end - off;
+    }
+  
+    /** IndexInput methods */
+    @Override
+    protected void readInternal(byte[] b, int offset, int len)
+         throws IOException {
+      synchronized (file) {
+        long position = off + getFilePointer();
+        file.seek(position);
+        int total = 0;
+
+        if (position + len > end) {
+          throw new EOFException("read past EOF: " + this);
+        }
+
+        try {
+          while (total < len) {
+            final int toRead = Math.min(CHUNK_SIZE, len - total);
+            final int i = file.read(b, offset + total, toRead);
+            if (i < 0) { // be defensive here, even though we checked before hand, something could have changed
+             throw new EOFException("read past EOF: " + this + " off: " + offset + " len: " + len + " total: " + total + " chunkLen: " + toRead + " end: " + end);
+            }
+            assert i > 0 : "RandomAccessFile.read with non zero-length toRead must always read at least one byte";
+            total += i;
+          }
+          assert total == len;
+        } catch (IOException ioe) {
+          throw new IOException(ioe.getMessage() + ": " + this, ioe);
+        }
+      }
+    }
+  
+    @Override
+    protected void seekInternal(long position) {
+    }
+    
+    boolean isFDValid() throws IOException {
+      return file.getFD().valid();
+    }
+  }
+}

lucene/misc/src/test/org/apache/lucene/store/TestRAFDirectory.java

@@ -0,0 +1,32 @@
+package org.apache.lucene.store;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.io.IOException;
+import java.nio.file.Path;
+
+/**
+ * Tests RAFDirectory
+ */
+public class TestRAFDirectory extends BaseDirectoryTestCase {
+
+  @Override
+  protected Directory getDirectory(Path path) throws IOException {
+    return new RAFDirectory(path);
+  }
+}

lucene/test-framework/src/java/org/apache/lucene/store/TestHelper.java

@@ -1,68 +0,0 @@
-package org.apache.lucene.store;
-
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements.  See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License.  You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-import java.io.IOException;
-
-import org.apache.lucene.store.SimpleFSDirectory.SimpleFSIndexInput;
-
-/** This class provides access to package-level features defined in the
- *  store package. It is used for testing only.
- */
-public final class TestHelper {
-  private TestHelper() {
-    //
-  }
-
-    /** Returns true if the instance of the provided input stream is actually
-     *  an SimpleFSIndexInput.
-     */
-    public static boolean isSimpleFSIndexInput(IndexInput is) {
-        return is instanceof SimpleFSIndexInput;
-    }
-
-    /** Returns true if the provided input stream is an SimpleFSIndexInput and
-     *  is a clone, that is it does not own its underlying file descriptor.
-     */
-    public static boolean isSimpleFSIndexInputClone(IndexInput is) {
-        if (isSimpleFSIndexInput(is)) {
-            return ((SimpleFSIndexInput) is).isClone;
-        } else {
-            return false;
-        }
-    }
-
-    /** Given an instance of SimpleFSDirectory.SimpleFSIndexInput, this method returns
-     *  true if the underlying file descriptor is valid, and false otherwise.
-     *  This can be used to determine if the OS file has been closed.
-     *  The descriptor becomes invalid when the non-clone instance of the
-     *  SimpleFSIndexInput that owns this descriptor is closed. However, the
-     *  descriptor may possibly become invalid in other ways as well.
-     */
-    public static boolean isSimpleFSIndexInputOpen(IndexInput is)
-    throws IOException
-    {
-        if (isSimpleFSIndexInput(is)) {
-            SimpleFSIndexInput fis = (SimpleFSIndexInput) is;
-            return fis.isFDValid();
-        } else {
-            return false;
-        }
-    }
-
-}