8288746: HttpClient resources could be reclaimed more eagerly

src/java.base/share/classes/module-info.java

@@ -226,6 +226,7 @@
         jdk.jfr;
     exports jdk.internal.ref to
         java.desktop,
+        java.net.http,
         jdk.incubator.foreign;
     exports jdk.internal.reflect to
         java.logging,

src/java.base/share/lib/security/default.policy

@@ -19,6 +19,7 @@ grant codeBase "jrt:/java.net.http" {
     permission java.lang.RuntimePermission "accessClassInPackage.sun.net.util";
     permission java.lang.RuntimePermission "accessClassInPackage.sun.net.www";
     permission java.lang.RuntimePermission "accessClassInPackage.jdk.internal.misc";
+    permission java.lang.RuntimePermission "accessClassInPackage.jdk.internal.ref";
     permission java.lang.RuntimePermission "modifyThread";
     permission java.net.SocketPermission "*","connect,resolve";
     permission java.net.URLPermission "http:*","*:*";

src/java.net.http/share/classes/jdk/internal/net/http/HttpClientFacade.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2022, 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
@@ -26,6 +26,7 @@
 package jdk.internal.net.http;
 
 import java.io.IOException;
+import java.lang.ref.Cleaner;
 import java.lang.ref.Reference;
 import java.net.Authenticator;
 import java.net.CookieHandler;
@@ -44,20 +45,29 @@
 import java.net.http.WebSocket;
 import jdk.internal.net.http.common.OperationTrackers.Trackable;
 import jdk.internal.net.http.common.OperationTrackers.Tracker;
+import jdk.internal.ref.CleanerFactory;
 
 /**
  * An HttpClientFacade is a simple class that wraps an HttpClient implementation
  * and delegates everything to its implementation delegate.
+ * @implSpec
+ * Though the facade strongly reference its implementation, the
+ * implementation MUST NOT strongly reference the facade.
+ * It MAY use weak references if needed.
  */
 public final class HttpClientFacade extends HttpClient implements Trackable {
 
+    static final Cleaner cleaner = CleanerFactory.cleaner();
+
     final HttpClientImpl impl;
 
     /**
      * Creates an HttpClientFacade.
      */
     HttpClientFacade(HttpClientImpl impl) {
         this.impl = impl;
+        // wakeup the impl when the facade is gc'ed
+        cleaner.register(this, impl::facadeCleanup);
     }
 
     @Override // for tests

src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java

@@ -491,6 +491,13 @@ private HttpClientImpl(HttpClientBuilderImpl builder,
         assert facadeRef.get() != null;
     }
 
+    // called when the facade is GC'ed.
+    // Just wakes up the selector to cleanup...
+    void facadeCleanup() {
+        SelectorManager selmgr = this.selmgr;
+        if (selmgr != null) selmgr.wakeupSelector();
+    }
+
     void onSubmitFailure(Runnable command, Throwable failure) {
         selmgr.abort(failure);
     }
@@ -723,7 +730,7 @@ public long getOutstandingWebSocketOperations() {
         }
         @Override
         public boolean isFacadeReferenced() {
-            return reference.get() != null;
+            return !reference.refersTo(null);
         }
         @Override
         public boolean isSelectorAlive() { return isAlive.get(); }
@@ -749,8 +756,7 @@ public Tracker getOperationsTracker() {
     // Called by the SelectorManager thread to figure out whether it's time
     // to terminate.
     boolean isReferenced() {
-        HttpClient facade = facade();
-        return facade != null || referenceCount() > 0;
+        return !facadeRef.refersTo(null) || referenceCount() > 0;
     }
 
     /**

test/jdk/java/net/httpclient/AsFileDownloadTest.java

@@ -29,6 +29,8 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.UncheckedIOException;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.URI;
@@ -77,7 +79,6 @@
  * @run testng/othervm AsFileDownloadTest
  * @run testng/othervm/java.security.policy=AsFileDownloadTest.policy AsFileDownloadTest
  */
-
 public class AsFileDownloadTest {
 
     SSLContext sslContext;
@@ -89,6 +90,7 @@ public class AsFileDownloadTest {
     String httpsURI;
     String http2URI;
     String https2URI;
+    final ReferenceTracker TRACKER = ReferenceTracker.INSTANCE;
 
     Path tempDir;
 
@@ -164,37 +166,49 @@ void test(String uriString, String contentDispositionValue, String expectedFilen
     {
         out.printf("test(%s, %s, %s): starting", uriString, contentDispositionValue, expectedFilename);
         HttpClient client = HttpClient.newBuilder().sslContext(sslContext).build();
+        TRACKER.track(client);
+        ReferenceQueue<HttpClient> queue = new ReferenceQueue<>();
+        WeakReference<HttpClient> ref = new WeakReference<>(client, queue);
+        try {
+            URI uri = URI.create(uriString);
+            HttpRequest request = HttpRequest.newBuilder(uri)
+                    .POST(BodyPublishers.ofString("May the luck of the Irish be with you!"))
+                    .build();
 
-        URI uri = URI.create(uriString);
-        HttpRequest request = HttpRequest.newBuilder(uri)
-                .POST(BodyPublishers.ofString("May the luck of the Irish be with you!"))
-                .build();
-
-        BodyHandler bh = ofFileDownload(tempDir.resolve(uri.getPath().substring(1)),
-                                        CREATE, TRUNCATE_EXISTING, WRITE);
-        HttpResponse<Path> response = client.send(request, bh);
-
-        Path body = response.body();
-        out.println("Got response: " + response);
-        out.println("Got body Path: " + body);
-        String fileContents = new String(Files.readAllBytes(response.body()), UTF_8);
-        out.println("Got body: " + fileContents);
-
-        assertEquals(response.statusCode(),200);
-        assertEquals(body.getFileName().toString(), expectedFilename);
-        assertTrue(response.headers().firstValue("Content-Disposition").isPresent());
-        assertEquals(response.headers().firstValue("Content-Disposition").get(),
-                     contentDispositionValue);
-        assertEquals(fileContents, "May the luck of the Irish be with you!");
-
-        if (!body.toAbsolutePath().startsWith(tempDir.toAbsolutePath())) {
-            System.out.println("Tempdir = " + tempDir.toAbsolutePath());
-            System.out.println("body = " + body.toAbsolutePath());
-            throw new AssertionError("body in wrong location");
+            BodyHandler bh = ofFileDownload(tempDir.resolve(uri.getPath().substring(1)),
+                    CREATE, TRUNCATE_EXISTING, WRITE);
+            HttpResponse<Path> response = client.send(request, bh);
+            Path body = response.body();
+            out.println("Got response: " + response);
+            out.println("Got body Path: " + body);
+            String fileContents = new String(Files.readAllBytes(response.body()), UTF_8);
+            out.println("Got body: " + fileContents);
+
+            assertEquals(response.statusCode(), 200);
+            assertEquals(body.getFileName().toString(), expectedFilename);
+            assertTrue(response.headers().firstValue("Content-Disposition").isPresent());
+            assertEquals(response.headers().firstValue("Content-Disposition").get(),
+                    contentDispositionValue);
+            assertEquals(fileContents, "May the luck of the Irish be with you!");
+
+            if (!body.toAbsolutePath().startsWith(tempDir.toAbsolutePath())) {
+                System.out.println("Tempdir = " + tempDir.toAbsolutePath());
+                System.out.println("body = " + body.toAbsolutePath());
+                throw new AssertionError("body in wrong location");
+            }
+            // additional checks unrelated to file download
+            caseInsensitivityOfHeaders(request.headers());
+            caseInsensitivityOfHeaders(response.headers());
+        } finally {
+            client = null;
+            System.gc();
+            while (!ref.refersTo(null)) {
+                System.gc();
+                if (queue.remove(100) == ref) break;
+            }
+            AssertionError failed = TRACKER.checkShutdown(1000);
+            if (failed != null) throw failed;
         }
-        // additional checks unrelated to file download
-        caseInsensitivityOfHeaders(request.headers());
-        caseInsensitivityOfHeaders(response.headers());
     }
 
     // --- Negative
@@ -246,18 +260,32 @@ void negativeTest(String uriString, String contentDispositionValue)
     {
         out.printf("negativeTest(%s, %s): starting", uriString, contentDispositionValue);
         HttpClient client = HttpClient.newBuilder().sslContext(sslContext).build();
+        TRACKER.track(client);
+        ReferenceQueue<HttpClient> queue = new ReferenceQueue<>();
+        WeakReference<HttpClient> ref = new WeakReference<>(client, queue);
 
-        URI uri = URI.create(uriString);
-        HttpRequest request = HttpRequest.newBuilder(uri)
-                .POST(BodyPublishers.ofString("Does not matter"))
-                .build();
-
-        BodyHandler bh = ofFileDownload(tempDir, CREATE, TRUNCATE_EXISTING, WRITE);
         try {
-            HttpResponse<Path> response = client.send(request, bh);
-            fail("UNEXPECTED response: " + response + ", path:" + response.body());
-        } catch (UncheckedIOException | IOException ioe) {
-            System.out.println("Caught expected: " + ioe);
+            URI uri = URI.create(uriString);
+            HttpRequest request = HttpRequest.newBuilder(uri)
+                    .POST(BodyPublishers.ofString("Does not matter"))
+                    .build();
+
+            BodyHandler bh = ofFileDownload(tempDir, CREATE, TRUNCATE_EXISTING, WRITE);
+            try {
+                HttpResponse<Path> response = client.send(request, bh);
+                fail("UNEXPECTED response: " + response + ", path:" + response.body());
+            } catch (UncheckedIOException | IOException ioe) {
+                System.out.println("Caught expected: " + ioe);
+            }
+        } finally {
+            client = null;
+            System.gc();
+            while (!ref.refersTo(null)) {
+                System.gc();
+                if (queue.remove(100) == ref) break;
+            }
+            AssertionError failed = TRACKER.checkShutdown(1000);
+            if (failed != null) throw failed;
         }
     }
 

test/jdk/java/net/httpclient/DigestEchoClient.java

@@ -23,6 +23,8 @@
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
 import java.math.BigInteger;
 import java.net.ProxySelector;
 import java.net.URI;
@@ -32,7 +34,6 @@
 import java.net.http.HttpRequest.BodyPublisher;
 import java.net.http.HttpRequest.BodyPublishers;
 import java.net.http.HttpResponse;
-import java.net.http.HttpResponse.BodyHandler;
 import java.net.http.HttpResponse.BodyHandlers;
 import java.nio.charset.StandardCharsets;
 import java.security.NoSuchAlgorithmException;
@@ -55,6 +56,7 @@
 import jdk.test.lib.net.SimpleSSLContext;
 import sun.net.NetProperties;
 import sun.net.www.HeaderParser;
+
 import static java.lang.System.out;
 import static java.lang.String.format;
 
@@ -386,6 +388,8 @@ void testBasic(Version clientVersion, Version serverVersion, boolean async,
                 server.getServerAddress(), "/foo/");
 
         HttpClient client = newHttpClient(server);
+        ReferenceQueue<HttpClient> queue = new ReferenceQueue<>();
+        WeakReference<HttpClient> ref = new WeakReference<>(client, queue);
         HttpResponse<String> r;
         CompletableFuture<HttpResponse<String>> cf1;
         String auth = null;
@@ -497,6 +501,14 @@ void testBasic(Version clientVersion, Version serverVersion, boolean async,
                 }
             }
         } finally {
+            client = null;
+            System.gc();
+            while (!ref.refersTo(null)) {
+                System.gc();
+                if (queue.remove(100) == ref) break;
+            }
+            var error = TRACKER.checkShutdown(500);
+            if (error != null) throw error;
         }
         System.out.println("OK");
     }

test/jdk/java/net/httpclient/ReferenceTracker.java

@@ -179,13 +179,15 @@ public AssertionError check(long graceDelayMs,
                                 boolean printThreads) {
         AssertionError fail = null;
         graceDelayMs = Math.max(graceDelayMs, 100);
-        long delay = Math.min(graceDelayMs, 500);
+        long delay = Math.min(graceDelayMs, 10);
         var count = delay > 0 ? graceDelayMs / delay : 1;
         for (int i = 0; i < count; i++) {
             if (TRACKERS.stream().anyMatch(hasOutstanding)) {
                 System.gc();
                 try {
-                    System.out.println("Waiting for HTTP operations to terminate...");
+                    if (i == 0) {
+                        System.out.println("Waiting for HTTP operations to terminate...");
+                    }
                     Thread.sleep(Math.min(graceDelayMs, Math.max(delay, 1)));
                 } catch (InterruptedException x) {
                     // OK