Repository: netty/netty
Issue: Avoid creating FileInputStream and FileOutputStream for obtaining FileChannel
Labels: improvement

There are multiple places in the production code of the project, e. g. in `ChunkedNioFile`, where `FileInputStream` or `FileOutputStream` are created solely to obtain a `FileChannel` from them. Until Netty projects bumps the compatibility level to Java 7 (where `FileChannel.open()` methods were added), it's better to obtain a FileChannel by means of creating a `RandomAccessFile`, because the latter doesn't override `Object.finalize()`. Also, I would extract those pieces of code into a utility methods like `openChannelForRead(File)` and `openChannelForWrite(File)`.

Issue comments:
Same goes for BufferedReader instances - same finalize problem. It's used in DNS and some other modules.
@leventov good catch... Check https://github.com/netty/netty/pull/8110

Pull request review comments:
This looks like it leaks, since you need to close `RandomAccessFile`. The documentation for `getChannel()` does not say closing the channel closes the `RandomAccessFile`. The documentation for `RandomAccessFile.close()` notes the opposite, that closing the file closes the channel. Looking at the source, it seems the channel holds an extra ref, so closing it does not close the `fd` held by `RandomAccessFile`.
@ejona86 maybe I am missing something but we call `output.close()` in the finally block so it should be fine ?
At risk of repeating myself: `output` is the _channel_. But you need to close the `RandomAccessFile`. It is subtle.
@ejona86 sorry I missed this... This is not correct, closing the `FileChannel` here will also close the `RandomAccessFile` from which it was created. See also http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/4a1e42601d61/src/share/classes/sun/nio/ch/FileChannelImpl.java#l138
Hmm... the decrement for the ref is also in RandomAccessFile, so the channel _must_ close the file. Okay. Very well. I continue to be amazed at how poor some documentation is to define resource management...

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
index 544bc7c82da..94309186b41 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
@@ -22,10 +22,9 @@
 import io.netty.util.internal.logging.InternalLoggerFactory;
 
 import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.RandomAccessFile;
 import java.nio.ByteBuffer;
 import java.nio.channels.FileChannel;
 import java.nio.charset.Charset;
@@ -125,9 +124,10 @@ public void setContent(ByteBuf buffer) throws IOException {
                 }
                 return;
             }
-            FileOutputStream outputStream = new FileOutputStream(file);
+            RandomAccessFile accessFile = new RandomAccessFile(file, "rw");
+            accessFile.setLength(0);
             try {
-                FileChannel localfileChannel = outputStream.getChannel();
+                FileChannel localfileChannel = accessFile.getChannel();
                 ByteBuffer byteBuffer = buffer.nioBuffer();
                 int written = 0;
                 while (written < size) {
@@ -136,7 +136,7 @@ public void setContent(ByteBuf buffer) throws IOException {
                 buffer.readerIndex(buffer.readerIndex() + written);
                 localfileChannel.force(false);
             } finally {
-                outputStream.close();
+                accessFile.close();
             }
             setCompleted();
         } finally {
@@ -163,8 +163,8 @@ public void addContent(ByteBuf buffer, boolean last)
                     file = tempFile();
                 }
                 if (fileChannel == null) {
-                    FileOutputStream outputStream = new FileOutputStream(file);
-                    fileChannel = outputStream.getChannel();
+                    RandomAccessFile accessFile = new RandomAccessFile(file, "rw");
+                    fileChannel = accessFile.getChannel();
                 }
                 while (written < localsize) {
                     written += fileChannel.write(byteBuffer);
@@ -182,8 +182,8 @@ public void addContent(ByteBuf buffer, boolean last)
                 file = tempFile();
             }
             if (fileChannel == null) {
-                FileOutputStream outputStream = new FileOutputStream(file);
-                fileChannel = outputStream.getChannel();
+                RandomAccessFile accessFile = new RandomAccessFile(file, "rw");
+                fileChannel = accessFile.getChannel();
             }
             fileChannel.force(false);
             fileChannel.close();
@@ -217,10 +217,11 @@ public void setContent(InputStream inputStream) throws IOException {
             delete();
         }
         file = tempFile();
-        FileOutputStream outputStream = new FileOutputStream(file);
+        RandomAccessFile accessFile = new RandomAccessFile(file, "rw");
+        accessFile.setLength(0);
         int written = 0;
         try {
-            FileChannel localfileChannel = outputStream.getChannel();
+            FileChannel localfileChannel = accessFile.getChannel();
             byte[] bytes = new byte[4096 * 4];
             ByteBuffer byteBuffer = ByteBuffer.wrap(bytes);
             int read = inputStream.read(bytes);
@@ -232,7 +233,7 @@ public void setContent(InputStream inputStream) throws IOException {
             }
             localfileChannel.force(false);
         } finally {
-            outputStream.close();
+            accessFile.close();
         }
         size = written;
         if (definedSize > 0 && definedSize < size) {
@@ -290,8 +291,8 @@ public ByteBuf getChunk(int length) throws IOException {
             return EMPTY_BUFFER;
         }
         if (fileChannel == null) {
-            FileInputStream inputStream = new FileInputStream(file);
-            fileChannel = inputStream.getChannel();
+            RandomAccessFile accessFile = new RandomAccessFile(file, "r");
+            fileChannel = accessFile.getChannel();
         }
         int read = 0;
         ByteBuffer byteBuffer = ByteBuffer.allocate(length);
@@ -349,15 +350,15 @@ public boolean renameTo(File dest) throws IOException {
         if (!file.renameTo(dest)) {
             // must copy
             IOException exception = null;
-            FileInputStream inputStream = null;
-            FileOutputStream outputStream = null;
+            RandomAccessFile inputAccessFile = null;
+            RandomAccessFile outputAccessFile = null;
             long chunkSize = 8196;
             long position = 0;
             try {
-                inputStream = new FileInputStream(file);
-                outputStream = new FileOutputStream(dest);
-                FileChannel in = inputStream.getChannel();
-                FileChannel out = outputStream.getChannel();
+                inputAccessFile = new RandomAccessFile(file, "r");
+                outputAccessFile = new RandomAccessFile(dest, "rw");
+                FileChannel in = inputAccessFile.getChannel();
+                FileChannel out = outputAccessFile.getChannel();
                 while (position < size) {
                     if (chunkSize < size - position) {
                         chunkSize = size - position;
@@ -367,9 +368,9 @@ public boolean renameTo(File dest) throws IOException {
             } catch (IOException e) {
                 exception = e;
             } finally {
-                if (inputStream != null) {
+                if (inputAccessFile != null) {
                     try {
-                        inputStream.close();
+                        inputAccessFile.close();
                     } catch (IOException e) {
                         if (exception == null) { // Choose to report the first exception
                             exception = e;
@@ -378,9 +379,9 @@ public boolean renameTo(File dest) throws IOException {
                         }
                     }
                 }
-                if (outputStream != null) {
+                if (outputAccessFile != null) {
                     try {
-                        outputStream.close();
+                        outputAccessFile.close();
                     } catch (IOException e) {
                         if (exception == null) { // Choose to report the first exception
                             exception = e;
@@ -422,17 +423,17 @@ private static byte[] readFrom(File src) throws IOException {
             throw new IllegalArgumentException(
                     "File too big to be loaded in memory");
         }
-        FileInputStream inputStream = new FileInputStream(src);
+        RandomAccessFile accessFile = new RandomAccessFile(src, "r");
         byte[] array = new byte[(int) srcsize];
         try {
-            FileChannel fileChannel = inputStream.getChannel();
+            FileChannel fileChannel = accessFile.getChannel();
             ByteBuffer byteBuffer = ByteBuffer.wrap(array);
             int read = 0;
             while (read < srcsize) {
                 read += fileChannel.read(byteBuffer);
             }
         } finally {
-            inputStream.close();
+            accessFile.close();
         }
         return array;
     }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
index 4cb7e567b25..5a0f322d4dc 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
@@ -20,10 +20,9 @@
 import io.netty.handler.codec.http.HttpConstants;
 
 import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.RandomAccessFile;
 import java.nio.ByteBuffer;
 import java.nio.channels.FileChannel;
 import java.nio.charset.Charset;
@@ -131,8 +130,8 @@ public void setContent(File file) throws IOException {
             throw new IllegalArgumentException("File too big to be loaded in memory");
         }
         checkSize(newsize);
-        FileInputStream inputStream = new FileInputStream(file);
-        FileChannel fileChannel = inputStream.getChannel();
+        RandomAccessFile accessFile = new RandomAccessFile(file, "r");
+        FileChannel fileChannel = accessFile.getChannel();
         byte[] array = new byte[(int) newsize];
         ByteBuffer byteBuffer = ByteBuffer.wrap(array);
         int read = 0;
@@ -140,7 +139,7 @@ public void setContent(File file) throws IOException {
             read += fileChannel.read(byteBuffer);
         }
         fileChannel.close();
-        inputStream.close();
+        accessFile.close();
         byteBuffer.flip();
         if (byteBuf != null) {
             byteBuf.release();
@@ -232,8 +231,8 @@ public boolean renameTo(File dest) throws IOException {
             return true;
         }
         int length = byteBuf.readableBytes();
-        FileOutputStream outputStream = new FileOutputStream(dest);
-        FileChannel fileChannel = outputStream.getChannel();
+        RandomAccessFile accessFile = new RandomAccessFile(dest, "rw");
+        FileChannel fileChannel = accessFile.getChannel();
         int written = 0;
         if (byteBuf.nioBufferCount() == 1) {
             ByteBuffer byteBuffer = byteBuf.nioBuffer();
@@ -249,7 +248,7 @@ public boolean renameTo(File dest) throws IOException {
 
         fileChannel.force(false);
         fileChannel.close();
-        outputStream.close();
+        accessFile.close();
         return written == length;
     }
 
diff --git a/handler/src/main/java/io/netty/handler/stream/ChunkedNioFile.java b/handler/src/main/java/io/netty/handler/stream/ChunkedNioFile.java
index 339a3e57769..0743a0dc65f 100644
--- a/handler/src/main/java/io/netty/handler/stream/ChunkedNioFile.java
+++ b/handler/src/main/java/io/netty/handler/stream/ChunkedNioFile.java
@@ -21,8 +21,8 @@
 import io.netty.channel.FileRegion;
 
 import java.io.File;
-import java.io.FileInputStream;
 import java.io.IOException;
+import java.io.RandomAccessFile;
 import java.nio.channels.FileChannel;
 
 /**
@@ -45,7 +45,7 @@ public class ChunkedNioFile implements ChunkedInput<ByteBuf> {
      * Creates a new instance that fetches data from the specified file.
      */
     public ChunkedNioFile(File in) throws IOException {
-        this(new FileInputStream(in).getChannel());
+        this(new RandomAccessFile(in, "r").getChannel());
     }
 
     /**
@@ -55,7 +55,7 @@ public ChunkedNioFile(File in) throws IOException {
      *                  {@link #readChunk(ChannelHandlerContext)} call
      */
     public ChunkedNioFile(File in, int chunkSize) throws IOException {
-        this(new FileInputStream(in).getChannel(), chunkSize);
+        this(new RandomAccessFile(in, "r").getChannel(), chunkSize);
     }
 
     /**


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/ReadOnlyDirectByteBufferBufTest.java b/buffer/src/test/java/io/netty/buffer/ReadOnlyDirectByteBufferBufTest.java
index 5af25396017..f92d5766824 100644
--- a/buffer/src/test/java/io/netty/buffer/ReadOnlyDirectByteBufferBufTest.java
+++ b/buffer/src/test/java/io/netty/buffer/ReadOnlyDirectByteBufferBufTest.java
@@ -21,9 +21,8 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.File;
-import java.io.FileInputStream;
-import java.io.FileOutputStream;
 import java.io.IOException;
+import java.io.RandomAccessFile;
 import java.nio.ByteBuffer;
 import java.nio.ReadOnlyBufferException;
 import java.nio.channels.FileChannel;
@@ -307,12 +306,12 @@ public void testWrapMemoryMapped() throws Exception {
         ByteBuf b2 = null;
 
         try {
-            output = new FileOutputStream(file).getChannel();
+            output = new RandomAccessFile(file, "rw").getChannel();
             byte[] bytes = new byte[1024];
             PlatformDependent.threadLocalRandom().nextBytes(bytes);
             output.write(ByteBuffer.wrap(bytes));
 
-            input = new FileInputStream(file).getChannel();
+            input = new RandomAccessFile(file, "r").getChannel();
             ByteBuffer m = input.map(FileChannel.MapMode.READ_ONLY, 0, input.size());
 
             b1 = buffer(m);
diff --git a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketFileRegionTest.java b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketFileRegionTest.java
index 881acd1bcff..ae858254259 100644
--- a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketFileRegionTest.java
+++ b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketFileRegionTest.java
@@ -32,9 +32,9 @@
 import org.junit.Test;
 
 import java.io.File;
-import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.io.RandomAccessFile;
 import java.nio.channels.WritableByteChannel;
 import java.util.Random;
 import java.util.concurrent.atomic.AtomicReference;
@@ -121,7 +121,7 @@ protected void channelRead0(ChannelHandlerContext ctx, ByteBuf msg) {
 
         // Request file region which is bigger then the underlying file.
         FileRegion region = new DefaultFileRegion(
-                new FileInputStream(file).getChannel(), 0, data.length + 1024);
+                new RandomAccessFile(file, "r").getChannel(), 0, data.length + 1024);
 
         assertThat(cc.writeAndFlush(region).await().cause(), CoreMatchers.<Throwable>instanceOf(IOException.class));
         cc.close().sync();
@@ -183,8 +183,8 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E
 
         Channel cc = cb.connect(sc.localAddress()).sync().channel();
         FileRegion region = new DefaultFileRegion(
-                new FileInputStream(file).getChannel(), startOffset, data.length - bufferSize);
-        FileRegion emptyRegion = new DefaultFileRegion(new FileInputStream(file).getChannel(), 0, 0);
+                new RandomAccessFile(file, "r").getChannel(), startOffset, data.length - bufferSize);
+        FileRegion emptyRegion = new DefaultFileRegion(new RandomAccessFile(file, "r").getChannel(), 0, 0);
 
         if (!defaultFileRegion) {
             region = new FileRegionWrapper(region);