6816049: (bf) MappedByteBuffer.force() method does not flush data correctly

Reviewed-by: chegar

src/share/classes/java/nio/Direct-X-Buffer.java.template

@@ -27,6 +27,7 @@
 
 package java.nio;
 
+import java.io.FileDescriptor;
 import sun.misc.Cleaner;
 import sun.misc.Unsafe;
 import sun.misc.VM;
@@ -114,7 +115,7 @@ class Direct$Type$Buffer$RW$$BO$
     //
     Direct$Type$Buffer$RW$(int cap) {                   // package-private
 #if[rw]
-        super(-1, 0, cap, cap, false);
+        super(-1, 0, cap, cap);
         boolean pa = VM.isDirectMemoryPageAligned();
         int ps = Bits.pageSize();
         long size = Math.max(1L, (long)cap + (pa ? ps : 0));
@@ -145,7 +146,7 @@ class Direct$Type$Buffer$RW$$BO$
     // Invoked only by JNI: NewDirectByteBuffer(void*, long)
     //
     private Direct$Type$Buffer(long addr, int cap) {
-        super(-1, 0, cap, cap, false);
+        super(-1, 0, cap, cap);
         address = addr;
         cleaner = null;
     }
@@ -154,14 +155,17 @@ class Direct$Type$Buffer$RW$$BO$
 
     // For memory-mapped buffers -- invoked by FileChannelImpl via reflection
     //
-    protected Direct$Type$Buffer$RW$(int cap, long addr, Runnable unmapper) {
+    protected Direct$Type$Buffer$RW$(int cap, long addr,
+                                     FileDescriptor fd,
+                                     Runnable unmapper)
+    {
 #if[rw]
-        super(-1, 0, cap, cap, true);
+        super(-1, 0, cap, cap, fd);
         address = addr;
         viewedBuffer = null;
         cleaner = Cleaner.create(this, unmapper);
 #else[rw]
-        super(cap, addr, unmapper);
+        super(cap, addr, fd, unmapper);
 #end[rw]
     }
 

src/share/classes/java/nio/MappedByteBuffer.java

@@ -25,6 +25,7 @@
 
 package java.nio;
 
+import java.io.FileDescriptor;
 import sun.misc.Unsafe;
 
 
@@ -71,26 +72,26 @@ public abstract class MappedByteBuffer
     // for optimization purposes, it's easier to do it the other way around.
     // This works because DirectByteBuffer is a package-private class.
 
-    // Volatile to make sure that the finalization thread sees the current
-    // value of this so that a region is not accidentally unmapped again later.
-    volatile boolean isAMappedBuffer;                   // package-private
+    // For mapped buffers, a FileDescriptor that may be used for mapping
+    // operations if valid; null if the buffer is not mapped.
+    private final FileDescriptor fd;
 
     // This should only be invoked by the DirectByteBuffer constructors
     //
     MappedByteBuffer(int mark, int pos, int lim, int cap, // package-private
-                     boolean mapped)
+                     FileDescriptor fd)
     {
         super(mark, pos, lim, cap);
-        isAMappedBuffer = mapped;
+        this.fd = fd;
     }
 
     MappedByteBuffer(int mark, int pos, int lim, int cap) { // package-private
         super(mark, pos, lim, cap);
-        isAMappedBuffer = false;
+        this.fd = null;
     }
 
     private void checkMapped() {
-        if (!isAMappedBuffer)
+        if (fd == null)
             // Can only happen if a luser explicitly casts a direct byte buffer
             throw new UnsupportedOperationException();
     }
@@ -191,13 +192,12 @@ public abstract class MappedByteBuffer
         checkMapped();
         if ((address != 0) && (capacity() != 0)) {
             long offset = mappingOffset();
-            force0(mappingAddress(offset), mappingLength(offset));
+            force0(fd, mappingAddress(offset), mappingLength(offset));
         }
         return this;
     }
 
     private native boolean isLoaded0(long address, long length, int pageCount);
     private native void load0(long address, long length);
-    private native void force0(long address, long length);
-
+    private native void force0(FileDescriptor fd, long address, long length);
 }

src/share/classes/sun/nio/ch/FileChannelImpl.java

@@ -699,15 +699,19 @@ public class FileChannelImpl
         static volatile long totalSize;
         static volatile long totalCapacity;
 
-        private long address;
-        private long size;
-        private int cap;
+        private volatile long address;
+        private final long size;
+        private final int cap;
+        private final FileDescriptor fd;
 
-        private Unmapper(long address, long size, int cap) {
+        private Unmapper(long address, long size, int cap,
+                         FileDescriptor fd)
+        {
             assert (address != 0);
             this.address = address;
             this.size = size;
             this.cap = cap;
+            this.fd = fd;
 
             synchronized (Unmapper.class) {
                 count++;
@@ -722,6 +726,15 @@ public class FileChannelImpl
             unmap0(address, size);
             address = 0;
 
+            // if this mapping has a valid file descriptor then we close it
+            if (fd.valid()) {
+                try {
+                    nd.close(fd);
+                } catch (IOException ignore) {
+                    // nothing we can do
+                }
+            }
+
             synchronized (Unmapper.class) {
                 count--;
                 totalSize -= size;
@@ -784,10 +797,12 @@ public class FileChannelImpl
             }
             if (size == 0) {
                 addr = 0;
+                // a valid file descriptor is not required
+                FileDescriptor dummy = new FileDescriptor();
                 if ((!writable) || (imode == MAP_RO))
-                    return Util.newMappedByteBufferR(0, 0, null);
+                    return Util.newMappedByteBufferR(0, 0, dummy, null);
                 else
-                    return Util.newMappedByteBuffer(0, 0, null);
+                    return Util.newMappedByteBuffer(0, 0, dummy, null);
             }
 
             int pagePosition = (int)(position % allocationGranularity);
@@ -813,14 +828,31 @@ public class FileChannelImpl
                 }
             }
 
+            // On Windows, and potentially other platforms, we need an open
+            // file descriptor for some mapping operations.
+            FileDescriptor mfd;
+            try {
+                mfd = nd.duplicateForMapping(fd);
+            } catch (IOException ioe) {
+                unmap0(addr, mapSize);
+                throw ioe;
+            }
+
             assert (IOStatus.checkAll(addr));
             assert (addr % allocationGranularity == 0);
             int isize = (int)size;
-            Unmapper um = new Unmapper(addr, size + pagePosition, isize);
-            if ((!writable) || (imode == MAP_RO))
-                return Util.newMappedByteBufferR(isize, addr + pagePosition, um);
-            else
-                return Util.newMappedByteBuffer(isize, addr + pagePosition, um);
+            Unmapper um = new Unmapper(addr, mapSize, isize, mfd);
+            if ((!writable) || (imode == MAP_RO)) {
+                return Util.newMappedByteBufferR(isize,
+                                                 addr + pagePosition,
+                                                 mfd,
+                                                 um);
+            } else {
+                return Util.newMappedByteBuffer(isize,
+                                                addr + pagePosition,
+                                                mfd,
+                                                um);
+            }
         } finally {
             threads.remove(ti);
             end(IOStatus.checkAll(addr));

src/share/classes/sun/nio/ch/FileDispatcher.java

@@ -45,4 +45,12 @@ abstract class FileDispatcher extends NativeDispatcher {
 
     abstract void release(FileDescriptor fd, long pos, long size)
         throws IOException;
+
+    /**
+     * Returns a dup of fd if a file descriptor is required for
+     * memory-mapping operations, otherwise returns an invalid
+     * FileDescriptor (meaning a newly allocated FileDescriptor)
+     */
+    abstract FileDescriptor duplicateForMapping(FileDescriptor fd)
+        throws IOException;
 }

src/share/classes/sun/nio/ch/Util.java

@@ -28,6 +28,7 @@ package sun.nio.ch;
 import java.lang.ref.SoftReference;
 import java.lang.reflect.*;
 import java.io.IOException;
+import java.io.FileDescriptor;
 import java.nio.ByteBuffer;
 import java.nio.MappedByteBuffer;
 import java.nio.channels.*;
@@ -364,6 +365,7 @@ class Util {
                         Constructor ctor = cl.getDeclaredConstructor(
                             new Class[] { int.class,
                                           long.class,
+                                          FileDescriptor.class,
                                           Runnable.class });
                         ctor.setAccessible(true);
                         directByteBufferConstructor = ctor;
@@ -381,6 +383,7 @@ class Util {
     }
 
     static MappedByteBuffer newMappedByteBuffer(int size, long addr,
+                                                FileDescriptor fd,
                                                 Runnable unmapper)
     {
         MappedByteBuffer dbb;
@@ -390,6 +393,7 @@ class Util {
             dbb = (MappedByteBuffer)directByteBufferConstructor.newInstance(
               new Object[] { new Integer(size),
                              new Long(addr),
+                             fd,
                              unmapper });
         } catch (InstantiationException e) {
             throw new InternalError();
@@ -411,6 +415,7 @@ class Util {
                         Constructor ctor = cl.getDeclaredConstructor(
                             new Class[] { int.class,
                                           long.class,
+                                          FileDescriptor.class,
                                           Runnable.class });
                         ctor.setAccessible(true);
                         directByteBufferRConstructor = ctor;
@@ -428,6 +433,7 @@ class Util {
     }
 
     static MappedByteBuffer newMappedByteBufferR(int size, long addr,
+                                                 FileDescriptor fd,
                                                  Runnable unmapper)
     {
         MappedByteBuffer dbb;
@@ -437,6 +443,7 @@ class Util {
             dbb = (MappedByteBuffer)directByteBufferRConstructor.newInstance(
               new Object[] { new Integer(size),
                              new Long(addr),
+                             fd,
                              unmapper });
         } catch (InstantiationException e) {
             throw new InternalError();

src/solaris/classes/sun/nio/ch/FileDispatcherImpl.java

@@ -94,6 +94,12 @@ class FileDispatcherImpl extends FileDispatcher
         preClose0(fd);
     }
 
+    FileDescriptor duplicateForMapping(FileDescriptor fd) {
+        // file descriptor not required for mapping operations; okay
+        // to return invalid file descriptor.
+        return new FileDescriptor();
+    }
+
     // -- Native methods --
 
     static native int read0(FileDescriptor fd, long address, int len)

src/solaris/native/java/nio/MappedByteBuffer.c

@@ -82,8 +82,8 @@ Java_java_nio_MappedByteBuffer_load0(JNIEnv *env, jobject obj, jlong address,
 
 
 JNIEXPORT void JNICALL
-Java_java_nio_MappedByteBuffer_force0(JNIEnv *env, jobject obj, jlong address,
-                                      jlong len)
+Java_java_nio_MappedByteBuffer_force0(JNIEnv *env, jobject obj, jobject fdo,
+                                      jlong address, jlong len)
 {
     void* a = (void *)jlong_to_ptr(address);
     int result = msync(a, (size_t)len, MS_SYNC);

src/windows/classes/sun/nio/ch/FileDispatcherImpl.java

@@ -26,10 +26,11 @@
 package sun.nio.ch;
 
 import java.io.*;
+import sun.misc.SharedSecrets;
+import sun.misc.JavaIOFileDescriptorAccess;
 
 class FileDispatcherImpl extends FileDispatcher
 {
-
     static {
         Util.load();
     }
@@ -94,6 +95,16 @@ class FileDispatcherImpl extends FileDispatcher
         close0(fd);
     }
 
+    FileDescriptor duplicateForMapping(FileDescriptor fd) throws IOException {
+        // on Windows we need to keep a handle to the file
+        JavaIOFileDescriptorAccess fdAccess =
+            SharedSecrets.getJavaIOFileDescriptorAccess();
+        FileDescriptor result = new FileDescriptor();
+        long handle = duplicateHandle(fdAccess.getHandle(fd));
+        fdAccess.setHandle(result, handle);
+        return result;
+    }
+
     //-- Native methods
 
     static native int read0(FileDescriptor fd, long address, int len)
@@ -132,4 +143,5 @@ class FileDispatcherImpl extends FileDispatcher
 
     static native void closeByHandle(long fd) throws IOException;
 
+    static native long duplicateHandle(long fd) throws IOException;
 }

src/windows/native/java/nio/MappedByteBuffer.c

@@ -51,11 +51,11 @@ Java_java_nio_MappedByteBuffer_load0(JNIEnv *env, jobject obj, jlong address,
 }
 
 JNIEXPORT void JNICALL
-Java_java_nio_MappedByteBuffer_force0(JNIEnv *env, jobject obj, jlong address,
-                                      jlong len)
+Java_java_nio_MappedByteBuffer_force0(JNIEnv *env, jobject obj, jobject fdo,
+                                      jlong address, jlong len)
 {
     void *a = (void *) jlong_to_ptr(address);
-    int result;
+    BOOL result;
     int retry;
 
     /*
@@ -71,6 +71,30 @@ Java_java_nio_MappedByteBuffer_force0(JNIEnv *env, jobject obj, jlong address,
         retry++;
     } while (retry < 3);
 
+    /**
+     * FlushViewOfFile only initiates the writing of dirty pages to disk
+     * so we have to call FlushFileBuffers to and ensure they are written.
+     */
+    if (result != 0) {
+        // by right, the jfieldID initialization should be in a static
+        // initializer but we do it here instead to avoiding needing to
+        // load nio.dll during startup.
+        static jfieldID handle_fdID;
+        HANDLE h;
+        if (handle_fdID == NULL) {
+            jclass clazz = (*env)->FindClass(env, "java/io/FileDescriptor");
+            if (clazz == NULL)
+                return; // exception thrown
+            handle_fdID = (*env)->GetFieldID(env, clazz, "handle", "J");
+        }
+        h = jlong_to_ptr((*env)->GetLongField(env, fdo, handle_fdID));
+        result = FlushFileBuffers(h);
+        if (result == 0 && GetLastError() == ERROR_ACCESS_DENIED) {
+            // read-only mapping
+            result = 1;
+        }
+    }
+
     if (result == 0) {
         JNU_ThrowIOExceptionWithLastError(env, "Flush failed");
     }

src/windows/native/sun/nio/ch/FileDispatcherImpl.c

@@ -32,6 +32,7 @@
 #include <io.h>
 #include "nio.h"
 #include "nio_util.h"
+#include "jlong.h"
 
 
 /**************************************************************
@@ -441,3 +442,15 @@ Java_sun_nio_ch_FileDispatcherImpl_closeByHandle(JNIEnv *env, jclass clazz,
 {
     closeFile(env, fd);
 }
+
+JNIEXPORT jlong JNICALL
+Java_sun_nio_ch_FileDispatcherImpl_duplicateHandle(JNIEnv *env, jclass this, jlong hFile)
+{
+    HANDLE hProcess = GetCurrentProcess();
+    HANDLE hResult;
+    BOOL res = DuplicateHandle(hProcess, hFile, hProcess, &hResult, 0, FALSE,
+                               DUPLICATE_SAME_ACCESS);
+    if (res == 0)
+       JNU_ThrowIOExceptionWithLastError(env, "DuplicateHandle failed");
+    return ptr_to_jlong(hResult);
+}

test/java/nio/MappedByteBuffer/Basic.java

@@ -24,7 +24,6 @@
 /* @test
  * @bug 4462336 6799037
  * @summary Simple MappedByteBuffer tests
- * @run main/othervm Basic
  */
 
 import java.io.*;
@@ -76,5 +75,10 @@ public class Basic {
             throw new RuntimeException("Incorrect isReadOnly");
         fc.close();
         raf.close();
+
+        // clean-up
+        mbb = null;
+        System.gc();
+        Thread.sleep(500);
     }
 }