6543863: (fc) FileLock.release can deadlock with FileChannel.close

6429910: (fc) FileChannel.lock() IOException: Bad file number, not AsynchronousCloseException
6814948: (fc) test/java/nio/channels/AsynchronousFileChannel/Lock.java failed intermittently
6822643: (fc) AsynchronousFileChannel.close does not invalidate FileLocks
Reviewed-by: sherman

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

@@ -113,16 +113,16 @@ abstract class AsynchronousFileChannelImpl
         }
     }
 
-    final void invalidateAllLocks() {
+    final void invalidateAllLocks() throws IOException {
         if (fileLockTable != null) {
-            try {
-                fileLockTable.removeAll( new FileLockTable.Releaser() {
-                    public void release(FileLock fl) {
-                        ((FileLockImpl)fl).invalidate();
+            for (FileLock fl: fileLockTable.removeAll()) {
+                synchronized (fl) {
+                    if (fl.isValid()) {
+                        FileLockImpl fli = (FileLockImpl)fl;
+                        implRelease(fli);
+                        fli.invalidate();
                     }
-                });
-            } catch (IOException e) {
-                throw new AssertionError(e);
+                }
             }
         }
     }
@@ -158,7 +158,21 @@ abstract class AsynchronousFileChannelImpl
     }
 
     /**
-     * Invoked by FileLockImpl to release lock acquired by this channel.
+     * Releases the given file lock.
+     */
+    protected abstract void implRelease(FileLockImpl fli) throws IOException;
+
+    /**
+     * Invoked by FileLockImpl to release the given file lock and remove it
+     * from the lock table.
      */
-    abstract void release(FileLockImpl fli) throws IOException;
+    final void release(FileLockImpl fli) throws IOException {
+        try {
+            begin();
+            implRelease(fli);
+            removeFromFileLockTable(fli);
+        } finally {
+            end();
+        }
+    }
 }

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

@@ -33,8 +33,6 @@ import java.nio.BufferPoolMXBean;
 import java.nio.channels.*;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Iterator;
-import java.lang.reflect.Field;
 import java.security.AccessController;
 import javax.management.ObjectName;
 import javax.management.MalformedObjectNameException;
@@ -95,14 +93,16 @@ public class FileChannelImpl
     // -- Standard channel operations --
 
     protected void implCloseChannel() throws IOException {
-        // Invalidate and release any locks that we still hold
+        // Release and invalidate any locks that we still hold
         if (fileLockTable != null) {
-            fileLockTable.removeAll( new FileLockTable.Releaser() {
-                public void release(FileLock fl) throws IOException {
-                    ((FileLockImpl)fl).invalidate();
-                    nd.release(fd, fl.position(), fl.size());
+            for (FileLock fl: fileLockTable.removeAll()) {
+                synchronized (fl) {
+                    if (fl.isValid()) {
+                        nd.release(fd, fl.position(), fl.size());
+                        ((FileLockImpl)fl).invalidate();
+                    }
                 }
-            });
+            }
         }
 
         nd.preClose(fd);
@@ -912,32 +912,33 @@ public class FileChannelImpl
         FileLockImpl fli = new FileLockImpl(this, position, size, shared);
         FileLockTable flt = fileLockTable();
         flt.add(fli);
-        boolean i = true;
+        boolean completed = false;
         int ti = -1;
         try {
             begin();
             ti = threads.add();
             if (!isOpen())
                 return null;
-            int result = nd.lock(fd, true, position, size, shared);
-            if (result == FileDispatcher.RET_EX_LOCK) {
-                assert shared;
-                FileLockImpl fli2 = new FileLockImpl(this, position, size,
-                                                     false);
-                flt.replace(fli, fli2);
-                return fli2;
-            }
-            if (result == FileDispatcher.INTERRUPTED || result == FileDispatcher.NO_LOCK) {
-                flt.remove(fli);
-                i = false;
+            int n;
+            do {
+                n = nd.lock(fd, true, position, size, shared);
+            } while ((n == FileDispatcher.INTERRUPTED) && isOpen());
+            if (isOpen()) {
+                if (n == FileDispatcher.RET_EX_LOCK) {
+                    assert shared;
+                    FileLockImpl fli2 = new FileLockImpl(this, position, size,
+                                                         false);
+                    flt.replace(fli, fli2);
+                    fli = fli2;
+                }
+                completed = true;
             }
-        } catch (IOException e) {
-            flt.remove(fli);
-            throw e;
         } finally {
+            if (!completed)
+                flt.remove(fli);
             threads.remove(ti);
             try {
-                end(i);
+                end(completed);
             } catch (ClosedByInterruptException e) {
                 throw new FileLockInterruptionException();
             }
@@ -985,7 +986,6 @@ public class FileChannelImpl
     }
 
     void release(FileLockImpl fli) throws IOException {
-        ensureOpen();
         int ti = threads.add();
         try {
             ensureOpen();
@@ -1005,7 +1005,7 @@ public class FileChannelImpl
      */
     private static class SimpleFileLockTable extends FileLockTable {
         // synchronize on list for access
-        private List<FileLock> lockList = new ArrayList<FileLock>(2);
+        private final List<FileLock> lockList = new ArrayList<FileLock>(2);
 
         public SimpleFileLockTable() {
         }
@@ -1034,14 +1034,11 @@ public class FileChannelImpl
             }
         }
 
-        public void removeAll(Releaser releaser) throws IOException {
+        public List<FileLock> removeAll() {
             synchronized(lockList) {
-                Iterator<FileLock> i = lockList.iterator();
-                while (i.hasNext()) {
-                    FileLock fl = i.next();
-                    releaser.release(fl);
-                    i.remove();
-                }
+                List<FileLock> result = new ArrayList<FileLock>(lockList);
+                lockList.clear();
+                return result;
             }
         }
 

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

@@ -31,25 +31,24 @@ import java.nio.channels.*;
 public class FileLockImpl
     extends FileLock
 {
-    boolean valid;
+    private volatile boolean valid = true;
 
     FileLockImpl(FileChannel channel, long position, long size, boolean shared)
     {
         super(channel, position, size, shared);
-        this.valid = true;
     }
 
     FileLockImpl(AsynchronousFileChannel channel, long position, long size, boolean shared)
     {
         super(channel, position, size, shared);
-        this.valid = true;
     }
 
-    public synchronized boolean isValid() {
+    public boolean isValid() {
         return valid;
     }
 
-    synchronized void invalidate() {
+    void invalidate() {
+        assert Thread.holdsLock(this);
         valid = false;
     }
 
@@ -66,5 +65,4 @@ public class FileLockImpl
             valid = false;
         }
     }
-
 }

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

@@ -60,23 +60,12 @@ abstract class FileLockTable {
      */
     public abstract void remove(FileLock fl);
 
-    /**
-     * An implementation of this interface releases a given file lock.
-     * Used with removeAll.
-     */
-    public abstract interface Releaser {
-        void release(FileLock fl) throws IOException;
-    }
-
     /**
      * Removes all file locks from the table.
-     * <p>
-     * The Releaser#release method is invoked for each file lock before
-     * it is removed.
      *
-     * @throws IOException if the release method throws IOException
+     * @return  The list of file locks removed
      */
-    public abstract void removeAll(Releaser r) throws IOException;
+    public abstract List<FileLock> removeAll();
 
     /**
      * Replaces an existing file lock in the table.
@@ -195,7 +184,7 @@ class SharedFileLockTable extends FileLockTable {
                 FileLockReference ref = list.get(index);
                 FileLock lock = ref.get();
                 if (lock == fl) {
-                    assert (lock != null) && (lock.channel() == channel);
+                    assert (lock != null) && (lock.acquiredBy() == channel);
                     ref.clear();
                     list.remove(index);
                     break;
@@ -206,7 +195,8 @@ class SharedFileLockTable extends FileLockTable {
     }
 
     @Override
-    public void removeAll(Releaser releaser) throws IOException {
+    public List<FileLock> removeAll() {
+        List<FileLock> result = new ArrayList<FileLock>();
         List<FileLockReference> list = lockMap.get(fileKey);
         if (list != null) {
             synchronized (list) {
@@ -216,13 +206,13 @@ class SharedFileLockTable extends FileLockTable {
                     FileLock lock = ref.get();
 
                     // remove locks obtained by this channel
-                    if (lock != null && lock.channel() == channel) {
-                        // invoke the releaser to invalidate/release the lock
-                        releaser.release(lock);
-
+                    if (lock != null && lock.acquiredBy() == channel) {
                         // remove the lock from the list
                         ref.clear();
                         list.remove(index);
+
+                        // add to result
+                        result.add(lock);
                     } else {
                         index++;
                     }
@@ -232,6 +222,7 @@ class SharedFileLockTable extends FileLockTable {
                 removeKeyIfEmpty(fileKey, list);
             }
         }
+        return result;
     }
 
     @Override

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

@@ -97,6 +97,9 @@ public class SimpleAsynchronousFileChannelImpl
             // then it will throw ClosedChannelException
         }
 
+        // Invalidate and release any locks that we still hold
+        invalidateAllLocks();
+
         // signal any threads blocked on this channel
         nd.preClose(fdObj);
         threads.signalAndWait();
@@ -109,9 +112,6 @@ public class SimpleAsynchronousFileChannelImpl
             closeLock.writeLock().unlock();
         }
 
-        // Invalidate and release any locks that we still hold
-        invalidateAllLocks();
-
         // close file
         nd.close(fdObj);
 
@@ -226,11 +226,9 @@ public class SimpleAsynchronousFileChannelImpl
                         do {
                             n = nd.lock(fdObj, true, position, size, shared);
                         } while ((n == FileDispatcher.INTERRUPTED) && isOpen());
-                        if (n == FileDispatcher.LOCKED) {
+                        if (n == FileDispatcher.LOCKED && isOpen()) {
                             result.setResult(fli);
                         } else {
-                            if (n != FileDispatcher.INTERRUPTED)
-                                throw new AssertionError();
                             throw new AsynchronousCloseException();
                         }
                     } catch (IOException x) {
@@ -279,16 +277,16 @@ public class SimpleAsynchronousFileChannelImpl
             do {
                 n = nd.lock(fdObj, false, position, size, shared);
             } while ((n == FileDispatcher.INTERRUPTED) && isOpen());
-            if (n != FileDispatcher.LOCKED) {
-                if (n == FileDispatcher.NO_LOCK)
-                    return null;    // locked by someone else
-                if (n == FileDispatcher.INTERRUPTED)
-                    throw new AsynchronousCloseException();
-                // should not get here
-                throw new AssertionError();
+            if (n == FileDispatcher.LOCKED && isOpen()) {
+                gotLock = true;
+                return fli;    // lock acquired
             }
-            gotLock = true;
-            return fli;
+            if (n == FileDispatcher.NO_LOCK)
+                return null;    // locked by someone else
+            if (n == FileDispatcher.INTERRUPTED)
+                throw new AsynchronousCloseException();
+            // should not get here
+            throw new AssertionError();
         } finally {
             if (!gotLock)
                 removeFromFileLockTable(fli);
@@ -298,14 +296,8 @@ public class SimpleAsynchronousFileChannelImpl
     }
 
     @Override
-    void release(FileLockImpl fli) throws IOException {
-        try {
-            begin();
-            nd.release(fdObj, fli.position(), fli.size());
-            removeFromFileLockTable(fli);
-        } finally {
-            end();
-        }
+    protected void implRelease(FileLockImpl fli) throws IOException {
+        nd.release(fdObj, fli.position(), fli.size());
     }
 
     @Override

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

@@ -354,16 +354,9 @@ public class WindowsAsynchronousFileChannelImpl
         }
     }
 
-    // invoke by FileFileImpl to release lock
     @Override
-    void release(FileLockImpl fli) throws IOException {
-        try {
-            begin();
-            nd.release(fdObj, fli.position(), fli.size());
-            removeFromFileLockTable(fli);
-        } finally {
-            end();
-        }
+    protected void implRelease(FileLockImpl fli) throws IOException {
+        nd.release(fdObj, fli.position(), fli.size());
     }
 
     /**

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

@@ -414,7 +414,7 @@ Java_sun_nio_ch_FileDispatcherImpl_release0(JNIEnv *env, jobject this,
     o.Offset = lowPos;
     o.OffsetHigh = highPos;
     result = UnlockFileEx(h, 0, lowNumBytes, highNumBytes, &o);
-    if (result == 0) {
+    if (result == 0 && GetLastError() != ERROR_NOT_LOCKED) {
         JNU_ThrowIOExceptionWithLastError(env, "Release failed");
     }
 }

test/java/nio/channels/AsynchronousFileChannel/Basic.java

@@ -22,7 +22,7 @@
  */
 
 /* @test
- * @bug 4607272
+ * @bug 4607272 6822643
  * @summary Unit test for AsynchronousFileChannel
  */
 
@@ -51,7 +51,6 @@ public class Basic {
         // run tests
         testUsingCompletionHandlers(ch);
         testUsingWaitOnResult(ch);
-        testLocking(ch);
         testInterruptHandlerThread(ch);
 
         // close channel and invoke test that expects channel to be closed
@@ -59,6 +58,7 @@ public class Basic {
         testClosedChannel(ch);
 
         // these tests open the file themselves
+        testLocking(blah.toPath());
         testCustomThreadPool(blah.toPath());
         testAsynchronousClose(blah.toPath());
         testCancel(blah.toPath());
@@ -160,47 +160,54 @@ public class Basic {
     }
 
     // exercise lock methods
-    static void testLocking(AsynchronousFileChannel ch)
-        throws IOException
-    {
+    static void testLocking(Path file) throws IOException {
         System.out.println("testLocking");
 
-        // test 1 - acquire lock and check that tryLock throws
-        // OverlappingFileLockException
+        AsynchronousFileChannel ch = AsynchronousFileChannel
+            .open(file, READ, WRITE);
         FileLock fl;
         try {
-            fl = ch.lock().get();
-        } catch (ExecutionException x) {
-            throw new RuntimeException(x);
-        } catch (InterruptedException x) {
-            throw new RuntimeException("Should not be interrupted");
-        }
-        if (!fl.acquiredBy().equals(ch))
-            throw new RuntimeException("FileLock#acquiredBy returned incorrect channel");
-        try {
-            ch.tryLock();
-            throw new RuntimeException("OverlappingFileLockException expected");
-        } catch (OverlappingFileLockException x) {
-        }
-        fl.release();
+            // test 1 - acquire lock and check that tryLock throws
+            // OverlappingFileLockException
+            try {
+                fl = ch.lock().get();
+            } catch (ExecutionException x) {
+                throw new RuntimeException(x);
+            } catch (InterruptedException x) {
+                throw new RuntimeException("Should not be interrupted");
+            }
+            if (!fl.acquiredBy().equals(ch))
+                throw new RuntimeException("FileLock#acquiredBy returned incorrect channel");
+            try {
+                ch.tryLock();
+                throw new RuntimeException("OverlappingFileLockException expected");
+            } catch (OverlappingFileLockException x) {
+            }
+            fl.release();
 
-        // test 2 - acquire try and check that lock throws OverlappingFileLockException
-        fl = ch.tryLock();
-        if (fl == null)
-            throw new RuntimeException("Unable to acquire lock");
-        try {
-            ch.lock(null, new CompletionHandler<FileLock,Void> () {
-                public void completed(FileLock result, Void att) {
-                }
-                public void failed(Throwable exc, Void att) {
-                }
-                public void cancelled(Void att) {
-                }
-            });
-            throw new RuntimeException("OverlappingFileLockException expected");
-        } catch (OverlappingFileLockException x) {
+            // test 2 - acquire try and check that lock throws OverlappingFileLockException
+            fl = ch.tryLock();
+            if (fl == null)
+                throw new RuntimeException("Unable to acquire lock");
+            try {
+                ch.lock(null, new CompletionHandler<FileLock,Void> () {
+                    public void completed(FileLock result, Void att) {
+                    }
+                    public void failed(Throwable exc, Void att) {
+                    }
+                    public void cancelled(Void att) {
+                    }
+                });
+                throw new RuntimeException("OverlappingFileLockException expected");
+            } catch (OverlappingFileLockException x) {
+            }
+        } finally {
+            ch.close();
         }
-        fl.release();
+
+        // test 3 - channel is closed so FileLock should no longer be valid
+        if (fl.isValid())
+            throw new RuntimeException("FileLock expected to be invalid");
     }
 
     // interrupt should not close channel

test/java/nio/channels/AsynchronousFileChannel/Lock.java

@@ -23,7 +23,7 @@
 
 
 /* @test
- * @bug 4607272
+ * @bug 4607272 6814948
  * @summary Unit test for AsynchronousFileChannel#lock method
  */
 

test/java/nio/channels/FileChannel/ReleaseOnCloseDeadlock.java

+/*
+ * Copyright 2009 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 6543863
+ * @summary Try to cause a deadlock between (Asynchronous)FileChannel.close
+ *   and FileLock.release
+ */
+
+import java.io.*;
+import java.nio.file.Path;
+import static java.nio.file.StandardOpenOption.*;
+import java.nio.channels.*;
+import java.util.concurrent.*;
+
+public class ReleaseOnCloseDeadlock {
+    private static final int LOCK_COUNT = 1024;
+
+    public static void main(String[] args) throws IOException {
+        File blah = File.createTempFile("blah", null);
+        blah.deleteOnExit();
+        for (int i=0; i<100; i++) {
+            test(blah.toPath());
+        }
+    }
+
+    static void test(Path file) throws IOException {
+        FileLock[] locks = new FileLock[LOCK_COUNT];
+
+        FileChannel fc = FileChannel.open(file, READ, WRITE);
+        for (int i=0; i<LOCK_COUNT; i++) {
+            locks[i] = fc.lock(i, 1, true);
+        }
+        tryToDeadlock(fc, locks);
+
+        AsynchronousFileChannel ch = AsynchronousFileChannel.open(file, READ, WRITE);
+        for (int i=0; i<LOCK_COUNT; i++) {
+            try {
+                locks[i] = ch.lock(i, 1, true, null, null).get();
+            } catch (InterruptedException x) {
+                throw new RuntimeException(x);
+            } catch (ExecutionException x) {
+                throw new RuntimeException(x);
+            }
+        }
+        tryToDeadlock(ch, locks);
+    }
+
+    static void tryToDeadlock(final Channel channel, FileLock[] locks)
+        throws IOException
+    {
+        // start thread to close the file (and invalidate the locks)
+        Thread closer = new Thread(
+            new Runnable() {
+                public void run() {
+                    try {
+                        channel.close();
+                    } catch (IOException ignore) {
+                        ignore.printStackTrace();
+                    }
+                }});
+        closer.start();
+
+        // release the locks explicitly
+        for (int i=0; i<locks.length; i++) {
+            try {
+                locks[i].release();
+            } catch (ClosedChannelException ignore) { }
+        }
+
+        // we are done when closer has terminated
+        while (closer.isAlive()) {
+            try {
+                closer.join();
+            } catch (InterruptedException ignore) { }
+        }
+    }
+}