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
--- a/jdk/src/share/classes/sun/nio/ch/AsynchronousFileChannelImpl.java Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/src/share/classes/sun/nio/ch/AsynchronousFileChannelImpl.java Wed Apr 15 16:16:35 2009 +0100
@@ -113,16 +113,16 @@
}
}
- 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 @@
}
/**
- * 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();
+ }
+ }
}
--- a/jdk/src/share/classes/sun/nio/ch/FileChannelImpl.java Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/src/share/classes/sun/nio/ch/FileChannelImpl.java Wed Apr 15 16:16:35 2009 +0100
@@ -33,8 +33,6 @@
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 @@
// -- 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 @@
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;
+ 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;
}
- if (result == FileDispatcher.INTERRUPTED || result == FileDispatcher.NO_LOCK) {
+ } finally {
+ if (!completed)
flt.remove(fli);
- i = false;
- }
- } catch (IOException e) {
- flt.remove(fli);
- throw e;
- } finally {
threads.remove(ti);
try {
- end(i);
+ end(completed);
} catch (ClosedByInterruptException e) {
throw new FileLockInterruptionException();
}
@@ -985,7 +986,6 @@
}
void release(FileLockImpl fli) throws IOException {
- ensureOpen();
int ti = threads.add();
try {
ensureOpen();
@@ -1005,7 +1005,7 @@
*/
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 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;
}
}
--- a/jdk/src/share/classes/sun/nio/ch/FileLockImpl.java Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/src/share/classes/sun/nio/ch/FileLockImpl.java Wed Apr 15 16:16:35 2009 +0100
@@ -31,25 +31,24 @@
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 @@
valid = false;
}
}
-
}
--- a/jdk/src/share/classes/sun/nio/ch/FileLockTable.java Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/src/share/classes/sun/nio/ch/FileLockTable.java Wed Apr 15 16:16:35 2009 +0100
@@ -61,22 +61,11 @@
public abstract void remove(FileLock fl);
/**
- * An implementation of this interface releases a given file lock.
- * Used with removeAll.
+ * Removes all file locks from the table.
+ *
+ * @return The list of file locks removed
*/
- 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
- */
- 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 @@
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 @@
}
@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 @@
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 @@
removeKeyIfEmpty(fileKey, list);
}
}
+ return result;
}
@Override
--- a/jdk/src/share/classes/sun/nio/ch/SimpleAsynchronousFileChannelImpl.java Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/src/share/classes/sun/nio/ch/SimpleAsynchronousFileChannelImpl.java Wed Apr 15 16:16:35 2009 +0100
@@ -97,6 +97,9 @@
// 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 @@
closeLock.writeLock().unlock();
}
- // Invalidate and release any locks that we still hold
- invalidateAllLocks();
-
// close file
nd.close(fdObj);
@@ -226,11 +226,9 @@
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 @@
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 @@
}
@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
--- a/jdk/src/windows/classes/sun/nio/ch/WindowsAsynchronousFileChannelImpl.java Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/src/windows/classes/sun/nio/ch/WindowsAsynchronousFileChannelImpl.java Wed Apr 15 16:16:35 2009 +0100
@@ -354,16 +354,9 @@
}
}
- // 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());
}
/**
--- a/jdk/src/windows/native/sun/nio/ch/FileDispatcherImpl.c Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/src/windows/native/sun/nio/ch/FileDispatcherImpl.c Wed Apr 15 16:16:35 2009 +0100
@@ -414,7 +414,7 @@
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");
}
}
--- a/jdk/test/java/nio/channels/AsynchronousFileChannel/Basic.java Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/test/java/nio/channels/AsynchronousFileChannel/Basic.java Wed Apr 15 16:16:35 2009 +0100
@@ -22,7 +22,7 @@
*/
/* @test
- * @bug 4607272
+ * @bug 4607272 6822643
* @summary Unit test for AsynchronousFileChannel
*/
@@ -51,7 +51,6 @@
// 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 @@
testClosedChannel(ch);
// these tests open the file themselves
+ testLocking(blah.toPath());
testCustomThreadPool(blah.toPath());
testAsynchronousClose(blah.toPath());
testCancel(blah.toPath());
@@ -160,47 +160,54 @@
}
// 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
--- a/jdk/test/java/nio/channels/AsynchronousFileChannel/Lock.java Wed Apr 15 14:53:34 2009 +0100
+++ b/jdk/test/java/nio/channels/AsynchronousFileChannel/Lock.java Wed Apr 15 16:16:35 2009 +0100
@@ -23,7 +23,7 @@
/* @test
- * @bug 4607272
+ * @bug 4607272 6814948
* @summary Unit test for AsynchronousFileChannel#lock method
*/
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/nio/channels/FileChannel/ReleaseOnCloseDeadlock.java Wed Apr 15 16:16:35 2009 +0100
@@ -0,0 +1,98 @@
+/*
+ * 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) { }
+ }
+ }
+}