More NioSocketImpl.read/write and ServerSocket.implAccept cleanup niosocketimpl-branch
authoralanb
Tue, 26 Feb 2019 08:53:16 +0000
branchniosocketimpl-branch
changeset 57211 4503441bec2e
parent 57210 a67ea4f53e56
child 57212 28b0946d3b81
More NioSocketImpl.read/write and ServerSocket.implAccept cleanup
src/java.base/share/classes/java/net/ServerSocket.java
src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java
--- a/src/java.base/share/classes/java/net/ServerSocket.java	Sun Feb 24 07:59:46 2019 +0000
+++ b/src/java.base/share/classes/java/net/ServerSocket.java	Tue Feb 26 08:53:16 2019 +0000
@@ -541,52 +541,32 @@
      * @spec JSR-51
      */
     protected final void implAccept(Socket s) throws IOException {
-        SocketImpl impl = getImpl();
         SocketImpl si = s.impl;
 
-        // Socket does not have a SocketImpl
+        // Socket has no SocketImpl
         if (si == null) {
             // create a SocketImpl and accept the connection
             si = Socket.createImpl();
-            assert !(si instanceof DelegatingSocketImpl);
-            impl.accept(si);
-            try {
-                // a custom impl has accepted the connection with a platform SocketImpl
-                if (!(impl instanceof PlatformSocketImpl) && (si instanceof PlatformSocketImpl)) {
-                    ((PlatformSocketImpl) si).postCustomAccept();
-                }
-            } finally {
-                securityCheckAccept(si);  // closes si if permission check fails
-            }
-
+            implAccept(si);
             // bind Socket to the SocketImpl and update socket state
             s.setImpl(si);
             s.postAccept();
             return;
         }
 
-        // Socket has a SOCKS or HTTP SocketImpl
+        // Socket has a SOCKS or HTTP SocketImpl, need delegate
         if (si instanceof DelegatingSocketImpl) {
             si = ((DelegatingSocketImpl) si).delegate();
             assert si instanceof PlatformSocketImpl;
         }
 
-        // ServerSocket or Socket (or both) have a platform SocketImpl
+        // ServerSocket or Socket (or both) have, or delegate to, a platform SocketImpl
         if (impl instanceof PlatformSocketImpl || si instanceof PlatformSocketImpl) {
             // create a new platform SocketImpl and accept the connection
-            var nsi = SocketImpl.createPlatformSocketImpl(false);
-            impl.accept(nsi);
-            try {
-                // a custom impl has accepted the connection
-                if (!(impl instanceof PlatformSocketImpl)) {
-                    nsi.postCustomAccept();
-                }
-            } finally {
-                securityCheckAccept(nsi);  // closes nsi if permission check fails
-            }
-
-            // copy state to the existing SocketImpl and update socket state
-            nsi.copyTo(si);
+            var psi = SocketImpl.createPlatformSocketImpl(false);
+            implAccept(psi);
+            // copy connection/state to the existing SocketImpl and update socket state
+            psi.copyTo(si);
             s.postAccept();
             return;
         }
@@ -610,6 +590,26 @@
     }
 
     /**
+     * Accepts a new connection so that the given SocketImpl is connected to
+     * the peer. The SocketImpl and connection are closed if the connection is
+     * denied by the security manager.
+     * @throws IOException if an I/O error occurs
+     * @throws SecurityException if the security manager's checkAccept method fails
+     */
+    private void implAccept(SocketImpl si) throws IOException {
+        assert !(si instanceof DelegatingSocketImpl);
+        impl.accept(si);
+        try {
+            // a custom impl has accepted the connection with a platform SocketImpl
+            if (!(impl instanceof PlatformSocketImpl) && (si instanceof PlatformSocketImpl)) {
+                ((PlatformSocketImpl) si).postCustomAccept();
+            }
+        } finally {
+            securityCheckAccept(si);  // closes si if permission check fails
+        }
+    }
+
+    /**
      * Invokes the security manager's checkAccept method. If the permission
      * check fails then it closes the SocketImpl.
      */
--- a/src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java	Sun Feb 24 07:59:46 2019 +0000
+++ b/src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java	Tue Feb 26 08:53:16 2019 +0000
@@ -115,10 +115,10 @@
     private long readerThread;
     private long writerThread;
 
-    // used when SO_REUSEADDR is emulated
+    // used when SO_REUSEADDR is emulated, protected by stateLock
     private boolean isReuseAddress;
 
-    // cached value of IPV6_TCLASS or IP_TOS socket option
+    // cached value of IPV6_TCLASS or IP_TOS socket option, protected by stateLock
     private int trafficClass;
 
     // read or accept timeout in millis
@@ -128,6 +128,10 @@
     private volatile boolean isInputClosed;
     private volatile boolean isOutputClosed;
 
+    // used by read to emulate legacy behavior, protected by readLock
+    private boolean readEOF;
+    private boolean connectionReset;
+
     /**
      * Creates a instance of this SocketImpl.
      * @param server true if this is a SocketImpl for a ServerSocket
@@ -254,6 +258,7 @@
     private int timedRead(FileDescriptor fd, byte[] b, int off, int len, int millis)
         throws IOException
     {
+        assert nonBlocking;
         long nanos = NANOSECONDS.convert(millis, TimeUnit.MILLISECONDS);
         long remainingNanos = nanos;
         long startNanos = System.nanoTime();
@@ -273,18 +278,23 @@
 
     /**
      * Reads bytes from the socket into the given byte array.
-     * @return the number of bytes read
-     * @throws IOException if the socket is closed or an I/O occurs
+     * @return the number of bytes read or -1 at EOF
+     * @throws SocketException if the socket is closed or a socket I/O error occurs
      * @throws SocketTimeoutException if the read timeout elapses
      */
-    private int read(byte[] b, int off, int len) throws IOException {
+    private int implRead(byte[] b, int off, int len) throws IOException {
         readLock.lock();
         try {
+            // emulate legacy behavior to return -1, even if socket is closed
+            if (readEOF)
+                return -1;
             int n = 0;
             FileDescriptor fd = beginRead();
             try {
+                if (connectionReset)
+                    throw new SocketException("Connection reset");
                 if (isInputClosed)
-                    return IOStatus.EOF;
+                    return -1;
                 int timeout = this.timeout;
                 configureNonBlockingIfNeeded(fd, timeout);
                 n = tryRead(fd, b, off, len);
@@ -300,7 +310,16 @@
                         } while (IOStatus.okayToRetry(n) && isOpen());
                     }
                 }
+                if (n == -1)
+                    readEOF = true;
                 return n;
+            } catch (SocketTimeoutException e) {
+                throw e;
+            } catch (ConnectionResetException e) {
+                connectionReset = true;
+                throw new SocketException("Connection reset");
+            } catch (IOException ioe) {
+                throw new SocketException(ioe.getMessage());
             } finally {
                 endRead(n > 0);
             }
@@ -310,6 +329,24 @@
     }
 
     /**
+     * Reads bytes from the socket into the given byte array.
+     * @return the number of bytes read or -1 at EOF
+     * @throws IndexOutOfBoundsException if the bound checks fail
+     * @throws SocketException if the socket is closed or a socket I/O error occurs
+     * @throws SocketTimeoutException if the read timeout elapses
+     */
+    private int read(byte[] b, int off, int len) throws IOException {
+        Objects.checkFromIndexSize(off, len, b.length);
+        if (len == 0) {
+            return 0;
+        } else {
+            // read up to MAX_BUFFER_SIZE bytes
+            int size = Math.min(len, MAX_BUFFER_SIZE);
+            return implRead(b, off, size);
+        }
+    }
+
+    /**
      * Marks the beginning of a write operation that might block.
      * @throws SocketException if the socket is closed or not connected
      */
@@ -355,9 +392,9 @@
     /**
      * Writes a sequence of bytes to this socket from the given byte array.
      * @return the number of bytes written
-     * @throws IOException if the socket is closed or an I/O occurs
+     * @throws SocketException if the socket is closed or an socket I/O error occurs
      */
-    private int write(byte[] b, int off, int len) throws IOException {
+    private int implWrite(byte[] b, int off, int len) throws IOException {
         writeLock.lock();
         try {
             int n = 0;
@@ -369,6 +406,8 @@
                     n = tryWrite(fd, b, off, len);
                 }
                 return n;
+            } catch (IOException ioe) {
+                throw new SocketException(ioe.getMessage());
             } finally {
                 endWrite(n > 0);
             }
@@ -378,6 +417,24 @@
     }
 
     /**
+     * Writes a sequence of bytes to this socket from the given byte array.
+     * @throws SocketException if the socket is closed or an socket I/O error occurs
+     */
+    private void write(byte[] b, int off, int len) throws IOException {
+        Objects.checkFromIndexSize(off, len, b.length);
+        if (len > 0) {
+            int pos = off;
+            int end = off + len;
+            while (pos < end) {
+                // write up to MAX_BUFFER_SIZE bytes
+                int size = Math.min((end - pos), MAX_BUFFER_SIZE);
+                int n = implWrite(b, pos, size);
+                pos += n;
+            }
+        }
+    }
+
+    /**
      * Creates the socket.
      * @param stream {@code true} for a streams socket
      */
@@ -440,6 +497,7 @@
                     nsi.close();
                 } catch (IOException ignore) { }
             }
+            // copy/reset fields protected by stateLock
             synchronized (nsi.stateLock) {
                 assert nsi.state == ST_NEW || nsi.state == ST_CLOSED;
                 synchronized (this.stateLock) {
@@ -456,9 +514,9 @@
 
                     // reset fields; do not reset timeout
                     nsi.nonBlocking = false;
+                    nsi.isReuseAddress = false;
                     nsi.isInputClosed = false;
                     nsi.isOutputClosed = false;
-                    nsi.isReuseAddress = false;
                     nsi.state = ST_CONNECTED;
 
                     // GC'ing of this impl should not close the file descriptor
@@ -469,6 +527,14 @@
                     nsi.closer = FileDescriptorCloser.create(nsi);
                 }
             }
+            // reset fields protected by readLock
+            nsi.readLock.lock();
+            try {
+                nsi.readEOF = false;
+                nsi.connectionReset = false;
+            } finally {
+                nsi.readLock.unlock();
+            }
         } else {
             synchronized (this.stateLock) {
                 // this SocketImpl should be connected
@@ -696,6 +762,7 @@
                             int millis)
         throws IOException
     {
+        assert nonBlocking;
         long nanos = NANOSECONDS.convert(millis, TimeUnit.MILLISECONDS);
         long remainingNanos = nanos;
         long startNanos = System.nanoTime();
@@ -783,8 +850,6 @@
     @Override
     protected InputStream getInputStream() {
         return new InputStream() {
-            // EOF or connection reset detected, not thread safe
-            private boolean eof, reset;
             @Override
             public int read() throws IOException {
                 byte[] a = new byte[1];
@@ -793,31 +858,7 @@
             }
             @Override
             public int read(byte[] b, int off, int len) throws IOException {
-                Objects.checkFromIndexSize(off, len, b.length);
-                if (eof) {
-                    return -1;
-                } else if (reset) {
-                    NioSocketImpl.this.ensureOpen();
-                    throw new SocketException("Connection reset");
-                } else if (len == 0) {
-                    return 0;
-                } else {
-                    try {
-                        // read up to MAX_BUFFER_SIZE bytes
-                        int size = Math.min(len, MAX_BUFFER_SIZE);
-                        int n = NioSocketImpl.this.read(b, off, size);
-                        if (n == -1)
-                            eof = true;
-                        return n;
-                    } catch (ConnectionResetException e) {
-                        reset = true;
-                        throw new SocketException("Connection reset");
-                    } catch (SocketTimeoutException e) {
-                        throw e;
-                    } catch (IOException ioe) {
-                        throw new SocketException(ioe.getMessage());
-                    }
-                }
+                return NioSocketImpl.this.read(b, off, len);
             }
             @Override
             public int available() throws IOException {
@@ -840,23 +881,8 @@
             }
             @Override
             public void write(byte[] b, int off, int len) throws IOException {
-                Objects.checkFromIndexSize(off, len, b.length);
-                if (len > 0) {
-                    try {
-                        int pos = off;
-                        int end = off + len;
-                        while (pos < end) {
-                            // write up to MAX_BUFFER_SIZE bytes
-                            int size = Math.min((end - pos), MAX_BUFFER_SIZE);
-                            int n = NioSocketImpl.this.write(b, pos, size);
-                            pos += n;
-                        }
-                    } catch (IOException ioe) {
-                        throw new SocketException(ioe.getMessage());
-                    }
-                }
+                NioSocketImpl.this.write(b, off, len);
             }
-
             @Override
             public void close() throws IOException {
                 NioSocketImpl.this.close();