8221395: HttpClient leaving connections in CLOSE_WAIT state until Java process ends

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

src/java.net.http/share/classes/jdk/internal/net/http/HttpConnection.java

@@ -317,14 +317,13 @@ abstract class HttpConnection implements Closeable {
     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 @@ abstract class HttpConnection implements Closeable {
                 .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();
         }
     }

test/jdk/java/net/httpclient/whitebox/ConnectionPoolTestDriver.java

@@ -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
  */

test/jdk/java/net/httpclient/whitebox/java.net.http/jdk/internal/net/http/ConnectionPoolTest.java

@@ -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 @@ package jdk.internal.net.http;
 
 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 @@ import jdk.internal.net.http.common.FlowTube;
  * @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 @@ public class ConnectionPoolTest {
             } 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 class ConnectionPoolTest {
         }
     }
 
+    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 @@ public class ConnectionPoolTest {
         @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 @@ public class ConnectionPoolTest {
         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 @@ public class ConnectionPoolTest {
             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 {