8293562: blocked threads with KeepAliveCache.get

Reviewed-by: dfuchs, michaelm

Author: djelinski

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

@@ -122,6 +122,12 @@ public KeepAliveCache() {}
      */
     @SuppressWarnings("removal")
     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;
         cacheLock.lock();
         try {
             boolean startThread = (keepAliveTimer == null);
@@ -172,42 +178,29 @@ public Void run() {
                 // 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);
+                oldClient = v.put(http);
             }
         } finally {
             cacheLock.unlock();
         }
+        // close after releasing locks
+        if (oldClient != null) {
+            oldClient.closeServer();
+        }
     }
 
     // returns the keep alive set by user in system property or -1 if not set
     private static int getUserKeepAlive(boolean isProxy) {
         return isProxy ? userKeepAliveProxy : userKeepAliveServer;
     }
 
-    /* remove an obsolete HttpClient from its VectorCache */
-    public void remove(HttpClient h, Object obj) {
-        cacheLock.lock();
-        try {
-            KeepAliveKey key = new KeepAliveKey(h.url, obj);
-            ClientVector v = super.get(key);
-            if (v != null) {
-                v.remove(h);
-                if (v.isEmpty()) {
-                    removeVector(key);
-                }
-            }
-        } finally {
-            cacheLock.unlock();
-        }
-    }
-
     /* called by a clientVector thread when all its connections have timed out
      * and that vector of connections should be removed.
      */
@@ -243,6 +236,7 @@ public void run() {
             try {
                 Thread.sleep(LIFETIME);
             } catch (InterruptedException e) {}
+            List<HttpClient> closeList = null;
 
             // Remove all outdated HttpClients.
             cacheLock.lock();
@@ -254,15 +248,18 @@ public void run() {
                     ClientVector v = get(key);
                     v.lock();
                     try {
-                        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()) {
@@ -278,6 +275,12 @@ public void run() {
                 }
             } finally {
                 cacheLock.unlock();
+                // close connections outside cacheLock
+                if (closeList != null) {
+                    for (HttpClient hc : closeList) {
+                        hc.closeServer();
+                    }
+                }
             }
         } while (!isEmpty());
     }
@@ -298,8 +301,8 @@ private void readObject(ObjectInputStream stream)
     }
 }
 
-/* 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> {
     @java.io.Serial
@@ -313,62 +316,47 @@ class ClientVector extends ArrayDeque<KeepAliveEntry> {
         this.nap = nap;
     }
 
+    /* return a still valid, idle HttpClient */
     HttpClient get() {
         lock();
         try {
-            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 "
-                                + Long.toString(currentTime - e.idleStartTime);
-                        KeepAliveCache.logger.finest(msg);
-                    }
-                }
-            } while ((hc == null) && (!isEmpty()));
-            return hc;
-        } finally {
-            unlock();
-        }
-    }
-
-    /* return a still valid, unused HttpClient */
-    void put(HttpClient h) {
-        lock();
-        try {
-            if (size() >= KeepAliveCache.getMaxConnections()) {
-                h.closeServer(); // otherwise the connection remains in limbo
+            if ((currentTime - e.idleStartTime) > nap) {
+                return null; // all connections stale - will be cleaned up later
             } else {
-                push(new KeepAliveEntry(h, System.currentTimeMillis()));
+                pollFirst();
+                if (KeepAliveCache.logger.isLoggable(PlatformLogger.Level.FINEST)) {
+                    String msg = "cached HttpClient was idle for "
+                            + Long.toString(currentTime - e.idleStartTime);
+                    KeepAliveCache.logger.finest(msg);
+                }
+                return e.hc;
             }
         } finally {
             unlock();
         }
     }
 
-    /* remove an HttpClient */
-    boolean remove(HttpClient h) {
+    HttpClient put(HttpClient h) {
+        HttpClient staleClient = null;
         lock();
         try {
-            for (KeepAliveEntry curr : this) {
-                if (curr.hc == h) {
-                    return super.remove(curr);
-                }
+            assert KeepAliveCache.getMaxConnections() > 0;
+            if (size() >= KeepAliveCache.getMaxConnections()) {
+                // remove oldest connection
+                staleClient = removeLast().hc;
             }
-            return false;
+            addFirst(new KeepAliveEntry(h, System.currentTimeMillis()));
         } finally {
             unlock();
         }
+        // close after releasing the locks
+        return staleClient;
     }
 
     final void lock() {
@@ -396,10 +384,10 @@ private void readObject(ObjectInputStream stream)
 }
 
 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
@@ -440,8 +428,8 @@ public int hashCode() {
 }
 
 class KeepAliveEntry {
-    HttpClient hc;
-    long idleStartTime;
+    final HttpClient hc;
+    final long idleStartTime;
 
     KeepAliveEntry(HttpClient hc, long idleStartTime) {
         this.hc = hc;

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

@@ -434,6 +434,15 @@ protected Socket createSocket() throws IOException {
         }
     }
 
+    @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() {