6631352: File{OutputStream,Writer} should implement atomic append mode using FILE_APPEND_DATA (win)

Reviewed-by: alanb, iris

make/java/java/mapfile-vers

@@ -85,7 +85,6 @@ SUNWprivate_1.1 {
 		Java_java_io_FileOutputStream_close0;
 		Java_java_io_FileOutputStream_initIDs;
 		Java_java_io_FileOutputStream_open;
-		Java_java_io_FileOutputStream_openAppend;
 		Java_java_io_FileOutputStream_write;
 		Java_java_io_FileOutputStream_writeBytes;
 		Java_java_io_FileSystem_getFileSystem;

src/share/classes/java/io/FileOutputStream.java

@@ -58,8 +58,6 @@ class FileOutputStream extends OutputStream
 
     private FileChannel channel= null;
 
-    private boolean append = false;
-
     private final Object closeLock = new Object();
     private volatile boolean closed = false;
     private static final ThreadLocal<Boolean> runningFinalize =
@@ -200,12 +198,7 @@ class FileOutputStream extends OutputStream
         }
         fd = new FileDescriptor();
         fd.incrementAndGetUseCount();
-        this.append = append;
-        if (append) {
-            openAppend(name);
-        } else {
-            open(name);
-        }
+        open(name, append);
     }
 
     /**
@@ -250,16 +243,12 @@ class FileOutputStream extends OutputStream
     }
 
     /**
-     * Opens a file, with the specified name, for writing.
-     * @param name name of file to be opened
-     */
-    private native void open(String name) throws FileNotFoundException;
-
-    /**
-     * Opens a file, with the specified name, for appending.
+     * Opens a file, with the specified name, for overwriting or appending.
      * @param name name of file to be opened
+     * @param append whether the file is to be opened in append mode
      */
-    private native void openAppend(String name) throws FileNotFoundException;
+    private native void open(String name, boolean append)
+        throws FileNotFoundException;
 
     /**
      * Writes the specified byte to this file output stream. Implements
@@ -383,7 +372,7 @@ class FileOutputStream extends OutputStream
     public FileChannel getChannel() {
         synchronized (this) {
             if (channel == null) {
-                channel = FileChannelImpl.open(fd, false, true, this, append);
+                channel = FileChannelImpl.open(fd, false, true, this);
 
                 /*
                  * Increment fd's use count. Invoking the channel's close()

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

@@ -52,39 +52,37 @@ public class FileChannelImpl
 {
 
     // Used to make native read and write calls
-    private static NativeDispatcher nd;
+    private static final NativeDispatcher nd;
 
     // Memory allocation size for mapping buffers
-    private static long allocationGranularity;
+    private static final long allocationGranularity;
 
     // Cached field for MappedByteBuffer.isAMappedBuffer
-    private static Field isAMappedBufferField;
+    private static final Field isAMappedBufferField;
 
     // File descriptor
-    private FileDescriptor fd;
+    private final FileDescriptor fd;
 
     // File access mode (immutable)
-    private boolean writable;
-    private boolean readable;
-    private boolean appending;
+    private final boolean writable;
+    private final boolean readable;
 
     // Required to prevent finalization of creating stream (immutable)
-    private Object parent;
+    private final Object parent;
 
     // Thread-safe set of IDs of native threads, for signalling
-    private NativeThreadSet threads = new NativeThreadSet(2);
+    private final NativeThreadSet threads = new NativeThreadSet(2);
 
     // Lock for operations involving position and size
-    private Object positionLock = new Object();
+    private final Object positionLock = new Object();
 
     private FileChannelImpl(FileDescriptor fd, boolean readable,
-                            boolean writable, Object parent, boolean append)
+                            boolean writable, Object parent)
     {
         this.fd = fd;
         this.readable = readable;
         this.writable = writable;
         this.parent = parent;
-        this.appending = append;
     }
 
     // Invoked by getChannel() methods
@@ -94,14 +92,7 @@ public class FileChannelImpl
                                    boolean readable, boolean writable,
                                    Object parent)
     {
-        return new FileChannelImpl(fd, readable, writable, parent, false);
-    }
-
-    public static FileChannel open(FileDescriptor fd,
-                                   boolean readable, boolean writable,
-                                   Object parent, boolean append)
-    {
-        return new FileChannelImpl(fd, readable, writable, parent, append);
+        return new FileChannelImpl(fd, readable, writable, parent);
     }
 
     private void ensureOpen() throws IOException {
@@ -134,15 +125,7 @@ public class FileChannelImpl
             // superclass AbstractInterruptibleChannel, but the isOpen logic in
             // that method will prevent this method from being reinvoked.
             //
-            if (parent instanceof FileInputStream)
-                ((FileInputStream)parent).close();
-            else if (parent instanceof FileOutputStream)
-                ((FileOutputStream)parent).close();
-            else if (parent instanceof RandomAccessFile)
-                ((RandomAccessFile)parent).close();
-            else
-                assert false;
-
+            ((java.io.Closeable)parent).close();
         } else {
             nd.close(fd);
         }
@@ -218,8 +201,6 @@ public class FileChannelImpl
                 if (!isOpen())
                     return 0;
                 ti = threads.add();
-                if (appending)
-                    position(size());
                 do {
                     n = IOUtil.write(fd, src, -1, nd, positionLock);
                 } while ((n == IOStatus.INTERRUPTED) && isOpen());
@@ -244,8 +225,6 @@ public class FileChannelImpl
                 if (!isOpen())
                     return 0;
                 ti = threads.add();
-                if (appending)
-                    position(size());
                 do {
                     n = IOUtil.write(fd, srcs, nd);
                 } while ((n == IOStatus.INTERRUPTED) && isOpen());
@@ -1051,7 +1030,7 @@ public class FileChannelImpl
         private FileKey fileKey;
 
         FileLockReference(FileLock referent,
-                          ReferenceQueue queue,
+                          ReferenceQueue<FileLock> queue,
                           FileKey key) {
             super(referent, queue);
             this.fileKey = key;
@@ -1073,7 +1052,7 @@ public class FileChannelImpl
             new ConcurrentHashMap<FileKey, ArrayList<FileLockReference>>();
 
         // reference queue for cleared refs
-        private static ReferenceQueue queue = new ReferenceQueue();
+        private static ReferenceQueue<FileLock> queue = new ReferenceQueue<FileLock>();
 
         // the enclosing file channel
         private FileChannelImpl fci;

src/share/native/java/io/io_util.c

@@ -95,7 +95,7 @@ readBytes(JNIEnv *env, jobject this, jbyteArray bytes,
     fd = GET_FD(this, fid);
     if (fd == -1) {
         JNU_ThrowIOException(env, "Stream Closed");
-        return  -1;
+        return -1;
     }
 
     nread = IO_Read(fd, buf, len);

src/solaris/native/java/io/FileOutputStream_md.c

@@ -53,13 +53,10 @@ Java_java_io_FileOutputStream_initIDs(JNIEnv *env, jclass fdClass) {
  */
 
 JNIEXPORT void JNICALL
-Java_java_io_FileOutputStream_open(JNIEnv *env, jobject this, jstring path) {
-    fileOpen(env, this, path, fos_fd, O_WRONLY | O_CREAT | O_TRUNC);
-}
-
-JNIEXPORT void JNICALL
-Java_java_io_FileOutputStream_openAppend(JNIEnv *env, jobject this, jstring path) {
-    fileOpen(env, this, path, fos_fd, O_WRONLY | O_CREAT | O_APPEND);
+Java_java_io_FileOutputStream_open(JNIEnv *env, jobject this,
+                                   jstring path, jboolean append) {
+    fileOpen(env, this, path, fos_fd,
+             O_WRONLY | O_CREAT | (append ? O_APPEND : O_TRUNC));
 }
 
 JNIEXPORT void JNICALL

src/windows/native/java/io/FileOutputStream_md.c

@@ -39,8 +39,6 @@
 
 jfieldID fos_fd; /* id for jobject 'fd' in java.io.FileOutputStream */
 
-jfieldID fos_append;
-
 /**************************************************************
  * static methods to store field ID's in initializers
  */
@@ -49,7 +47,6 @@ JNIEXPORT void JNICALL
 Java_java_io_FileOutputStream_initIDs(JNIEnv *env, jclass fosClass) {
     fos_fd =
         (*env)->GetFieldID(env, fosClass, "fd", "Ljava/io/FileDescriptor;");
-    fos_append = (*env)->GetFieldID(env, fosClass, "append", "Z");
 }
 
 /**************************************************************
@@ -57,45 +54,20 @@ Java_java_io_FileOutputStream_initIDs(JNIEnv *env, jclass fosClass) {
  */
 
 JNIEXPORT void JNICALL
-Java_java_io_FileOutputStream_open(JNIEnv *env, jobject this, jstring path) {
-    fileOpen(env, this, path, fos_fd, O_WRONLY | O_CREAT | O_TRUNC);
-}
-
-JNIEXPORT void JNICALL
-Java_java_io_FileOutputStream_openAppend(JNIEnv *env, jobject this, jstring path) {
-    fileOpen(env, this, path, fos_fd, O_WRONLY | O_CREAT | O_APPEND);
+Java_java_io_FileOutputStream_open(JNIEnv *env, jobject this,
+                                   jstring path, jboolean append) {
+    fileOpen(env, this, path, fos_fd,
+             O_WRONLY | O_CREAT | (append ? O_APPEND : O_TRUNC));
 }
 
 JNIEXPORT void JNICALL
 Java_java_io_FileOutputStream_write(JNIEnv *env, jobject this, jint byte) {
-    jboolean append = (*env)->GetBooleanField(env, this, fos_append);
-    FD fd = GET_FD(this, fos_fd);
-    if (fd == -1) {
-        JNU_ThrowIOException(env, "Stream Closed");
-        return;
-    }
-    if (append == JNI_TRUE) {
-        if (IO_Lseek(fd, 0L, SEEK_END) == -1) {
-            JNU_ThrowIOExceptionWithLastError(env, "Append failed");
-        }
-    }
     writeSingle(env, this, byte, fos_fd);
 }
 
 JNIEXPORT void JNICALL
 Java_java_io_FileOutputStream_writeBytes(JNIEnv *env,
     jobject this, jbyteArray bytes, jint off, jint len) {
-    jboolean append = (*env)->GetBooleanField(env, this, fos_append);
-    FD fd = GET_FD(this, fos_fd);
-    if (fd == -1) {
-        JNU_ThrowIOException(env, "Stream Closed");
-        return;
-    }
-    if (append == JNI_TRUE) {
-        if (IO_Lseek(fd, 0L, SEEK_END) == -1) {
-            JNU_ThrowIOExceptionWithLastError(env, "Append failed");
-        }
-    }
     writeBytes(env, this, bytes, off, len, fos_fd);
 }
 

src/windows/native/java/io/io_util_md.c

@@ -42,7 +42,7 @@
 
 extern jboolean onNT = JNI_FALSE;
 
-static int MAX_INPUT_EVENTS = 2000;
+static DWORD MAX_INPUT_EVENTS = 2000;
 
 void
 initializeWindowsVersion() {
@@ -190,9 +190,16 @@ pathToNTPath(JNIEnv *env, jstring path, jboolean throwFNFE) {
 jlong
 winFileHandleOpen(JNIEnv *env, jstring path, int flags)
 {
+    /* To implement O_APPEND, we use the strategy from
+       http://msdn2.microsoft.com/en-us/library/aa363858.aspx
+       "You can get atomic append by opening a file with
+       FILE_APPEND_DATA access and _without_ FILE_WRITE_DATA access.
+       If you do this then all writes will ignore the current file
+       pointer and be done at the end-of file." */
     const DWORD access =
-        (flags & O_RDWR)   ? (GENERIC_WRITE | GENERIC_READ) :
+        (flags & O_APPEND) ? (FILE_GENERIC_WRITE & ~FILE_WRITE_DATA) :
         (flags & O_WRONLY) ?  GENERIC_WRITE :
+        (flags & O_RDWR)   ? (GENERIC_READ | GENERIC_WRITE) :
         GENERIC_READ;
     const DWORD sharing =
         FILE_SHARE_READ | FILE_SHARE_WRITE;
@@ -495,7 +502,7 @@ handleClose(JNIEnv *env, jobject this, jfieldID fid)
     FD fd = GET_FD(this, fid);
     HANDLE h = (HANDLE)fd;
 
-    if (fd == INVALID_HANDLE_VALUE) {
+    if (h == INVALID_HANDLE_VALUE) {
         return 0;
     }
 

test/java/io/FileOutputStream/AtomicAppend.java

+/*
+ * Copyright 2007 Sun Microsystems, Inc.  All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
+ * CA 95054 USA or visit www.sun.com if you need additional information or
+ * have any questions.
+ */
+
+/*
+ * @test
+ * @bug 6631352
+ * @summary Check that appends are atomic
+ */
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
+
+public class AtomicAppend {
+    // Before the fix for
+    // 6631352: Implement atomic append mode using FILE_APPEND_DATA (win)
+    // this would fail intermittently on windows
+    void test(String[] args) throws Throwable {
+        final int nThreads = 10;
+        final int writes = 1000;
+        final File file = new File("foo");
+        file.delete();
+        try {
+            final ExecutorService es = Executors.newFixedThreadPool(nThreads);
+            for (int i = 0; i < nThreads; i++)
+                es.execute(new Runnable() { public void run() {
+                    try {
+                        FileOutputStream s = new FileOutputStream(file, true);
+                        for (int j = 0; j < 1000; j++) {
+                            s.write((int) 'x');
+                            s.flush();
+                        }
+                        s.close();
+                    } catch (Throwable t) { unexpected(t); }}});
+            es.shutdown();
+            es.awaitTermination(10L, TimeUnit.MINUTES);
+            equal(file.length(), (long) (nThreads * writes));
+        } finally {
+            file.delete();
+        }
+    }
+
+    //--------------------- Infrastructure ---------------------------
+    volatile int passed = 0, failed = 0;
+    void pass() {passed++;}
+    void fail() {failed++; Thread.dumpStack();}
+    void fail(String msg) {System.err.println(msg); fail();}
+    void unexpected(Throwable t) {failed++; t.printStackTrace();}
+    void check(boolean cond) {if (cond) pass(); else fail();}
+    void equal(Object x, Object y) {
+        if (x == null ? y == null : x.equals(y)) pass();
+        else fail(x + " not equal to " + y);}
+    public static void main(String[] args) throws Throwable {
+        new AtomicAppend().instanceMain(args);}
+    void instanceMain(String[] args) throws Throwable {
+        try {test(args);} catch (Throwable t) {unexpected(t);}
+        System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed);
+        if (failed > 0) throw new AssertionError("Some tests failed");}
+}