diff --git a/jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java b/jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java index 16f02d7cdde..06e8e09b422 100644 --- a/jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java +++ b/jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java @@ -27,9 +27,11 @@ import java.io.IOException; import java.io.NotSerializableException; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.HashMap; import java.net.URL; +import java.util.List; /** * A class that implements a cache of idle Http connections for keep-alive @@ -76,56 +78,68 @@ public 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; + 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; - java.security.AccessController.doPrivileged( - new java.security.PrivilegedAction() { - 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; + 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; + java.security.AccessController.doPrivileged( + new java.security.PrivilegedAction() { + 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; } + }); + } - 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; - } - }); + KeepAliveKey key = new KeepAliveKey(url, obj); + ClientVector v = super.get(key); + + if (v == null) { + int keepAliveTimeout = http.getKeepAliveTimeout(); + v = new ClientVector(keepAliveTimeout > 0? + keepAliveTimeout*1000 : LIFETIME); + v.put(http); + super.put(key, v); + } else { + oldClient = v.put(http); + } } - - KeepAliveKey key = new KeepAliveKey(url, obj); - ClientVector v = super.get(key); - - if (v == null) { - int keepAliveTimeout = http.getKeepAliveTimeout(); - v = new ClientVector(keepAliveTimeout > 0? - keepAliveTimeout*1000 : LIFETIME); - v.put(http); - super.put(key, v); - } else { - v.put(http); + // close after releasing locks + if (oldClient != null) { + oldClient.closeServer(); } } @@ -135,7 +149,7 @@ public synchronized void remove (HttpClient h, Object obj) { ClientVector v = super.get(key); if (v != null) { v.remove(h); - if (v.empty()) { + if (v.isEmpty()) { removeVector(key); } } @@ -171,39 +185,31 @@ public void run() { try { Thread.sleep(LIFETIME); } catch (InterruptedException e) {} - synchronized (this) { - /* Remove all unused HttpClients. Starting from the - * bottom of the stack (the least-recently used first). - * REMIND: It'd be nice to not remove *all* connections - * that aren't presently in use. One could have been added - * a second ago that's still perfectly valid, and we're - * needlessly axing it. But it's not clear how to do this - * cleanly, and doing it right may be more trouble than it's - * worth. - */ + List closeList = null; + // Remove all outdated HttpClients. + synchronized (this) { long currentTime = System.currentTimeMillis(); - - ArrayList keysToRemove - = new ArrayList(); + List keysToRemove = new ArrayList<>(); for (KeepAliveKey key : keySet()) { ClientVector v = get(key); synchronized (v) { - int i; - - for (i = 0; i < v.size(); i++) { - KeepAliveEntry e = v.elementAt(i); + KeepAliveEntry e = v.peekLast(); + while (e != null) { if ((currentTime - e.idleStartTime) > v.nap) { - HttpClient h = e.hc; - h.closeServer(); + v.pollLast(); + if (closeList == null) { + closeList = new ArrayList<>(); + } + closeList.add(e.hc); } else { break; } + e = v.peekLast(); } - v.subList(0, i).clear(); - if (v.size() == 0) { + if (v.isEmpty()) { keysToRemove.add(key); } } @@ -213,9 +219,13 @@ public void run() { removeVector(key); } } - } while (size() > 0); - - return; + // close connections outside cacheLock + if (closeList != null) { + for (HttpClient hc : closeList) { + hc.closeServer(); + } + } + } while (!isEmpty()); } /* @@ -232,12 +242,10 @@ private void readObject(java.io.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 java.util.Stack { +class ClientVector extends ArrayDeque { private static final long serialVersionUID = -8680532108106489459L; // sleep time in milliseconds, before cache clear @@ -250,31 +258,42 @@ class ClientVector extends java.util.Stack { } synchronized HttpClient get() { - if (empty()) { + // check the most recent connection, use if still valid + KeepAliveEntry e = peekFirst(); + if (e == null) { return null; + } + + long currentTime = System.currentTimeMillis(); + if ((currentTime - e.idleStartTime) > nap) { + return null; // all connections stale - will be cleaned up later } else { - // 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; - } - } while ((hc== null) && (!empty())); - return hc; + pollFirst(); + return e.hc; } } + /* remove an HttpClient */ + synchronized boolean remove(HttpClient h) { + for (KeepAliveEntry curr : this) { + if (curr.hc == h) { + return super.remove(curr); + } + } + return false; + } + /* 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; } /* @@ -293,10 +312,10 @@ private void readObject(java.io.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 @@ -337,8 +356,8 @@ public int hashCode() { } class KeepAliveEntry { - HttpClient hc; - long idleStartTime; + final HttpClient hc; + final long idleStartTime; KeepAliveEntry(HttpClient hc, long idleStartTime) { this.hc = hc; diff --git a/jdk/src/share/classes/sun/net/www/protocol/https/HttpsClient.java b/jdk/src/share/classes/sun/net/www/protocol/https/HttpsClient.java index 98e3fcc3213..131a3dc7634 100644 --- a/jdk/src/share/classes/sun/net/www/protocol/https/HttpsClient.java +++ b/jdk/src/share/classes/sun/net/www/protocol/https/HttpsClient.java @@ -426,6 +426,15 @@ public boolean needsTunneling() { && proxy.type() != Proxy.Type.SOCKS); } + @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 void afterConnect() throws IOException, UnknownHostException { if (!isCachedConnection()) { diff --git a/jdk/test/sun/net/www/http/KeepAliveCache/B8293562.java b/jdk/test/sun/net/www/http/KeepAliveCache/B8293562.java new file mode 100644 index 00000000000..4e0cff8bab4 --- /dev/null +++ b/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 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(); + } + } +} +