8221395: HttpClient leaving connections in CLOSE_WAIT state until Java process ends
Summary: When a non WebSocket connection is not returned to the pool, it needs to be closed even if HttpConnection::isOpen yields false.
Reviewed-by: chegar, michaelm
--- a/src/java.net.http/share/classes/jdk/internal/net/http/HttpConnection.java Thu Mar 28 11:08:23 2019 +0100
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/HttpConnection.java Thu Mar 28 12:16:36 2019 +0000
@@ -317,14 +317,13 @@
void closeOrReturnToCache(HttpHeaders hdrs) {
if (hdrs == null) {
// the connection was closed by server, eof
+ Log.logTrace("Cannot return connection to pool: closing {0}", this);
close();
return;
}
- if (!isOpen()) {
- return;
- }
HttpClientImpl client = client();
if (client == null) {
+ Log.logTrace("Client released: closing {0}", this);
close();
return;
}
@@ -333,10 +332,12 @@
.map((s) -> !s.equalsIgnoreCase("close"))
.orElse(true);
- if (keepAlive) {
+ if (keepAlive && isOpen()) {
Log.logTrace("Returning connection to the pool: {0}", this);
pool.returnToPool(this);
} else {
+ Log.logTrace("Closing connection (keepAlive={0}, isOpen={1}): {2}",
+ keepAlive, isOpen(), this);
close();
}
}
--- a/test/jdk/java/net/httpclient/whitebox/ConnectionPoolTestDriver.java Thu Mar 28 11:08:23 2019 +0100
+++ b/test/jdk/java/net/httpclient/whitebox/ConnectionPoolTestDriver.java Thu Mar 28 12:16:36 2019 +0000
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2019, 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
@@ -23,7 +23,7 @@
/*
* @test
- * @bug 8187044 8187111
+ * @bug 8187044 8187111 8221395
* @summary Verifies that the ConnectionPool correctly handle
* connection deadlines and purges the right connections
* from the cache.
@@ -35,4 +35,8 @@
* @run main/othervm
* --add-reads java.net.http=java.management
* java.net.http/jdk.internal.net.http.ConnectionPoolTest testPoolSize
+ * @run main/othervm
+ * --add-reads java.net.http=java.management
+ * -Djdk.httpclient.HttpClient.log=errors,requests,headers,content,ssl,trace,channel
+ * java.net.http/jdk.internal.net.http.ConnectionPoolTest testCloseOrReturnToPool
*/
--- a/test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/ConnectionPoolTest.java Thu Mar 28 11:08:23 2019 +0100
+++ b/test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/ConnectionPoolTest.java Thu Mar 28 12:16:36 2019 +0000
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2019, 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
@@ -25,17 +25,25 @@
import java.io.IOException;
import java.lang.management.ManagementFactory;
+import java.lang.ref.Reference;
import java.net.Authenticator;
import java.net.CookieHandler;
import java.net.InetSocketAddress;
import java.net.ProxySelector;
+import java.net.Socket;
+import java.net.SocketAddress;
+import java.net.SocketOption;
+import java.net.http.HttpHeaders;
import java.nio.ByteBuffer;
import java.nio.channels.SocketChannel;
+import java.nio.channels.spi.SelectorProvider;
import java.time.Duration;
-import java.util.Arrays;
+import java.util.HashMap;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
import java.util.Random;
+import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Executor;
import java.util.concurrent.Flow;
@@ -53,7 +61,7 @@
* @summary Verifies that the ConnectionPool correctly handle
* connection deadlines and purges the right connections
* from the cache.
- * @bug 8187044 8187111
+ * @bug 8187044 8187111 8221395
* @author danielfuchs
*/
public class ConnectionPoolTest {
@@ -78,7 +86,10 @@
} else if ("testPoolSize".equals(arg)) {
assert args.length == 1 : "testPoolSize should be run in its own VM";
testPoolSize();
- }
+ } else if ("testCloseOrReturnToPool".equals(arg)) {
+ assert args.length == 1 : "testCloseOrReturnToPool should be run in its own VM";
+ testCloseOrReturnToPool();
+ } else throw new RuntimeException("unknown test case: " + arg);
}
}
@@ -226,6 +237,113 @@
}
}
+ public static void testCloseOrReturnToPool() throws Exception {
+ HttpClientFacade facade = (HttpClientFacade)HttpClient.newHttpClient();
+ HttpClientImpl client = facade.impl;
+ ConnectionPool pool = client.connectionPool();
+ InetSocketAddress proxy = InetSocketAddress.createUnresolved("bar", 80);
+
+ InetSocketAddress addr = InetSocketAddress.createUnresolved("foo1", 80);
+ HttpConnectionStub conn1 = new HttpConnectionStub(facade, client, addr, proxy, true);
+ HttpHeaders hdrs = HttpHeaders.of(new HashMap<>(), (s1,s2) -> true);
+ HttpConnection conn;
+
+ conn1.reopen();
+ if (!conn1.isOpen()) {
+ throw new RuntimeException("conn1 finished");
+ }
+
+ conn1.closeOrReturnToCache(hdrs);
+
+ // Check we can find conn1 in the pool
+ if (conn1 != (conn = pool.getConnection(true, addr, proxy))) {
+ throw new RuntimeException("conn1 not returned, got: " + conn);
+ }
+ System.out.println("Found connection in the pool: " + conn );
+
+ // Try to return it with no headers: the connection should
+ // be closed and not returned to the pool (EOF).
+ conn.closeOrReturnToCache(null);
+ if ((conn = pool.getConnection(true, addr, proxy)) != null) {
+ throw new RuntimeException(conn + " found in the pool!");
+ }
+ if (!conn1.closed) {
+ throw new RuntimeException("conn1 not closed!");
+ }
+ System.out.println("EOF connection successfully closed when returned to pool");
+
+ // reopen the connection
+ conn1.reopen();
+ if (!conn1.isOpen()) {
+ throw new RuntimeException("conn1 finished");
+ }
+
+ // Try to return it with empty headers: the connection should
+ // be returned to the pool.
+ conn1.closeOrReturnToCache(hdrs);
+ if (conn1 != (conn = pool.getConnection(true, addr, proxy))) {
+ throw new RuntimeException("conn1 not returned to pool, got: " + conn);
+ }
+ if (conn1.closed) {
+ throw new RuntimeException("conn1 closed");
+ }
+ if (!conn1.isOpen()) {
+ throw new RuntimeException("conn1 finished");
+ }
+
+ System.out.println("Keep alive connection successfully returned to pool");
+
+ // Try to return it with connection: close headers: the connection should
+ // not be returned to the pool, and should be closed.
+ HttpHeaders hdrs2 = HttpHeaders.of(Map.of("connection", List.of("close")), (s1, s2) -> true);
+ conn1.closeOrReturnToCache(hdrs2);
+ if ((conn = pool.getConnection(true, addr, proxy)) != null) {
+ throw new RuntimeException(conn + " found in the pool!");
+ }
+ if (!conn1.closed) {
+ throw new RuntimeException("conn1 not closed!");
+ }
+ System.out.println("Close connection successfully closed when returned to pool");
+
+ // reopen and finish the connection.
+ conn1.reopen();
+ conn1.finish(true);
+ if (conn1.closed) {
+ throw new RuntimeException("conn1 closed");
+ }
+ if (conn1.isOpen()) {
+ throw new RuntimeException("conn1 is opened!");
+ }
+ conn1.closeOrReturnToCache(hdrs2);
+ if ((conn = pool.getConnection(true, addr, proxy)) != null) {
+ throw new RuntimeException(conn + " found in the pool!");
+ }
+ if (!conn1.closed) {
+ throw new RuntimeException("conn1 not closed!");
+ }
+ System.out.println("Finished 'close' connection successfully closed when returned to pool");
+
+ // reopen and finish the connection.
+ conn1.reopen();
+ conn1.finish(true);
+ if (conn1.closed) {
+ throw new RuntimeException("conn1 closed");
+ }
+ if (conn1.isOpen()) {
+ throw new RuntimeException("conn1 is opened!");
+ }
+ conn1.closeOrReturnToCache(hdrs);
+ if ((conn = pool.getConnection(true, addr, proxy)) != null) {
+ throw new RuntimeException(conn + " found in the pool!");
+ }
+ if (!conn1.closed) {
+ throw new RuntimeException("conn1 not closed!");
+ }
+ System.out.println("Finished keep-alive connection successfully closed when returned to pool");
+
+ Reference.reachabilityFence(facade);
+ }
+
static <T> T error() {
throw new InternalError("Should not reach here: wrong test assumptions!");
}
@@ -241,22 +359,108 @@
@Override
public void subscribe(Flow.Subscriber<? super List<ByteBuffer>> subscriber) {
}
- @Override public boolean isFinished() { return conn.closed; }
+ @Override public boolean isFinished() { return conn.finished; }
+ }
+
+ static class SocketChannelStub extends SocketChannel {
+
+ SocketChannelStub() { super(SelectorProvider.provider()); }
+
+ @Override
+ public SocketChannel bind(SocketAddress local) throws IOException {
+ return error();
+ }
+ @Override
+ public <T> SocketChannel setOption(SocketOption<T> name, T value) throws IOException {
+ return error();
+ }
+ @Override
+ public SocketChannel shutdownInput() throws IOException {
+ return error();
+ }
+ @Override
+ public SocketChannel shutdownOutput() throws IOException {
+ return error();
+ }
+ @Override
+ public Socket socket() { return error(); }
+ @Override
+ public boolean isConnected() { return true; }
+ @Override
+ public boolean isConnectionPending() { return false; }
+ @Override
+ public boolean connect(SocketAddress remote) throws IOException {
+ return error();
+ }
+ @Override
+ public boolean finishConnect() throws IOException {
+ return error();
+ }
+ @Override
+ public SocketAddress getRemoteAddress() throws IOException {
+ return error();
+ }
+ @Override
+ public int read(ByteBuffer dst) throws IOException {
+ return error();
+ }
+ @Override
+ public long read(ByteBuffer[] dsts, int offset, int length) throws IOException {
+ return error();
+ }
+ @Override
+ public int write(ByteBuffer src) throws IOException {
+ return error();
+ }
+ @Override
+ public long write(ByteBuffer[] srcs, int offset, int length) throws IOException {
+ return 0;
+ }
+ @Override
+ public SocketAddress getLocalAddress() throws IOException {
+ return error();
+ }
+ @Override
+ public <T> T getOption(SocketOption<T> name) throws IOException {
+ return error();
+ }
+ @Override
+ public Set<SocketOption<?>> supportedOptions() {
+ return error();
+ }
+ @Override
+ protected void implCloseSelectableChannel() throws IOException {
+ error();
+ }
+ @Override
+ protected void implConfigureBlocking(boolean block) throws IOException {
+ error();
+ }
}
// Emulates an HttpConnection that has a strong reference to its HttpClient.
static class HttpConnectionStub extends HttpConnection {
- public HttpConnectionStub(HttpClient client,
+ public HttpConnectionStub(
+ HttpClient client,
InetSocketAddress address,
InetSocketAddress proxy,
boolean secured) {
- super(address, null);
+ this(client, null, address, proxy, secured);
+ }
+ public HttpConnectionStub(
+ HttpClient client,
+ HttpClientImpl impl,
+ InetSocketAddress address,
+ InetSocketAddress proxy,
+ boolean secured) {
+ super(address, impl);
this.key = ConnectionPool.cacheKey(address, proxy);
this.address = address;
this.proxy = proxy;
this.secured = secured;
this.client = client;
+ this.channel = new SocketChannelStub();
this.flow = new FlowTubeStub(this);
}
@@ -266,16 +470,23 @@
final ConnectionPool.CacheKey key;
final HttpClient client;
final FlowTubeStub flow;
- volatile boolean closed;
+ final SocketChannel channel;
+ volatile boolean closed, finished;
+
+ // Used for testing closeOrReturnToPool.
+ void finish(boolean finished) { this.finished = finished; }
+ void reopen() { closed = finished = false;}
// All these return something
@Override boolean connected() {return !closed;}
@Override boolean isSecure() {return secured;}
@Override boolean isProxied() {return proxy!=null;}
@Override ConnectionPool.CacheKey cacheKey() {return key;}
+ @Override FlowTube getConnectionFlow() {return flow;}
+ @Override SocketChannel channel() {return channel;}
@Override
public void close() {
- closed=true;
+ closed=finished=true;
System.out.println("closed: " + this);
}
@Override
@@ -283,13 +494,11 @@
return "HttpConnectionStub: " + address + " proxy: " + proxy;
}
+
// All these throw errors
@Override public HttpPublisher publisher() {return error();}
@Override public CompletableFuture<Void> connectAsync(Exchange<?> e) {return error();}
@Override public CompletableFuture<Void> finishConnect() {return error();}
- @Override SocketChannel channel() {return error();}
- @Override
- FlowTube getConnectionFlow() {return flow;}
}
// Emulates an HttpClient that has a strong reference to its connection pool.
static class HttpClientStub extends HttpClient {