From b315e54c3597bc12655258ad96039f5567feba7d Mon Sep 17 00:00:00 2001 From: alanb Date: Wed, 17 Apr 2013 16:11:19 +0100 Subject: [PATCH] 8012019: (fc) Thread.interrupt triggers hang in FileChannelImpl.pread (win) Reviewed-by: chegar --- .../sun/nio/ch/DatagramChannelImpl.java | 4 +- .../classes/sun/nio/ch/FileChannelImpl.java | 30 +++- src/share/classes/sun/nio/ch/IOUtil.java | 22 ++- .../classes/sun/nio/ch/NativeDispatcher.java | 16 +- .../ch/SimpleAsynchronousFileChannelImpl.java | 4 +- .../classes/sun/nio/ch/SocketChannelImpl.java | 4 +- .../sun/nio/ch/FileDispatcherImpl.java | 9 +- .../classes/sun/nio/ch/SinkChannelImpl.java | 2 +- .../classes/sun/nio/ch/SourceChannelImpl.java | 2 +- .../ch/UnixAsynchronousSocketChannelImpl.java | 8 +- .../sun/nio/ch/FileDispatcherImpl.java | 21 +-- .../FileChannel/InterruptDeadlock.java | 137 ++++++++++++++++++ 12 files changed, 213 insertions(+), 46 deletions(-) create mode 100644 test/java/nio/channels/FileChannel/InterruptDeadlock.java diff --git a/src/share/classes/sun/nio/ch/DatagramChannelImpl.java b/src/share/classes/sun/nio/ch/DatagramChannelImpl.java index cdd5b3c2a..4e41a9aa1 100644 --- a/src/share/classes/sun/nio/ch/DatagramChannelImpl.java +++ b/src/share/classes/sun/nio/ch/DatagramChannelImpl.java @@ -538,7 +538,7 @@ class DatagramChannelImpl return 0; readerThread = NativeThread.current(); do { - n = IOUtil.read(fd, buf, -1, nd, readLock); + n = IOUtil.read(fd, buf, -1, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); return IOStatus.normalize(n); } finally { @@ -594,7 +594,7 @@ class DatagramChannelImpl return 0; writerThread = NativeThread.current(); do { - n = IOUtil.write(fd, buf, -1, nd, writeLock); + n = IOUtil.write(fd, buf, -1, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); return IOStatus.normalize(n); } finally { diff --git a/src/share/classes/sun/nio/ch/FileChannelImpl.java b/src/share/classes/sun/nio/ch/FileChannelImpl.java index a9b979166..ac9be90f2 100644 --- a/src/share/classes/sun/nio/ch/FileChannelImpl.java +++ b/src/share/classes/sun/nio/ch/FileChannelImpl.java @@ -140,7 +140,7 @@ public class FileChannelImpl if (!isOpen()) return 0; do { - n = IOUtil.read(fd, dst, -1, nd, positionLock); + n = IOUtil.read(fd, dst, -1, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); return IOStatus.normalize(n); } finally { @@ -192,7 +192,7 @@ public class FileChannelImpl if (!isOpen()) return 0; do { - n = IOUtil.write(fd, src, -1, nd, positionLock); + n = IOUtil.write(fd, src, -1, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); return IOStatus.normalize(n); } finally { @@ -671,6 +671,17 @@ public class FileChannelImpl if (!readable) throw new NonReadableChannelException(); ensureOpen(); + if (nd.needsPositionLock()) { + synchronized (positionLock) { + return readInternal(dst, position); + } + } else { + return readInternal(dst, position); + } + } + + private int readInternal(ByteBuffer dst, long position) throws IOException { + assert !nd.needsPositionLock() || Thread.holdsLock(positionLock); int n = 0; int ti = -1; try { @@ -679,7 +690,7 @@ public class FileChannelImpl if (!isOpen()) return -1; do { - n = IOUtil.read(fd, dst, position, nd, positionLock); + n = IOUtil.read(fd, dst, position, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); return IOStatus.normalize(n); } finally { @@ -697,6 +708,17 @@ public class FileChannelImpl if (!writable) throw new NonWritableChannelException(); ensureOpen(); + if (nd.needsPositionLock()) { + synchronized (positionLock) { + return writeInternal(src, position); + } + } else { + return writeInternal(src, position); + } + } + + private int writeInternal(ByteBuffer src, long position) throws IOException { + assert !nd.needsPositionLock() || Thread.holdsLock(positionLock); int n = 0; int ti = -1; try { @@ -705,7 +727,7 @@ public class FileChannelImpl if (!isOpen()) return -1; do { - n = IOUtil.write(fd, src, position, nd, positionLock); + n = IOUtil.write(fd, src, position, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); return IOStatus.normalize(n); } finally { diff --git a/src/share/classes/sun/nio/ch/IOUtil.java b/src/share/classes/sun/nio/ch/IOUtil.java index 7846486ec..6f45da73d 100644 --- a/src/share/classes/sun/nio/ch/IOUtil.java +++ b/src/share/classes/sun/nio/ch/IOUtil.java @@ -44,11 +44,11 @@ public class IOUtil { private IOUtil() { } // No instantiation static int write(FileDescriptor fd, ByteBuffer src, long position, - NativeDispatcher nd, Object lock) + NativeDispatcher nd) throws IOException { if (src instanceof DirectBuffer) - return writeFromNativeBuffer(fd, src, position, nd, lock); + return writeFromNativeBuffer(fd, src, position, nd); // Substitute a native buffer int pos = src.position(); @@ -62,7 +62,7 @@ public class IOUtil { // Do not update src until we see how many bytes were written src.position(pos); - int n = writeFromNativeBuffer(fd, bb, position, nd, lock); + int n = writeFromNativeBuffer(fd, bb, position, nd); if (n > 0) { // now update src src.position(pos + n); @@ -74,8 +74,7 @@ public class IOUtil { } private static int writeFromNativeBuffer(FileDescriptor fd, ByteBuffer bb, - long position, NativeDispatcher nd, - Object lock) + long position, NativeDispatcher nd) throws IOException { int pos = bb.position(); @@ -89,7 +88,7 @@ public class IOUtil { if (position != -1) { written = nd.pwrite(fd, ((DirectBuffer)bb).address() + pos, - rem, position, lock); + rem, position); } else { written = nd.write(fd, ((DirectBuffer)bb).address() + pos, rem); } @@ -184,18 +183,18 @@ public class IOUtil { } static int read(FileDescriptor fd, ByteBuffer dst, long position, - NativeDispatcher nd, Object lock) + NativeDispatcher nd) throws IOException { if (dst.isReadOnly()) throw new IllegalArgumentException("Read-only buffer"); if (dst instanceof DirectBuffer) - return readIntoNativeBuffer(fd, dst, position, nd, lock); + return readIntoNativeBuffer(fd, dst, position, nd); // Substitute a native buffer ByteBuffer bb = Util.getTemporaryDirectBuffer(dst.remaining()); try { - int n = readIntoNativeBuffer(fd, bb, position, nd, lock); + int n = readIntoNativeBuffer(fd, bb, position, nd); bb.flip(); if (n > 0) dst.put(bb); @@ -206,8 +205,7 @@ public class IOUtil { } private static int readIntoNativeBuffer(FileDescriptor fd, ByteBuffer bb, - long position, NativeDispatcher nd, - Object lock) + long position, NativeDispatcher nd) throws IOException { int pos = bb.position(); @@ -220,7 +218,7 @@ public class IOUtil { int n = 0; if (position != -1) { n = nd.pread(fd, ((DirectBuffer)bb).address() + pos, - rem, position, lock); + rem, position); } else { n = nd.read(fd, ((DirectBuffer)bb).address() + pos, rem); } diff --git a/src/share/classes/sun/nio/ch/NativeDispatcher.java b/src/share/classes/sun/nio/ch/NativeDispatcher.java index 4752ce8e3..bcc507d54 100644 --- a/src/share/classes/sun/nio/ch/NativeDispatcher.java +++ b/src/share/classes/sun/nio/ch/NativeDispatcher.java @@ -38,8 +38,16 @@ abstract class NativeDispatcher abstract int read(FileDescriptor fd, long address, int len) throws IOException; - int pread(FileDescriptor fd, long address, int len, - long position, Object lock) throws IOException + /** + * Returns {@code true} if pread/pwrite needs to be synchronized with + * position sensitive methods. + */ + boolean needsPositionLock() { + return false; + } + + int pread(FileDescriptor fd, long address, int len, long position) + throws IOException { throw new IOException("Operation Unsupported"); } @@ -50,8 +58,8 @@ abstract class NativeDispatcher abstract int write(FileDescriptor fd, long address, int len) throws IOException; - int pwrite(FileDescriptor fd, long address, int len, - long position, Object lock) throws IOException + int pwrite(FileDescriptor fd, long address, int len, long position) + throws IOException { throw new IOException("Operation Unsupported"); } diff --git a/src/share/classes/sun/nio/ch/SimpleAsynchronousFileChannelImpl.java b/src/share/classes/sun/nio/ch/SimpleAsynchronousFileChannelImpl.java index ede531bd0..741abf733 100644 --- a/src/share/classes/sun/nio/ch/SimpleAsynchronousFileChannelImpl.java +++ b/src/share/classes/sun/nio/ch/SimpleAsynchronousFileChannelImpl.java @@ -318,7 +318,7 @@ public class SimpleAsynchronousFileChannelImpl try { begin(); do { - n = IOUtil.read(fdObj, dst, position, nd, null); + n = IOUtil.read(fdObj, dst, position, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); if (n < 0 && !isOpen()) throw new AsynchronousCloseException(); @@ -372,7 +372,7 @@ public class SimpleAsynchronousFileChannelImpl try { begin(); do { - n = IOUtil.write(fdObj, src, position, nd, null); + n = IOUtil.write(fdObj, src, position, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); if (n < 0 && !isOpen()) throw new AsynchronousCloseException(); diff --git a/src/share/classes/sun/nio/ch/SocketChannelImpl.java b/src/share/classes/sun/nio/ch/SocketChannelImpl.java index 9696038ec..22be8fb6e 100644 --- a/src/share/classes/sun/nio/ch/SocketChannelImpl.java +++ b/src/share/classes/sun/nio/ch/SocketChannelImpl.java @@ -356,7 +356,7 @@ class SocketChannelImpl // except that the shutdown operation plays the role of // nd.preClose(). for (;;) { - n = IOUtil.read(fd, buf, -1, nd, readLock); + n = IOUtil.read(fd, buf, -1, nd); if ((n == IOStatus.INTERRUPTED) && isOpen()) { // The system call was interrupted but the channel // is still open, so retry @@ -447,7 +447,7 @@ class SocketChannelImpl writerThread = NativeThread.current(); } for (;;) { - n = IOUtil.write(fd, buf, -1, nd, writeLock); + n = IOUtil.write(fd, buf, -1, nd); if ((n == IOStatus.INTERRUPTED) && isOpen()) continue; return IOStatus.normalize(n); diff --git a/src/solaris/classes/sun/nio/ch/FileDispatcherImpl.java b/src/solaris/classes/sun/nio/ch/FileDispatcherImpl.java index 79f238dc7..37c1d5b90 100644 --- a/src/solaris/classes/sun/nio/ch/FileDispatcherImpl.java +++ b/src/solaris/classes/sun/nio/ch/FileDispatcherImpl.java @@ -46,8 +46,9 @@ class FileDispatcherImpl extends FileDispatcher return read0(fd, address, len); } - int pread(FileDescriptor fd, long address, int len, - long position, Object lock) throws IOException { + int pread(FileDescriptor fd, long address, int len, long position) + throws IOException + { return pread0(fd, address, len, position); } @@ -59,8 +60,8 @@ class FileDispatcherImpl extends FileDispatcher return write0(fd, address, len); } - int pwrite(FileDescriptor fd, long address, int len, - long position, Object lock) throws IOException + int pwrite(FileDescriptor fd, long address, int len, long position) + throws IOException { return pwrite0(fd, address, len, position); } diff --git a/src/solaris/classes/sun/nio/ch/SinkChannelImpl.java b/src/solaris/classes/sun/nio/ch/SinkChannelImpl.java index e99c184f8..d388dd47f 100644 --- a/src/solaris/classes/sun/nio/ch/SinkChannelImpl.java +++ b/src/solaris/classes/sun/nio/ch/SinkChannelImpl.java @@ -165,7 +165,7 @@ class SinkChannelImpl return 0; thread = NativeThread.current(); do { - n = IOUtil.write(fd, src, -1, nd, lock); + n = IOUtil.write(fd, src, -1, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); return IOStatus.normalize(n); } finally { diff --git a/src/solaris/classes/sun/nio/ch/SourceChannelImpl.java b/src/solaris/classes/sun/nio/ch/SourceChannelImpl.java index b6b005ae3..cc374127e 100644 --- a/src/solaris/classes/sun/nio/ch/SourceChannelImpl.java +++ b/src/solaris/classes/sun/nio/ch/SourceChannelImpl.java @@ -165,7 +165,7 @@ class SourceChannelImpl return 0; thread = NativeThread.current(); do { - n = IOUtil.read(fd, dst, -1, nd, lock); + n = IOUtil.read(fd, dst, -1, nd); } while ((n == IOStatus.INTERRUPTED) && isOpen()); return IOStatus.normalize(n); } finally { diff --git a/src/solaris/classes/sun/nio/ch/UnixAsynchronousSocketChannelImpl.java b/src/solaris/classes/sun/nio/ch/UnixAsynchronousSocketChannelImpl.java index d7fccc9d5..33859f035 100644 --- a/src/solaris/classes/sun/nio/ch/UnixAsynchronousSocketChannelImpl.java +++ b/src/solaris/classes/sun/nio/ch/UnixAsynchronousSocketChannelImpl.java @@ -384,7 +384,7 @@ class UnixAsynchronousSocketChannelImpl if (scattering) { n = (int)IOUtil.read(fd, readBuffers, nd); } else { - n = IOUtil.read(fd, readBuffer, -1, nd, null); + n = IOUtil.read(fd, readBuffer, -1, nd); } if (n == IOStatus.UNAVAILABLE) { // spurious wakeup, is this possible? @@ -505,7 +505,7 @@ class UnixAsynchronousSocketChannelImpl if (isScatteringRead) { n = (int)IOUtil.read(fd, dsts, nd); } else { - n = IOUtil.read(fd, dst, -1, nd, null); + n = IOUtil.read(fd, dst, -1, nd); } } @@ -579,7 +579,7 @@ class UnixAsynchronousSocketChannelImpl if (gathering) { n = (int)IOUtil.write(fd, writeBuffers, nd); } else { - n = IOUtil.write(fd, writeBuffer, -1, nd, null); + n = IOUtil.write(fd, writeBuffer, -1, nd); } if (n == IOStatus.UNAVAILABLE) { // spurious wakeup, is this possible? @@ -688,7 +688,7 @@ class UnixAsynchronousSocketChannelImpl if (isGatheringWrite) { n = (int)IOUtil.write(fd, srcs, nd); } else { - n = IOUtil.write(fd, src, -1, nd, null); + n = IOUtil.write(fd, src, -1, nd); } } diff --git a/src/windows/classes/sun/nio/ch/FileDispatcherImpl.java b/src/windows/classes/sun/nio/ch/FileDispatcherImpl.java index 276c6199a..de3c4f9f7 100644 --- a/src/windows/classes/sun/nio/ch/FileDispatcherImpl.java +++ b/src/windows/classes/sun/nio/ch/FileDispatcherImpl.java @@ -49,18 +49,21 @@ class FileDispatcherImpl extends FileDispatcher this(false); } + @Override + boolean needsPositionLock() { + return true; + } + int read(FileDescriptor fd, long address, int len) throws IOException { return read0(fd, address, len); } - int pread(FileDescriptor fd, long address, int len, - long position, Object lock) throws IOException + int pread(FileDescriptor fd, long address, int len, long position) + throws IOException { - synchronized(lock) { - return pread0(fd, address, len, position); - } + return pread0(fd, address, len, position); } long readv(FileDescriptor fd, long address, int len) throws IOException { @@ -71,12 +74,10 @@ class FileDispatcherImpl extends FileDispatcher return write0(fd, address, len, append); } - int pwrite(FileDescriptor fd, long address, int len, - long position, Object lock) throws IOException + int pwrite(FileDescriptor fd, long address, int len, long position) + throws IOException { - synchronized(lock) { - return pwrite0(fd, address, len, position); - } + return pwrite0(fd, address, len, position); } long writev(FileDescriptor fd, long address, int len) throws IOException { diff --git a/test/java/nio/channels/FileChannel/InterruptDeadlock.java b/test/java/nio/channels/FileChannel/InterruptDeadlock.java new file mode 100644 index 000000000..102d7db90 --- /dev/null +++ b/test/java/nio/channels/FileChannel/InterruptDeadlock.java @@ -0,0 +1,137 @@ +/* + * Copyright (c) 2013, Oracle and/or its affiliates. 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 Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* @test + * @bug 8012019 + * @summary Tests interruption of threads doing position-based read methods in + * an attempt to provoke a deadlock between position sensitive and position + * insensitive methods + */ +import java.nio.ByteBuffer; +import java.nio.channels.*; +import java.nio.file.*; +import static java.nio.file.StandardOpenOption.*; + +public class InterruptDeadlock { + + /** + * A thread that continuously reads from a FileChannel with + * read(ByteBuffer,long). The thread terminates when interrupted and/or + * the FileChannel is closed. + */ + static class Reader extends Thread { + final FileChannel fc; + volatile Exception exception; + + Reader(FileChannel fc) { + this.fc = fc; + } + + @Override + public void run() { + ByteBuffer bb = ByteBuffer.allocate(1024); + try { + long pos = 0L; + for (;;) { + bb.clear(); + int n = fc.read(bb, pos); + if (n > 0) + pos += n; + // fc.size is important here as it is position sensitive + if (pos > fc.size()) + pos = 0L; + } + } catch (ClosedChannelException x) { + System.out.println(x.getClass() + " (expected)"); + } catch (Exception unexpected) { + this.exception = unexpected; + } + } + + Exception exception() { + return exception; + } + + static Reader startReader(FileChannel fc) { + Reader r = new Reader(fc); + r.start(); + return r; + } + } + + // the number of reader threads to start + private static final int READER_COUNT = 4; + + public static void main(String[] args) throws Exception { + Path file = Paths.get("data.txt"); + try (FileChannel fc = FileChannel.open(file, CREATE, TRUNCATE_EXISTING, WRITE)) { + fc.position(1024L * 1024L); + fc.write(ByteBuffer.wrap(new byte[1])); + } + + Reader[] readers = new Reader[READER_COUNT]; + + for (int i=1; i<=20; i++) { + System.out.format("Iteration: %s%n", i); + + try (FileChannel fc = FileChannel.open(file)) { + boolean failed = false; + + // start reader threads + for (int j=0; j