Skip to content

Commit 03f25a9

Browse files
committedOct 3, 2022
8293562: blocked threads with KeepAliveCache.get
Reviewed-by: dfuchs, michaelm
1 parent a69ee85 commit 03f25a9

File tree

3 files changed

+303
-68
lines changed

3 files changed

+303
-68
lines changed
 

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

+56-68
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,12 @@ public KeepAliveCache() {}
122122
*/
123123
@SuppressWarnings("removal")
124124
public void put(final URL url, Object obj, HttpClient http) {
125+
// this method may need to close an HttpClient, either because
126+
// it is not cacheable, or because the cache is at its capacity.
127+
// In the latter case, we close the least recently used client.
128+
// The client to close is stored in oldClient, and is closed
129+
// after cacheLock is released.
130+
HttpClient oldClient = null;
125131
cacheLock.lock();
126132
try {
127133
boolean startThread = (keepAliveTimer == null);
@@ -172,42 +178,29 @@ public Void run() {
172178
// alive, which could be 0, if the user specified 0 for the property
173179
assert keepAliveTimeout >= 0;
174180
if (keepAliveTimeout == 0) {
175-
http.closeServer();
181+
oldClient = http;
176182
} else {
177183
v = new ClientVector(keepAliveTimeout * 1000);
178184
v.put(http);
179185
super.put(key, v);
180186
}
181187
} else {
182-
v.put(http);
188+
oldClient = v.put(http);
183189
}
184190
} finally {
185191
cacheLock.unlock();
186192
}
193+
// close after releasing locks
194+
if (oldClient != null) {
195+
oldClient.closeServer();
196+
}
187197
}
188198

189199
// returns the keep alive set by user in system property or -1 if not set
190200
private static int getUserKeepAlive(boolean isProxy) {
191201
return isProxy ? userKeepAliveProxy : userKeepAliveServer;
192202
}
193203

194-
/* remove an obsolete HttpClient from its VectorCache */
195-
public void remove(HttpClient h, Object obj) {
196-
cacheLock.lock();
197-
try {
198-
KeepAliveKey key = new KeepAliveKey(h.url, obj);
199-
ClientVector v = super.get(key);
200-
if (v != null) {
201-
v.remove(h);
202-
if (v.isEmpty()) {
203-
removeVector(key);
204-
}
205-
}
206-
} finally {
207-
cacheLock.unlock();
208-
}
209-
}
210-
211204
/* called by a clientVector thread when all its connections have timed out
212205
* and that vector of connections should be removed.
213206
*/
@@ -243,6 +236,7 @@ public void run() {
243236
try {
244237
Thread.sleep(LIFETIME);
245238
} catch (InterruptedException e) {}
239+
List<HttpClient> closeList = null;
246240

247241
// Remove all outdated HttpClients.
248242
cacheLock.lock();
@@ -254,15 +248,18 @@ public void run() {
254248
ClientVector v = get(key);
255249
v.lock();
256250
try {
257-
KeepAliveEntry e = v.peek();
251+
KeepAliveEntry e = v.peekLast();
258252
while (e != null) {
259253
if ((currentTime - e.idleStartTime) > v.nap) {
260-
v.poll();
261-
e.hc.closeServer();
254+
v.pollLast();
255+
if (closeList == null) {
256+
closeList = new ArrayList<>();
257+
}
258+
closeList.add(e.hc);
262259
} else {
263260
break;
264261
}
265-
e = v.peek();
262+
e = v.peekLast();
266263
}
267264

268265
if (v.isEmpty()) {
@@ -278,6 +275,12 @@ public void run() {
278275
}
279276
} finally {
280277
cacheLock.unlock();
278+
// close connections outside cacheLock
279+
if (closeList != null) {
280+
for (HttpClient hc : closeList) {
281+
hc.closeServer();
282+
}
283+
}
281284
}
282285
} while (!isEmpty());
283286
}
@@ -298,8 +301,8 @@ private void readObject(ObjectInputStream stream)
298301
}
299302
}
300303

301-
/* FILO order for recycling HttpClients, should run in a thread
302-
* to time them out. If > maxConns are in use, block.
304+
/* LIFO order for reusing HttpClients. Most recent entries at the front.
305+
* If > maxConns are in use, discard oldest.
303306
*/
304307
class ClientVector extends ArrayDeque<KeepAliveEntry> {
305308
@java.io.Serial
@@ -313,62 +316,47 @@ class ClientVector extends ArrayDeque<KeepAliveEntry> {
313316
this.nap = nap;
314317
}
315318

319+
/* return a still valid, idle HttpClient */
316320
HttpClient get() {
317321
lock();
318322
try {
319-
if (isEmpty()) {
323+
// check the most recent connection, use if still valid
324+
KeepAliveEntry e = peekFirst();
325+
if (e == null) {
320326
return null;
321327
}
322-
323-
// Loop until we find a connection that has not timed out
324-
HttpClient hc = null;
325328
long currentTime = System.currentTimeMillis();
326-
do {
327-
KeepAliveEntry e = pop();
328-
if ((currentTime - e.idleStartTime) > nap) {
329-
e.hc.closeServer();
330-
} else {
331-
hc = e.hc;
332-
if (KeepAliveCache.logger.isLoggable(PlatformLogger.Level.FINEST)) {
333-
String msg = "cached HttpClient was idle for "
334-
+ Long.toString(currentTime - e.idleStartTime);
335-
KeepAliveCache.logger.finest(msg);
336-
}
337-
}
338-
} while ((hc == null) && (!isEmpty()));
339-
return hc;
340-
} finally {
341-
unlock();
342-
}
343-
}
344-
345-
/* return a still valid, unused HttpClient */
346-
void put(HttpClient h) {
347-
lock();
348-
try {
349-
if (size() >= KeepAliveCache.getMaxConnections()) {
350-
h.closeServer(); // otherwise the connection remains in limbo
329+
if ((currentTime - e.idleStartTime) > nap) {
330+
return null; // all connections stale - will be cleaned up later
351331
} else {
352-
push(new KeepAliveEntry(h, System.currentTimeMillis()));
332+
pollFirst();
333+
if (KeepAliveCache.logger.isLoggable(PlatformLogger.Level.FINEST)) {
334+
String msg = "cached HttpClient was idle for "
335+
+ Long.toString(currentTime - e.idleStartTime);
336+
KeepAliveCache.logger.finest(msg);
337+
}
338+
return e.hc;
353339
}
354340
} finally {
355341
unlock();
356342
}
357343
}
358344

359-
/* remove an HttpClient */
360-
boolean remove(HttpClient h) {
345+
HttpClient put(HttpClient h) {
346+
HttpClient staleClient = null;
361347
lock();
362348
try {
363-
for (KeepAliveEntry curr : this) {
364-
if (curr.hc == h) {
365-
return super.remove(curr);
366-
}
349+
assert KeepAliveCache.getMaxConnections() > 0;
350+
if (size() >= KeepAliveCache.getMaxConnections()) {
351+
// remove oldest connection
352+
staleClient = removeLast().hc;
367353
}
368-
return false;
354+
addFirst(new KeepAliveEntry(h, System.currentTimeMillis()));
369355
} finally {
370356
unlock();
371357
}
358+
// close after releasing the locks
359+
return staleClient;
372360
}
373361

374362
final void lock() {
@@ -396,10 +384,10 @@ private void readObject(ObjectInputStream stream)
396384
}
397385

398386
class KeepAliveKey {
399-
private String protocol = null;
400-
private String host = null;
401-
private int port = 0;
402-
private Object obj = null; // additional key, such as socketfactory
387+
private final String protocol;
388+
private final String host;
389+
private final int port;
390+
private final Object obj; // additional key, such as socketfactory
403391

404392
/**
405393
* Constructor
@@ -440,8 +428,8 @@ public int hashCode() {
440428
}
441429

442430
class KeepAliveEntry {
443-
HttpClient hc;
444-
long idleStartTime;
431+
final HttpClient hc;
432+
final long idleStartTime;
445433

446434
KeepAliveEntry(HttpClient hc, long idleStartTime) {
447435
this.hc = hc;

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

+9
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,15 @@ protected Socket createSocket() throws IOException {
434434
}
435435
}
436436

437+
@Override
438+
public void closeServer() {
439+
try {
440+
// SSLSocket.close may block up to timeout. Make sure it's short.
441+
serverSocket.setSoTimeout(1);
442+
} catch (Exception e) {}
443+
super.closeServer();
444+
}
445+
437446

438447
@Override
439448
public boolean needsTunneling() {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,238 @@
1+
/*
2+
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/*
25+
* @test
26+
* @bug 8293562
27+
* @library /test/lib
28+
* @run main/othervm -Dhttp.keepAlive.time.server=1 B8293562
29+
* @summary Http keep-alive thread should close sockets without holding a lock
30+
*/
31+
32+
import com.sun.net.httpserver.HttpServer;
33+
34+
import javax.net.ssl.HandshakeCompletedListener;
35+
import javax.net.ssl.HttpsURLConnection;
36+
import javax.net.ssl.SSLSession;
37+
import javax.net.ssl.SSLSocket;
38+
import javax.net.ssl.SSLSocketFactory;
39+
import java.io.FileNotFoundException;
40+
import java.io.IOException;
41+
import java.io.InputStream;
42+
import java.net.InetAddress;
43+
import java.net.InetSocketAddress;
44+
import java.net.Proxy;
45+
import java.net.Socket;
46+
import java.net.URL;
47+
import java.net.UnknownHostException;
48+
import java.util.concurrent.CompletableFuture;
49+
import java.util.concurrent.CountDownLatch;
50+
import java.util.concurrent.Executors;
51+
import java.util.concurrent.TimeUnit;
52+
53+
public class B8293562 {
54+
static HttpServer server;
55+
static CountDownLatch closing = new CountDownLatch(1);
56+
static CountDownLatch secondRequestDone = new CountDownLatch(1);
57+
static CompletableFuture<Void> result = new CompletableFuture<>();
58+
59+
public static void main(String[] args) throws Exception {
60+
startHttpServer();
61+
clientHttpCalls();
62+
}
63+
64+
public static void startHttpServer() throws Exception {
65+
server = HttpServer.create(new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 10);
66+
server.setExecutor(Executors.newCachedThreadPool());
67+
server.start();
68+
}
69+
70+
public static void clientHttpCalls() throws Exception {
71+
try {
72+
System.out.println("http server listen on: " + server.getAddress().getPort());
73+
String hostAddr = InetAddress.getLoopbackAddress().getHostAddress();
74+
if (hostAddr.indexOf(':') > -1) hostAddr = "[" + hostAddr + "]";
75+
String baseURLStr = "https://" + hostAddr + ":" + server.getAddress().getPort() + "/";
76+
77+
URL testUrl = new URL (baseURLStr);
78+
79+
// SlowCloseSocketFactory is not a real SSLSocketFactory;
80+
// it produces regular non-SSL sockets. Effectively, the request
81+
// is made over http.
82+
HttpsURLConnection.setDefaultSSLSocketFactory(new SlowCloseSocketFactory());
83+
System.out.println("Performing first request");
84+
HttpsURLConnection uc = (HttpsURLConnection)testUrl.openConnection(Proxy.NO_PROXY);
85+
byte[] buf = new byte[1024];
86+
try {
87+
uc.getInputStream();
88+
throw new RuntimeException("Expected 404 here");
89+
} catch (FileNotFoundException ignored) { }
90+
try (InputStream is = uc.getErrorStream()) {
91+
while (is.read(buf) >= 0) {
92+
}
93+
}
94+
System.out.println("First request completed");
95+
closing.await();
96+
// KeepAliveThread is closing the connection now
97+
System.out.println("Performing second request");
98+
HttpsURLConnection uc2 = (HttpsURLConnection)testUrl.openConnection(Proxy.NO_PROXY);
99+
100+
try {
101+
uc2.getInputStream();
102+
throw new RuntimeException("Expected 404 here");
103+
} catch (FileNotFoundException ignored) { }
104+
try (InputStream is = uc2.getErrorStream()) {
105+
while (is.read(buf) >= 0) {
106+
}
107+
}
108+
System.out.println("Second request completed");
109+
// let the socket know it can close now
110+
secondRequestDone.countDown();
111+
result.get();
112+
System.out.println("Test completed successfully");
113+
} finally {
114+
server.stop(1);
115+
}
116+
}
117+
118+
static class SlowCloseSocket extends SSLSocket {
119+
@Override
120+
public synchronized void close() throws IOException {
121+
String threadName = Thread.currentThread().getName();
122+
System.out.println("Connection closing, thread name: " + threadName);
123+
closing.countDown();
124+
super.close();
125+
if (threadName.equals("Keep-Alive-Timer")) {
126+
try {
127+
if (secondRequestDone.await(5, TimeUnit.SECONDS)) {
128+
result.complete(null);
129+
} else {
130+
result.completeExceptionally(new RuntimeException(
131+
"Wait for second request timed out"));
132+
}
133+
} catch (InterruptedException e) {
134+
result.completeExceptionally(new RuntimeException(
135+
"Wait for second request was interrupted"));
136+
}
137+
} else {
138+
result.completeExceptionally(new RuntimeException(
139+
"Close invoked from unexpected thread"));
140+
}
141+
System.out.println("Connection closed");
142+
}
143+
144+
// required abstract method overrides
145+
@Override
146+
public String[] getSupportedCipherSuites() {
147+
return new String[0];
148+
}
149+
@Override
150+
public String[] getEnabledCipherSuites() {
151+
return new String[0];
152+
}
153+
@Override
154+
public void setEnabledCipherSuites(String[] suites) { }
155+
@Override
156+
public String[] getSupportedProtocols() {
157+
return new String[0];
158+
}
159+
@Override
160+
public String[] getEnabledProtocols() {
161+
return new String[0];
162+
}
163+
@Override
164+
public void setEnabledProtocols(String[] protocols) { }
165+
@Override
166+
public SSLSession getSession() {
167+
return null;
168+
}
169+
@Override
170+
public void addHandshakeCompletedListener(HandshakeCompletedListener listener) { }
171+
@Override
172+
public void removeHandshakeCompletedListener(HandshakeCompletedListener listener) { }
173+
@Override
174+
public void startHandshake() throws IOException { }
175+
@Override
176+
public void setUseClientMode(boolean mode) { }
177+
@Override
178+
public boolean getUseClientMode() {
179+
return false;
180+
}
181+
@Override
182+
public void setNeedClientAuth(boolean need) { }
183+
@Override
184+
public boolean getNeedClientAuth() {
185+
return false;
186+
}
187+
@Override
188+
public void setWantClientAuth(boolean want) { }
189+
@Override
190+
public boolean getWantClientAuth() {
191+
return false;
192+
}
193+
@Override
194+
public void setEnableSessionCreation(boolean flag) { }
195+
@Override
196+
public boolean getEnableSessionCreation() {
197+
return false;
198+
}
199+
}
200+
201+
static class SlowCloseSocketFactory extends SSLSocketFactory {
202+
203+
@Override
204+
public Socket createSocket() throws IOException {
205+
return new SlowCloseSocket();
206+
}
207+
// required abstract method overrides
208+
@Override
209+
public Socket createSocket(String host, int port) throws IOException, UnknownHostException {
210+
throw new UnsupportedOperationException();
211+
}
212+
@Override
213+
public Socket createSocket(String host, int port, InetAddress localHost, int localPort) throws IOException, UnknownHostException {
214+
throw new UnsupportedOperationException();
215+
}
216+
@Override
217+
public Socket createSocket(InetAddress host, int port) throws IOException {
218+
throw new UnsupportedOperationException();
219+
}
220+
@Override
221+
public Socket createSocket(InetAddress address, int port, InetAddress localAddress, int localPort) throws IOException {
222+
throw new UnsupportedOperationException();
223+
}
224+
@Override
225+
public String[] getDefaultCipherSuites() {
226+
return new String[0];
227+
}
228+
@Override
229+
public String[] getSupportedCipherSuites() {
230+
return new String[0];
231+
}
232+
@Override
233+
public Socket createSocket(Socket s, String host, int port, boolean autoClose) throws IOException {
234+
throw new UnsupportedOperationException();
235+
}
236+
}
237+
}
238+

0 commit comments

Comments
 (0)
Please sign in to comment.