8212132: (dc) Remove DatagramChannelImpl finalize method
authoralanb
Sun, 27 Oct 2019 12:13:51 +0000
changeset 58808 9261ad32cba9
parent 58805 d3382812b788
child 58809 44dc3d796110
8212132: (dc) Remove DatagramChannelImpl finalize method Reviewed-by: bpb, chegar, dfuchs, martin
src/java.base/share/classes/sun/nio/ch/DatagramChannelImpl.java
src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java
test/jdk/java/nio/channels/DatagramChannel/Unref.java
--- a/src/java.base/share/classes/sun/nio/ch/DatagramChannelImpl.java	Fri Oct 25 13:17:31 2019 -0700
+++ b/src/java.base/share/classes/sun/nio/ch/DatagramChannelImpl.java	Sun Oct 27 12:13:51 2019 +0000
@@ -27,6 +27,8 @@
 
 import java.io.FileDescriptor;
 import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.ref.Cleaner.Cleanable;
 import java.net.DatagramSocket;
 import java.net.Inet4Address;
 import java.net.Inet6Address;
@@ -55,6 +57,7 @@
 import java.util.Set;
 import java.util.concurrent.locks.ReentrantLock;
 
+import jdk.internal.ref.CleanerFactory;
 import sun.net.ResourceManager;
 import sun.net.ext.ExtendedSocketOptions;
 import sun.net.util.IPAddressUtil;
@@ -68,7 +71,7 @@
     implements SelChImpl
 {
     // Used to make native read and write calls
-    private static NativeDispatcher nd = new DatagramDispatcher();
+    private static final NativeDispatcher nd = new DatagramDispatcher();
 
     // The protocol family of the socket
     private final ProtocolFamily family;
@@ -76,6 +79,7 @@
     // Our file descriptor
     private final FileDescriptor fd;
     private final int fdVal;
+    private final Cleanable cleaner;
 
     // Cached InetAddress and port for unconnected DatagramChannels
     // used by receive0
@@ -138,6 +142,7 @@
             ResourceManager.afterUdpClose();
             throw ioe;
         }
+        this.cleaner = CleanerFactory.cleaner().register(this, closerFor(fd));
     }
 
     public DatagramChannelImpl(SelectorProvider sp, ProtocolFamily family)
@@ -164,6 +169,7 @@
             ResourceManager.afterUdpClose();
             throw ioe;
         }
+        this.cleaner = CleanerFactory.cleaner().register(this, closerFor(fd));
     }
 
     public DatagramChannelImpl(SelectorProvider sp, FileDescriptor fd)
@@ -179,6 +185,7 @@
                 : StandardProtocolFamily.INET;
         this.fd = fd;
         this.fdVal = IOUtil.fdVal(fd);
+        this.cleaner = CleanerFactory.cleaner().register(this, closerFor(fd));
         synchronized (stateLock) {
             this.localAddress = Net.localAddress(fd);
         }
@@ -1181,10 +1188,10 @@
         if ((readerThread == 0) && (writerThread == 0) && !isRegistered()) {
             state = ST_CLOSED;
             try {
-                nd.close(fd);
-            } finally {
-                // notify resource manager
-                ResourceManager.afterUdpClose();
+                // close socket
+                cleaner.clean();
+            } catch (UncheckedIOException ioe) {
+                throw ioe.getCause();
             }
             return true;
         } else {
@@ -1283,13 +1290,6 @@
         }
     }
 
-    @SuppressWarnings("deprecation")
-    protected void finalize() throws IOException {
-        // fd is null if constructor threw exception
-        if (fd != null)
-            close();
-    }
-
     /**
      * Translates native poll revent set into a ready operation set
      */
@@ -1377,6 +1377,21 @@
         return fdVal;
     }
 
+    /**
+     * Returns an action to close the given file descriptor.
+     */
+    private static Runnable closerFor(FileDescriptor fd) {
+        return () -> {
+            try {
+                nd.close(fd);
+            } catch (IOException ioe) {
+                throw new UncheckedIOException(ioe);
+            } finally {
+                // decrement
+                ResourceManager.afterUdpClose();
+            }
+        };
+    }
 
     // -- Native methods --
 
--- a/src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java	Fri Oct 25 13:17:31 2019 -0700
+++ b/src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java	Sun Oct 27 12:13:51 2019 +0000
@@ -30,8 +30,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.UncheckedIOException;
-import java.lang.invoke.MethodHandles;
-import java.lang.invoke.VarHandle;
+import java.lang.ref.Cleaner.Cleanable;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.ProtocolFamily;
@@ -106,7 +105,7 @@
 
     // set by SocketImpl.create, protected by stateLock
     private boolean stream;
-    private FileDescriptorCloser closer;
+    private Cleanable cleaner;
 
     // set to true when the socket is in non-blocking mode
     private volatile boolean nonBlocking;
@@ -471,9 +470,10 @@
                     ResourceManager.afterUdpClose();
                 throw ioe;
             }
+            Runnable closer = closerFor(fd, stream);
             this.fd = fd;
             this.stream = stream;
-            this.closer = FileDescriptorCloser.create(this);
+            this.cleaner = CleanerFactory.cleaner().register(this, closer);
             this.state = ST_UNCONNECTED;
         }
     }
@@ -777,10 +777,11 @@
         }
 
         // set the fields
+        Runnable closer = closerFor(newfd, true);
         synchronized (nsi.stateLock) {
             nsi.fd = newfd;
             nsi.stream = true;
-            nsi.closer = FileDescriptorCloser.create(nsi);
+            nsi.cleaner = CleanerFactory.cleaner().register(nsi, closer);
             nsi.localport = localAddress.getPort();
             nsi.address = isaa[0].getAddress();
             nsi.port = isaa[0].getPort();
@@ -850,7 +851,7 @@
         assert Thread.holdsLock(stateLock) && state == ST_CLOSING;
         if (readerThread == 0 && writerThread == 0) {
             try {
-                closer.run();
+                cleaner.clean();
             } catch (UncheckedIOException ioe) {
                 throw ioe.getCause();
             } finally {
@@ -1193,53 +1194,28 @@
     }
 
     /**
-     * A task that closes a SocketImpl's file descriptor. The task runs when the
-     * SocketImpl is explicitly closed and when the SocketImpl becomes phantom
-     * reachable.
+     * Returns an action to close the given file descriptor.
      */
-    private static class FileDescriptorCloser implements Runnable {
-        private static final VarHandle CLOSED;
-        static {
-            try {
-                MethodHandles.Lookup l = MethodHandles.lookup();
-                CLOSED = l.findVarHandle(FileDescriptorCloser.class,
-                                         "closed",
-                                         boolean.class);
-            } catch (Exception e) {
-                throw new InternalError(e);
-            }
-        }
-
-        private final FileDescriptor fd;
-        private final boolean stream;
-        private volatile boolean closed;
-
-        FileDescriptorCloser(FileDescriptor fd, boolean stream) {
-            this.fd = fd;
-            this.stream = stream;
-        }
-
-        static FileDescriptorCloser create(NioSocketImpl impl) {
-            assert Thread.holdsLock(impl.stateLock);
-            var closer = new FileDescriptorCloser(impl.fd, impl.stream);
-            CleanerFactory.cleaner().register(impl, closer);
-            return closer;
-        }
-
-        @Override
-        public void run() {
-            if (CLOSED.compareAndSet(this, false, true)) {
+    private static Runnable closerFor(FileDescriptor fd, boolean stream) {
+        if (stream) {
+            return () -> {
+                try {
+                    nd.close(fd);
+                } catch (IOException ioe) {
+                    throw new UncheckedIOException(ioe);
+                }
+            };
+        } else {
+            return () -> {
                 try {
                     nd.close(fd);
                 } catch (IOException ioe) {
                     throw new UncheckedIOException(ioe);
                 } finally {
-                    if (!stream) {
-                        // decrement
-                        ResourceManager.afterUdpClose();
-                    }
+                    // decrement
+                    ResourceManager.afterUdpClose();
                 }
-            }
+            };
         }
     }
 
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/nio/channels/DatagramChannel/Unref.java	Sun Oct 27 12:13:51 2019 +0000
@@ -0,0 +1,170 @@
+/*
+ * Copyright (c) 2019, 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 8212132
+ * @summary Test that DatagramChannel does not leak file descriptors
+ * @requires os.family != "windows"
+ * @modules jdk.management
+ * @library /test/lib
+ * @run main/othervm Unref
+ */
+
+import java.io.IOException;
+import java.lang.management.ManagementFactory;
+import java.net.InetSocketAddress;
+import java.net.StandardProtocolFamily;
+import java.nio.channels.DatagramChannel;
+import java.nio.channels.SelectionKey;
+import java.nio.channels.Selector;
+
+import com.sun.management.UnixOperatingSystemMXBean;
+
+import jtreg.SkippedException;
+import jdk.test.lib.net.IPSupport;
+
+public class Unref {
+
+    interface DatagramChannelSupplier {
+        DatagramChannel get() throws IOException;
+    }
+
+    public static void main(String[] args) throws Exception {
+        if (unixOperatingSystemMXBean() == null)
+            throw new SkippedException("This test requires UnixOperatingSystemMXBean");
+
+        test(DatagramChannel::open);
+        if (IPSupport.hasIPv4())
+            test(() -> DatagramChannel.open(StandardProtocolFamily.INET));
+        if (IPSupport.hasIPv6())
+            test(() -> DatagramChannel.open(StandardProtocolFamily.INET6));
+    }
+
+    static void test(DatagramChannelSupplier supplier) throws Exception {
+        openAndClose(supplier); // warm-up
+
+        try (Selector sel = Selector.open()) {
+            long count = fileDescriptorCount();
+
+            // open+close
+            openAndClose(supplier);
+            assertEquals(fileDescriptorCount(), count);
+
+            // open+unref, file descriptor should be closed by cleaner
+            openAndUnref(supplier);
+            assertEquals(waitForFileDescriptorCount(count), count);
+
+            // open+register+close+flush
+            openRegisterAndClose(supplier, sel);
+            assertEquals(fileDescriptorCount(), count);
+
+            // open+register+flush, file descriptor should be closed by cleaner
+            openRegisterAndUnref(supplier, sel);
+            assertEquals(waitForFileDescriptorCount(count), count);
+        }
+    }
+
+    /**
+     * Create a DatagramChannel and closes it.
+     */
+    static void openAndClose(DatagramChannelSupplier supplier) throws IOException {
+        System.out.println("openAndClose ...");
+        DatagramChannel dc = supplier.get();
+        dc.close();
+    }
+
+    /**
+     * Create a DatagramChannel and exits without closing the channel.
+     */
+    static void openAndUnref(DatagramChannelSupplier supplier) throws IOException {
+        System.out.println("openAndUnref ...");
+        DatagramChannel dc = supplier.get();
+    }
+
+    /**
+     * Create a DatagramChannel, register it with a Selector, close the channel
+     * while register, and then finally flush the channel from the Selector.
+     */
+    static void openRegisterAndClose(DatagramChannelSupplier supplier, Selector sel)
+        throws IOException
+    {
+        System.out.println("openRegisterAndClose ...");
+        try (DatagramChannel dc = supplier.get()) {
+            dc.bind(new InetSocketAddress(0));
+            dc.configureBlocking(false);
+            dc.register(sel, SelectionKey.OP_READ);
+            sel.selectNow();
+        }
+
+        // flush, should close channel
+        sel.selectNow();
+    }
+
+    /**
+     * Creates a DatagramChannel, registers with a Selector, cancels the key
+     * and flushes the channel from the Selector. This method exits without
+     * closing the channel.
+     */
+    static void openRegisterAndUnref(DatagramChannelSupplier supplier, Selector sel)
+        throws IOException
+    {
+        System.out.println("openRegisterAndUnref ...");
+        DatagramChannel dc = supplier.get();
+        dc.bind(new InetSocketAddress(0));
+        dc.configureBlocking(false);
+        SelectionKey key = dc.register(sel, SelectionKey.OP_READ);
+        sel.selectNow();
+        key.cancel();
+        sel.selectNow();
+    }
+
+    /**
+     * If the file descriptor count is higher than the given count then invoke
+     * System.gc() and wait for the file descriptor count to drop.
+     */
+    static long waitForFileDescriptorCount(long target) throws InterruptedException {
+        long actual = fileDescriptorCount();
+        if (actual > target) {
+            System.gc();
+            while ((actual = fileDescriptorCount()) > target) {
+                Thread.sleep(10);
+            }
+        }
+        return actual;
+    }
+
+    static UnixOperatingSystemMXBean unixOperatingSystemMXBean() {
+        return ManagementFactory.getPlatformMXBean(UnixOperatingSystemMXBean.class);
+    }
+
+    static long fileDescriptorCount() {
+        return unixOperatingSystemMXBean().getOpenFileDescriptorCount();
+    }
+
+    static void assertEquals(long actual, long expected) {
+        if (actual != expected)
+            throw new RuntimeException("actual=" + actual + ", expected=" + expected);
+    }
+}
+