8293562: KeepAliveCache Blocks Threads while Closing Connections

Reviewed-by: sgehwolf, phh, andrew
Backport-of: 770c1f65c588f3156f9b70097df752d8059c1038

jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java

@@ -112,49 +112,56 @@ public class KeepAliveCache
      * @param url  The URL contains info about the host and port
      * @param http The HttpClient to be cached
      */
-    public synchronized void put(final URL url, Object obj, HttpClient http) {
-        boolean startThread = (keepAliveTimer == null);
-        if (!startThread) {
-            if (!keepAliveTimer.isAlive()) {
-                startThread = true;
-            }
-        }
-        if (startThread) {
-            clear();
-            /* Unfortunately, we can't always believe the keep-alive timeout we got
-             * back from the server.  If I'm connected through a Netscape proxy
-             * to a server that sent me a keep-alive
-             * time of 15 sec, the proxy unilaterally terminates my connection
-             * The robustness to get around this is in HttpClient.parseHTTP()
-             */
-            final KeepAliveCache cache = this;
-            AccessController.doPrivileged(new PrivilegedAction<Void>() {
-                public Void run() {
-                   // We want to create the Keep-Alive-Timer in the
-                    // system threadgroup
-                    ThreadGroup grp = Thread.currentThread().getThreadGroup();
-                    ThreadGroup parent = null;
-                    while ((parent = grp.getParent()) != null) {
-                        grp = parent;
-                    }
-
-                    keepAliveTimer = new Thread(grp, cache, "Keep-Alive-Timer");
-                    keepAliveTimer.setDaemon(true);
-                    keepAliveTimer.setPriority(Thread.MAX_PRIORITY - 2);
-                    // Set the context class loader to null in order to avoid
-                    // keeping a strong reference to an application classloader.
-                    keepAliveTimer.setContextClassLoader(null);
-                    keepAliveTimer.start();
-                    return null;
+    public void put(final URL url, Object obj, HttpClient http) {
+        // this method may need to close an HttpClient, either because
+        // it is not cacheable, or because the cache is at its capacity.
+        // In the latter case, we close the least recently used client.
+        // The client to close is stored in oldClient, and is closed
+        // after cacheLock is released.
+        HttpClient oldClient = null;
+        synchronized (this) {
+            boolean startThread = (keepAliveTimer == null);
+            if (!startThread) {
+                if (!keepAliveTimer.isAlive()) {
+                    startThread = true;
                 }
-            });
-        }
+            }
+            if (startThread) {
+                clear();
+                /* Unfortunately, we can't always believe the keep-alive timeout we got
+                * back from the server.  If I'm connected through a Netscape proxy
+                * to a server that sent me a keep-alive
+                * time of 15 sec, the proxy unilaterally terminates my connection
+                * The robustness to get around this is in HttpClient.parseHTTP()
+                */
+                final KeepAliveCache cache = this;
+                AccessController.doPrivileged(new PrivilegedAction<Void>() {
+                    public Void run() {
+                        // We want to create the Keep-Alive-Timer in the
+                        // system threadgroup
+                        ThreadGroup grp = Thread.currentThread().getThreadGroup();
+                        ThreadGroup parent = null;
+                        while ((parent = grp.getParent()) != null) {
+                            grp = parent;
+                        }
 
-        KeepAliveKey key = new KeepAliveKey(url, obj);
-        ClientVector v = super.get(key);
+                        keepAliveTimer = new Thread(grp, cache, "Keep-Alive-Timer");
+                        keepAliveTimer.setDaemon(true);
+                        keepAliveTimer.setPriority(Thread.MAX_PRIORITY - 2);
+                        // Set the context class loader to null in order to avoid
+                        // keeping a strong reference to an application classloader.
+                        keepAliveTimer.setContextClassLoader(null);
+                        keepAliveTimer.start();
+                        return null;
+                    }
+                 });
+            }
 
-        if (v == null) {
-            int keepAliveTimeout = http.getKeepAliveTimeout();
+            KeepAliveKey key = new KeepAliveKey(url, obj);
+            ClientVector v = super.get(key);
+
+            if (v == null) {
+                int keepAliveTimeout = http.getKeepAliveTimeout();
                 if (keepAliveTimeout == 0) {
                     keepAliveTimeout = getUserKeepAlive(http.getUsingProxy());
                     if (keepAliveTimeout == -1) {
@@ -174,14 +181,19 @@ public class KeepAliveCache
                 // alive, which could be 0, if the user specified 0 for the property
                 assert keepAliveTimeout >= 0;
                 if (keepAliveTimeout == 0) {
-                    http.closeServer();
+                    oldClient = http;
                 } else {
                     v = new ClientVector(keepAliveTimeout * 1000);
                     v.put(http);
                     super.put(key, v);
                 }
-        } else {
-            v.put(http);
+            } else {
+                oldClient = v.put(http);
+            }
+        }
+        // close after releasing locks
+        if (oldClient != null) {
+            oldClient.closeServer();
         }
     }
 
@@ -213,7 +225,6 @@ public class KeepAliveCache
      * Check to see if this URL has a cached HttpClient
      */
     public synchronized HttpClient get(URL url, Object obj) {
-
         KeepAliveKey key = new KeepAliveKey(url, obj);
         ClientVector v = super.get(key);
         if (v == null) { // nothing in cache yet
@@ -232,6 +243,7 @@ public class KeepAliveCache
             try {
                 Thread.sleep(LIFETIME);
             } catch (InterruptedException e) {}
+            List<HttpClient> closeList = null;
 
             // Remove all outdated HttpClients.
             synchronized (this) {
@@ -241,15 +253,18 @@ public class KeepAliveCache
                 for (KeepAliveKey key : keySet()) {
                     ClientVector v = get(key);
                     synchronized (v) {
-                        KeepAliveEntry e = v.peek();
+                        KeepAliveEntry e = v.peekLast();
                         while (e != null) {
                             if ((currentTime - e.idleStartTime) > v.nap) {
-                                v.poll();
-                                e.hc.closeServer();
+                                v.pollLast();
+                                if (closeList == null) {
+                                    closeList = new ArrayList<>();
+                                }
+                                closeList.add(e.hc);
                             } else {
                                 break;
                             }
-                            e = v.peek();
+                            e = v.peekLast();
                         }
 
                         if (v.isEmpty()) {
@@ -262,6 +277,12 @@ public class KeepAliveCache
                     removeVector(key);
                 }
             }
+            // close connections outside cacheLock
+            if (closeList != null) {
+                 for (HttpClient hc : closeList) {
+                     hc.closeServer();
+               }
+          }
         } while (!isEmpty());
     }
 
@@ -279,8 +300,8 @@ public class KeepAliveCache
     }
 }
 
-/* FILO order for recycling HttpClients, should run in a thread
- * to time them out.  If > maxConns are in use, block.
+/* LIFO order for reusing HttpClients. Most recent entries at the front.
+ * If > maxConns are in use, discard oldest.
  */
 class ClientVector extends ArrayDeque<KeepAliveEntry> {
     private static final long serialVersionUID = -8680532108106489459L;
@@ -293,36 +314,37 @@ class ClientVector extends ArrayDeque<KeepAliveEntry> {
     }
 
     synchronized HttpClient get() {
-        if (isEmpty()) {
+        // check the most recent connection, use if still valid
+        KeepAliveEntry e = peekFirst();
+        if (e == null) {
             return null;
         }
 
-        // Loop until we find a connection that has not timed out
-        HttpClient hc = null;
         long currentTime = System.currentTimeMillis();
-        do {
-            KeepAliveEntry e = pop();
-            if ((currentTime - e.idleStartTime) > nap) {
-                e.hc.closeServer();
-            } else {
-                hc = e.hc;
-                if (KeepAliveCache.logger.isLoggable(PlatformLogger.Level.FINEST)) {
-                    String msg = "cached HttpClient was idle for "
+        if ((currentTime - e.idleStartTime) > nap) {
+            return null; // all connections stale - will be cleaned up later
+        } else {
+            pollFirst();
+            if (KeepAliveCache.logger.isLoggable(PlatformLogger.Level.FINEST)) {
+                String msg = "cached HttpClient was idle for "
                         + Long.toString(currentTime - e.idleStartTime);
-                    KeepAliveCache.logger.finest(msg);
-                }
+                KeepAliveCache.logger.finest(msg);
             }
-        } while ((hc == null) && (!isEmpty()));
-        return hc;
+            return e.hc;
+        }
     }
 
     /* return a still valid, unused HttpClient */
-    synchronized void put(HttpClient h) {
+    synchronized HttpClient put(HttpClient h) {
+        HttpClient staleClient = null;
+        assert KeepAliveCache.getMaxConnections() > 0;
         if (size() >= KeepAliveCache.getMaxConnections()) {
-            h.closeServer(); // otherwise the connection remains in limbo
-        } else {
-            push(new KeepAliveEntry(h, System.currentTimeMillis()));
+           // remove oldest connection
+           staleClient = removeLast().hc;
         }
+        addFirst(new KeepAliveEntry(h, System.currentTimeMillis()));
+        // close after releasing the locks
+        return staleClient;
     }
 
     /* remove an HttpClient */
@@ -350,10 +372,10 @@ class ClientVector extends ArrayDeque<KeepAliveEntry> {
 }
 
 class KeepAliveKey {
-    private String      protocol = null;
-    private String      host = null;
-    private int         port = 0;
-    private Object      obj = null; // additional key, such as socketfactory
+    private final String      protocol;
+    private final String      host;
+    private final int         port;
+    private final Object      obj; // additional key, such as socketfactory
 
     /**
      * Constructor
@@ -394,8 +416,8 @@ class KeepAliveKey {
 }
 
 class KeepAliveEntry {
-    HttpClient hc;
-    long idleStartTime;
+    final HttpClient hc;
+    final long idleStartTime;
 
     KeepAliveEntry(HttpClient hc, long idleStartTime) {
         this.hc = hc;

jdk/src/share/classes/sun/net/www/protocol/https/HttpsClient.java

@@ -419,6 +419,14 @@ final class HttpsClient extends HttpClient
         }
     }
 
+    @Override
+    public void closeServer() {
+        try {
+            // SSLSocket.close may block up to timeout. Make sure it's short.
+            serverSocket.setSoTimeout(1);
+        } catch (Exception e) {}
+        super.closeServer();
+    }
 
     @Override
     public boolean needsTunneling() {

jdk/test/sun/net/www/http/KeepAliveCache/B8293562.java

@@ -0,0 +1,238 @@
+/*
+ * Copyright (c) 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
+ * 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 8293562
+ * @library /test/lib
+ * @run main/othervm -Dhttp.keepAlive.time.server=1 B8293562
+ * @summary Http keep-alive thread should close sockets without holding a lock
+ */
+
+import com.sun.net.httpserver.HttpServer;
+
+import javax.net.ssl.HandshakeCompletedListener;
+import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.SSLSession;
+import javax.net.ssl.SSLSocket;
+import javax.net.ssl.SSLSocketFactory;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.Proxy;
+import java.net.Socket;
+import java.net.URL;
+import java.net.UnknownHostException;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+public class B8293562 {
+    static HttpServer server;
+    static CountDownLatch closing = new CountDownLatch(1);
+    static CountDownLatch secondRequestDone = new CountDownLatch(1);
+    static CompletableFuture<Void> result = new CompletableFuture<>();
+
+    public static void main(String[] args) throws Exception {
+        startHttpServer();
+        clientHttpCalls();
+    }
+
+    public static void startHttpServer() throws Exception {
+        server = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 10);
+        server.setExecutor(Executors.newCachedThreadPool());
+        server.start();
+    }
+
+    public static void clientHttpCalls() throws Exception {
+        try {
+            System.out.println("http server listen on: " + server.getAddress().getPort());
+            String hostAddr = InetAddress.getLoopbackAddress().getHostAddress();
+            if (hostAddr.indexOf(':') > -1) hostAddr = "[" + hostAddr + "]";
+            String baseURLStr = "https://" + hostAddr + ":" + server.getAddress().getPort() + "/";
+
+            URL testUrl = new URL (baseURLStr);
+
+            // SlowCloseSocketFactory is not a real SSLSocketFactory;
+            // it produces regular non-SSL sockets. Effectively, the request
+            // is made over http.
+            HttpsURLConnection.setDefaultSSLSocketFactory(new SlowCloseSocketFactory());
+            System.out.println("Performing first request");
+            HttpsURLConnection uc = (HttpsURLConnection)testUrl.openConnection(Proxy.NO_PROXY);
+            byte[] buf = new byte[1024];
+            try {
+                uc.getInputStream();
+                throw new RuntimeException("Expected 404 here");
+            } catch (FileNotFoundException ignored) { }
+            try (InputStream is = uc.getErrorStream()) {
+                while (is.read(buf) >= 0) {
+                }
+            }
+            System.out.println("First request completed");
+            closing.await();
+            // KeepAliveThread is closing the connection now
+            System.out.println("Performing second request");
+            HttpsURLConnection uc2 = (HttpsURLConnection)testUrl.openConnection(Proxy.NO_PROXY);
+
+            try {
+                uc2.getInputStream();
+                throw new RuntimeException("Expected 404 here");
+            } catch (FileNotFoundException ignored) { }
+            try (InputStream is = uc2.getErrorStream()) {
+                while (is.read(buf) >= 0) {
+                }
+            }
+            System.out.println("Second request completed");
+            // let the socket know it can close now
+            secondRequestDone.countDown();
+            result.get();
+            System.out.println("Test completed successfully");
+        } finally {
+            server.stop(1);
+        }
+    }
+
+    static class SlowCloseSocket extends SSLSocket {
+        @Override
+        public synchronized void close() throws IOException {
+            String threadName = Thread.currentThread().getName();
+            System.out.println("Connection closing, thread name: " + threadName);
+            closing.countDown();
+            super.close();
+            if (threadName.equals("Keep-Alive-Timer")) {
+                try {
+                    if (secondRequestDone.await(5, TimeUnit.SECONDS)) {
+                        result.complete(null);
+                    } else {
+                        result.completeExceptionally(new RuntimeException(
+                                "Wait for second request timed out"));
+                    }
+                } catch (InterruptedException e) {
+                    result.completeExceptionally(new RuntimeException(
+                            "Wait for second request was interrupted"));
+                }
+            } else {
+                result.completeExceptionally(new RuntimeException(
+                        "Close invoked from unexpected thread"));
+            }
+            System.out.println("Connection closed");
+        }
+
+        // required abstract method overrides
+        @Override
+        public String[] getSupportedCipherSuites() {
+            return new String[0];
+        }
+        @Override
+        public String[] getEnabledCipherSuites() {
+            return new String[0];
+        }
+        @Override
+        public void setEnabledCipherSuites(String[] suites) { }
+        @Override
+        public String[] getSupportedProtocols() {
+            return new String[0];
+        }
+        @Override
+        public String[] getEnabledProtocols() {
+            return new String[0];
+        }
+        @Override
+        public void setEnabledProtocols(String[] protocols) { }
+        @Override
+        public SSLSession getSession() {
+            return null;
+        }
+        @Override
+        public void addHandshakeCompletedListener(HandshakeCompletedListener listener) { }
+        @Override
+        public void removeHandshakeCompletedListener(HandshakeCompletedListener listener) { }
+        @Override
+        public void startHandshake() throws IOException { }
+        @Override
+        public void setUseClientMode(boolean mode) { }
+        @Override
+        public boolean getUseClientMode() {
+            return false;
+        }
+        @Override
+        public void setNeedClientAuth(boolean need) { }
+        @Override
+        public boolean getNeedClientAuth() {
+            return false;
+        }
+        @Override
+        public void setWantClientAuth(boolean want) { }
+        @Override
+        public boolean getWantClientAuth() {
+            return false;
+        }
+        @Override
+        public void setEnableSessionCreation(boolean flag) { }
+        @Override
+        public boolean getEnableSessionCreation() {
+            return false;
+        }
+    }
+
+    static class SlowCloseSocketFactory extends SSLSocketFactory {
+
+        @Override
+        public Socket createSocket() throws IOException {
+            return new SlowCloseSocket();
+        }
+        // required abstract method overrides
+        @Override
+        public Socket createSocket(String host, int port) throws IOException, UnknownHostException {
+            throw new UnsupportedOperationException();
+        }
+        @Override
+        public Socket createSocket(String host, int port, InetAddress localHost, int localPort) throws IOException, UnknownHostException {
+            throw new UnsupportedOperationException();
+        }
+        @Override
+        public Socket createSocket(InetAddress host, int port) throws IOException {
+            throw new UnsupportedOperationException();
+        }
+        @Override
+        public Socket createSocket(InetAddress address, int port, InetAddress localAddress, int localPort) throws IOException {
+            throw new UnsupportedOperationException();
+        }
+        @Override
+        public String[] getDefaultCipherSuites() {
+            return new String[0];
+        }
+        @Override
+        public String[] getSupportedCipherSuites() {
+            return new String[0];
+        }
+        @Override
+        public Socket createSocket(Socket s, String host, int port, boolean autoClose) throws IOException {
+            throw new UnsupportedOperationException();
+        }
+    }
+}
+