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 5acef4d7ce7..b93653e70e2 100644 --- a/jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java +++ b/jdk/src/share/classes/sun/net/www/http/KeepAliveCache.java @@ -112,49 +112,56 @@ 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; - AccessController.doPrivileged(new 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; + AccessController.doPrivileged(new 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); + KeepAliveKey key = new KeepAliveKey(url, obj); + ClientVector v = super.get(key); - if (v == null) { - int keepAliveTimeout = http.getKeepAliveTimeout(); + if (v == null) { + int keepAliveTimeout = http.getKeepAliveTimeout(); if (keepAliveTimeout == 0) { keepAliveTimeout = getUserKeepAlive(http.getUsingProxy()); if (keepAliveTimeout == -1) { @@ -174,14 +181,19 @@ 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); + } else { + oldClient = v.put(http); + } + } + // close after releasing locks + if (oldClient != null) { + oldClient.closeServer(); } } @@ -213,7 +225,6 @@ synchronized void removeVector(KeepAliveKey k) { * 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 void run() { try { Thread.sleep(LIFETIME); } catch (InterruptedException e) {} + List closeList = null; // Remove all outdated HttpClients. synchronized (this) { @@ -241,15 +253,18 @@ public void run() { 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 void run() { removeVector(key); } } + // close connections outside cacheLock + if (closeList != null) { + for (HttpClient hc : closeList) { + hc.closeServer(); + } + } } while (!isEmpty()); } @@ -279,8 +300,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 { private static final long serialVersionUID = -8680532108106489459L; @@ -293,36 +314,37 @@ class ClientVector extends ArrayDeque { } 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 @@ 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 @@ -394,8 +416,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..590c04e4223 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 @@ -419,6 +419,14 @@ 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() { 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(); + } + } +} +