# HG changeset patch # User clanger # Date 1508396489 -7200 # Node ID 74e1913a98c00ada251a3616a88e19b047cfc2df # Parent 31c9cf5eca62345ac73b2eec1b47f38ed25f155e 8155590: Dubious collection management in sun.net.www.http.KeepAliveCache Reviewed-by: rriggs, vtewari diff -r 31c9cf5eca62 -r 74e1913a98c0 src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java --- a/src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java Thu Oct 19 14:49:20 2017 +0800 +++ b/src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java Thu Oct 19 09:01:29 2017 +0200 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1996, 2011, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1996, 2017, 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 @@ -27,10 +27,18 @@ import java.io.IOException; import java.io.NotSerializableException; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; +import java.net.URL; +import java.security.AccessController; +import java.security.PrivilegedAction; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.HashMap; -import java.net.URL; +import java.util.List; + import jdk.internal.misc.InnocuousThread; +import sun.security.action.GetIntegerAction; /** * A class that implements a cache of idle Http connections for keep-alive @@ -53,14 +61,14 @@ static int result = -1; static int getMaxConnections() { if (result == -1) { - result = java.security.AccessController.doPrivileged( - new sun.security.action.GetIntegerAction("http.maxConnections", - MAX_CONNECTIONS)) + result = AccessController.doPrivileged( + new GetIntegerAction("http.maxConnections", MAX_CONNECTIONS)) .intValue(); - if (result <= 0) + if (result <= 0) { result = MAX_CONNECTIONS; + } } - return result; + return result; } static final int LIFETIME = 5000; @@ -93,8 +101,7 @@ * The robustness to get around this is in HttpClient.parseHTTP() */ final KeepAliveCache cache = this; - java.security.AccessController.doPrivileged( - new java.security.PrivilegedAction<>() { + AccessController.doPrivileged(new PrivilegedAction<>() { public Void run() { keepAliveTimer = InnocuousThread.newSystemThread("Keep-Alive-Timer", cache); keepAliveTimer.setDaemon(true); @@ -110,8 +117,8 @@ if (v == null) { int keepAliveTimeout = http.getKeepAliveTimeout(); - v = new ClientVector(keepAliveTimeout > 0? - keepAliveTimeout*1000 : LIFETIME); + v = new ClientVector(keepAliveTimeout > 0 ? + keepAliveTimeout * 1000 : LIFETIME); v.put(http); super.put(key, v); } else { @@ -120,12 +127,12 @@ } /* remove an obsolete HttpClient from its VectorCache */ - public synchronized void remove (HttpClient h, Object obj) { + public synchronized void remove(HttpClient h, Object obj) { KeepAliveKey key = new KeepAliveKey(h.url, obj); ClientVector v = super.get(key); if (v != null) { v.remove(h); - if (v.empty()) { + if (v.isEmpty()) { removeVector(key); } } @@ -142,7 +149,6 @@ * 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 @@ -161,39 +167,27 @@ try { Thread.sleep(LIFETIME); } catch (InterruptedException e) {} + + // Remove all outdated HttpClients. 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. - */ - 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.peek(); + while (e != null) { if ((currentTime - e.idleStartTime) > v.nap) { - HttpClient h = e.hc; - h.closeServer(); + v.poll(); + e.hc.closeServer(); } else { break; } + e = v.peek(); } - v.subList(0, i).clear(); - if (v.size() == 0) { + if (v.isEmpty()) { keysToRemove.add(key); } } @@ -203,21 +197,19 @@ removeVector(key); } } - } while (size() > 0); - - return; + } while (!isEmpty()); } /* * Do not serialize this class! */ - private void writeObject(java.io.ObjectOutputStream stream) - throws IOException { + private void writeObject(ObjectOutputStream stream) throws IOException { throw new NotSerializableException(); } - private void readObject(java.io.ObjectInputStream stream) - throws IOException, ClassNotFoundException { + private void readObject(ObjectInputStream stream) + throws IOException, ClassNotFoundException + { throw new NotSerializableException(); } } @@ -225,37 +217,33 @@ /* FILO order for recycling HttpClients, should run in a thread * to time them out. If > maxConns are in use, block. */ - - -class ClientVector extends java.util.Stack { +class ClientVector extends ArrayDeque { private static final long serialVersionUID = -8680532108106489459L; // sleep time in milliseconds, before cache clear int nap; - - - ClientVector (int nap) { + ClientVector(int nap) { this.nap = nap; } synchronized HttpClient get() { - if (empty()) { + if (isEmpty()) { return null; - } 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; } + + // 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) && (!isEmpty())); + return hc; } /* return a still valid, unused HttpClient */ @@ -267,21 +255,30 @@ } } + /* remove an HttpClient */ + synchronized boolean remove(HttpClient h) { + for (KeepAliveEntry curr : this) { + if (curr.hc == h) { + return super.remove(curr); + } + } + return false; + } + /* * Do not serialize this class! */ - private void writeObject(java.io.ObjectOutputStream stream) - throws IOException { + private void writeObject(ObjectOutputStream stream) throws IOException { throw new NotSerializableException(); } - private void readObject(java.io.ObjectInputStream stream) - throws IOException, ClassNotFoundException { + private void readObject(ObjectInputStream stream) + throws IOException, ClassNotFoundException + { throw new NotSerializableException(); } } - class KeepAliveKey { private String protocol = null; private String host = null;