HTTPCLIENT-2177: keep successful tunnel connections alive regardless of `Connection: close`

httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultClientConnectionReuseStrategy.java

@@ -0,0 +1,57 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl;
+
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.impl.DefaultConnectionReuseStrategy;
+import org.apache.hc.core5.http.protocol.HttpContext;
+
+/**
+ * Extension of core {@link DefaultConnectionReuseStrategy} that treats
+ * CONNECT method exchnages involved in proxy tunnelling as a special case.
+ *
+ * @since 5.2
+ */
+@Contract(threading = ThreadingBehavior.STATELESS)
+public class DefaultClientConnectionReuseStrategy extends DefaultConnectionReuseStrategy {
+
+    public static final DefaultClientConnectionReuseStrategy INSTANCE = new DefaultClientConnectionReuseStrategy();
+
+    @Override
+    public boolean keepAlive(final HttpRequest request, final HttpResponse response, final HttpContext context) {
+        if (Method.CONNECT.isSame(request.getMethod()) && response.getCode() == HttpStatus.SC_OK) {
+            return true;
+        }
+        return super.keepAlive(request, response, context);
+    }
+
+}

httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java

@@ -53,6 +53,7 @@ import org.apache.hc.client5.http.cookie.CookieStore;
 import org.apache.hc.client5.http.impl.ChainElement;
 import org.apache.hc.client5.http.impl.CookieSpecSupport;
 import org.apache.hc.client5.http.impl.DefaultAuthenticationStrategy;
+import org.apache.hc.client5.http.impl.DefaultClientConnectionReuseStrategy;
 import org.apache.hc.client5.http.impl.DefaultConnectionKeepAliveStrategy;
 import org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy;
 import org.apache.hc.client5.http.impl.DefaultRedirectStrategy;
@@ -92,7 +93,6 @@ import org.apache.hc.core5.http.config.Http1Config;
 import org.apache.hc.core5.http.config.Lookup;
 import org.apache.hc.core5.http.config.NamedElementChain;
 import org.apache.hc.core5.http.config.RegistryBuilder;
-import org.apache.hc.core5.http.impl.DefaultConnectionReuseStrategy;
 import org.apache.hc.core5.http.nio.command.ShutdownCommand;
 import org.apache.hc.core5.http.protocol.DefaultHttpProcessor;
 import org.apache.hc.core5.http.protocol.HttpProcessor;
@@ -888,12 +888,12 @@ public class HttpAsyncClientBuilder {
             if (systemProperties) {
                 final String s = getProperty("http.keepAlive", "true");
                 if ("true".equalsIgnoreCase(s)) {
-                    reuseStrategyCopy = DefaultConnectionReuseStrategy.INSTANCE;
+                    reuseStrategyCopy = DefaultClientConnectionReuseStrategy.INSTANCE;
                 } else {
                     reuseStrategyCopy = (request, response, context) -> false;
                 }
             } else {
-                reuseStrategyCopy = DefaultConnectionReuseStrategy.INSTANCE;
+                reuseStrategyCopy = DefaultClientConnectionReuseStrategy.INSTANCE;
             }
         }
         final AsyncPushConsumerRegistry pushConsumerRegistry = new AsyncPushConsumerRegistry();

httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientEventHandlerFactory.java

@@ -31,6 +31,7 @@ import java.io.IOException;
 import java.util.Iterator;
 import java.util.List;
 
+import org.apache.hc.client5.http.impl.DefaultClientConnectionReuseStrategy;
 import org.apache.hc.core5.http.ConnectionReuseStrategy;
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpConnection;
@@ -38,7 +39,6 @@ import org.apache.hc.core5.http.HttpRequest;
 import org.apache.hc.core5.http.HttpResponse;
 import org.apache.hc.core5.http.config.CharCodingConfig;
 import org.apache.hc.core5.http.config.Http1Config;
-import org.apache.hc.core5.http.impl.DefaultConnectionReuseStrategy;
 import org.apache.hc.core5.http.impl.Http1StreamListener;
 import org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexerFactory;
 import org.apache.hc.core5.http.impl.nio.DefaultHttpRequestWriterFactory;
@@ -96,7 +96,7 @@ class HttpAsyncClientEventHandlerFactory implements IOEventHandlerFactory {
         this.h2Config = h2Config != null ? h2Config : H2Config.DEFAULT;
         this.h1Config = h1Config != null ? h1Config : Http1Config.DEFAULT;
         this.charCodingConfig = charCodingConfig != null ? charCodingConfig : CharCodingConfig.DEFAULT;
-        this.http1ConnectionReuseStrategy = connectionReuseStrategy != null ? connectionReuseStrategy : DefaultConnectionReuseStrategy.INSTANCE;
+        this.http1ConnectionReuseStrategy = connectionReuseStrategy != null ? connectionReuseStrategy : DefaultClientConnectionReuseStrategy.INSTANCE;
         this.http1ResponseParserFactory = new DefaultHttpResponseParserFactory(h1Config);
         this.http1RequestWriterFactory = DefaultHttpRequestWriterFactory.INSTANCE;
     }

httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClients.java

@@ -30,6 +30,7 @@ package org.apache.hc.client5.http.impl.async;
 import org.apache.hc.client5.http.DnsResolver;
 import org.apache.hc.client5.http.SchemePortResolver;
 import org.apache.hc.client5.http.SystemDefaultDnsResolver;
+import org.apache.hc.client5.http.impl.DefaultClientConnectionReuseStrategy;
 import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
 import org.apache.hc.client5.http.impl.nio.PoolingAsyncClientConnectionManagerBuilder;
 import org.apache.hc.client5.http.nio.AsyncClientConnectionManager;
@@ -37,7 +38,6 @@ import org.apache.hc.client5.http.ssl.DefaultClientTlsStrategy;
 import org.apache.hc.core5.concurrent.DefaultThreadFactory;
 import org.apache.hc.core5.http.config.CharCodingConfig;
 import org.apache.hc.core5.http.config.Http1Config;
-import org.apache.hc.core5.http.impl.DefaultConnectionReuseStrategy;
 import org.apache.hc.core5.http.nio.ssl.TlsStrategy;
 import org.apache.hc.core5.http.protocol.DefaultHttpProcessor;
 import org.apache.hc.core5.http.protocol.HttpProcessor;
@@ -157,7 +157,7 @@ public final class HttpAsyncClients {
                         h2Config,
                         h1Config,
                         CharCodingConfig.DEFAULT,
-                        DefaultConnectionReuseStrategy.INSTANCE),
+                        DefaultClientConnectionReuseStrategy.INSTANCE),
                 pushConsumerRegistry,
                 versionPolicy,
                 ioReactorConfig,

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ConnectExec.java

@@ -227,22 +227,23 @@ public final class ConnectExec implements ExecChainHandler {
                 throw new HttpException("Unexpected response to CONNECT request: " + new StatusLine(response));
             }
 
+            if (this.reuseStrategy.keepAlive(connect, response, context)) {
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug("{} connection kept alive", exchangeId);
+                }
+                // Consume response content
+                final HttpEntity entity = response.getEntity();
+                EntityUtils.consume(entity);
+            } else {
+                execRuntime.disconnectEndpoint();
+            }
+
             if (config.isAuthenticationEnabled()) {
                 if (this.authenticator.isChallenged(proxy, ChallengeType.PROXY, response,
                         proxyAuthExchange, context)) {
                     if (this.authenticator.updateAuthState(proxy, ChallengeType.PROXY, response,
                             this.proxyAuthStrategy, proxyAuthExchange, context)) {
                         // Retry request
-                        if (this.reuseStrategy.keepAlive(request, response, context)) {
-                            if (LOG.isDebugEnabled()) {
-                                LOG.debug("{} connection kept alive", exchangeId);
-                            }
-                            // Consume response content
-                            final HttpEntity entity = response.getEntity();
-                            EntityUtils.consume(entity);
-                        } else {
-                            execRuntime.disconnectEndpoint();
-                        }
                         response = null;
                     }
                 }
@@ -250,7 +251,7 @@ public final class ConnectExec implements ExecChainHandler {
         }
 
         final int status = response.getCode();
-        if (status >= HttpStatus.SC_REDIRECTION) {
+        if (status != HttpStatus.SC_OK) {
 
             // Buffer response content
             final HttpEntity entity = response.getEntity();

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java

@@ -55,6 +55,7 @@ import org.apache.hc.client5.http.entity.InputStreamFactory;
 import org.apache.hc.client5.http.impl.ChainElement;
 import org.apache.hc.client5.http.impl.CookieSpecSupport;
 import org.apache.hc.client5.http.impl.DefaultAuthenticationStrategy;
+import org.apache.hc.client5.http.impl.DefaultClientConnectionReuseStrategy;
 import org.apache.hc.client5.http.impl.DefaultConnectionKeepAliveStrategy;
 import org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy;
 import org.apache.hc.client5.http.impl.DefaultRedirectStrategy;
@@ -92,7 +93,6 @@ import org.apache.hc.core5.http.config.Lookup;
 import org.apache.hc.core5.http.config.NamedElementChain;
 import org.apache.hc.core5.http.config.Registry;
 import org.apache.hc.core5.http.config.RegistryBuilder;
-import org.apache.hc.core5.http.impl.DefaultConnectionReuseStrategy;
 import org.apache.hc.core5.http.impl.io.HttpRequestExecutor;
 import org.apache.hc.core5.http.protocol.DefaultHttpProcessor;
 import org.apache.hc.core5.http.protocol.HttpProcessor;
@@ -742,12 +742,12 @@ public class HttpClientBuilder {
             if (systemProperties) {
                 final String s = System.getProperty("http.keepAlive", "true");
                 if ("true".equalsIgnoreCase(s)) {
-                    reuseStrategyCopy = DefaultConnectionReuseStrategy.INSTANCE;
+                    reuseStrategyCopy = DefaultClientConnectionReuseStrategy.INSTANCE;
                 } else {
                     reuseStrategyCopy = (request, response, context) -> false;
                 }
             } else {
-                reuseStrategyCopy = DefaultConnectionReuseStrategy.INSTANCE;
+                reuseStrategyCopy = DefaultClientConnectionReuseStrategy.INSTANCE;
             }
         }
 

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/MinimalHttpClient.java

@@ -37,6 +37,7 @@ import org.apache.hc.client5.http.classic.ExecRuntime;
 import org.apache.hc.client5.http.config.Configurable;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.impl.ConnectionShutdownException;
+import org.apache.hc.client5.http.impl.DefaultClientConnectionReuseStrategy;
 import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
 import org.apache.hc.client5.http.impl.ExecSupport;
 import org.apache.hc.client5.http.io.HttpClientConnectionManager;
@@ -52,7 +53,6 @@ import org.apache.hc.core5.http.ConnectionReuseStrategy;
 import org.apache.hc.core5.http.HttpEntity;
 import org.apache.hc.core5.http.HttpException;
 import org.apache.hc.core5.http.HttpHost;
-import org.apache.hc.core5.http.impl.DefaultConnectionReuseStrategy;
 import org.apache.hc.core5.http.impl.io.HttpRequestExecutor;
 import org.apache.hc.core5.http.protocol.BasicHttpContext;
 import org.apache.hc.core5.http.protocol.DefaultHttpProcessor;
@@ -96,7 +96,7 @@ public class MinimalHttpClient extends CloseableHttpClient {
     MinimalHttpClient(final HttpClientConnectionManager connManager) {
         super();
         this.connManager = Args.notNull(connManager, "HTTP connection manager");
-        this.reuseStrategy = DefaultConnectionReuseStrategy.INSTANCE;
+        this.reuseStrategy = DefaultClientConnectionReuseStrategy.INSTANCE;
         this.schemePortResolver = DefaultSchemePortResolver.INSTANCE;
         this.requestExecutor = new HttpRequestExecutor(this.reuseStrategy);
         this.httpProcessor = new DefaultHttpProcessor(

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProxyClient.java

@@ -36,12 +36,13 @@ import org.apache.hc.client5.http.RouteInfo.LayerType;
 import org.apache.hc.client5.http.RouteInfo.TunnelType;
 import org.apache.hc.client5.http.auth.AuthExchange;
 import org.apache.hc.client5.http.auth.AuthSchemeFactory;
-import org.apache.hc.client5.http.auth.StandardAuthScheme;
 import org.apache.hc.client5.http.auth.AuthScope;
 import org.apache.hc.client5.http.auth.ChallengeType;
 import org.apache.hc.client5.http.auth.Credentials;
+import org.apache.hc.client5.http.auth.StandardAuthScheme;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.client5.http.impl.DefaultAuthenticationStrategy;
+import org.apache.hc.client5.http.impl.DefaultClientConnectionReuseStrategy;
 import org.apache.hc.client5.http.impl.TunnelRefusedException;
 import org.apache.hc.client5.http.impl.auth.BasicCredentialsProvider;
 import org.apache.hc.client5.http.impl.auth.BasicSchemeFactory;
@@ -66,7 +67,6 @@ import org.apache.hc.core5.http.config.CharCodingConfig;
 import org.apache.hc.core5.http.config.Http1Config;
 import org.apache.hc.core5.http.config.Lookup;
 import org.apache.hc.core5.http.config.RegistryBuilder;
-import org.apache.hc.core5.http.impl.DefaultConnectionReuseStrategy;
 import org.apache.hc.core5.http.impl.io.HttpRequestExecutor;
 import org.apache.hc.core5.http.io.HttpConnectionFactory;
 import org.apache.hc.core5.http.io.entity.EntityUtils;
@@ -125,7 +125,7 @@ public class ProxyClient {
                 .register(StandardAuthScheme.SPNEGO, SPNegoSchemeFactory.DEFAULT)
                 .register(StandardAuthScheme.KERBEROS, KerberosSchemeFactory.DEFAULT)
                 .build();
-        this.reuseStrategy = new DefaultConnectionReuseStrategy();
+        this.reuseStrategy = DefaultClientConnectionReuseStrategy.INSTANCE;
     }
 
     /**

httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestConnectExec.java

@@ -221,7 +221,7 @@ public class TestConnectExec {
         final TunnelRefusedException exception = Assert.assertThrows(TunnelRefusedException.class, () ->
                 exec.execute(request, scope, execChain));
         Assert.assertEquals("Ka-boom", exception.getResponseMessage());
-        Mockito.verify(execRuntime).disconnectEndpoint();
+        Mockito.verify(execRuntime, Mockito.atLeastOnce()).disconnectEndpoint();
         Mockito.verify(execRuntime).discardEndpoint();
     }
 
@@ -246,7 +246,7 @@ public class TestConnectExec {
         Mockito.doAnswer(connectionState.connectAnswer()).when(execRuntime).connectEndpoint(Mockito.any());
         Mockito.when(execRuntime.isEndpointConnected()).thenAnswer(connectionState.isConnectedAnswer());
         Mockito.when(reuseStrategy.keepAlive(
-                Mockito.same(request),
+                Mockito.any(),
                 Mockito.any(),
                 Mockito.<HttpClientContext>any())).thenReturn(Boolean.TRUE);
         Mockito.when(execRuntime.execute(
@@ -257,7 +257,7 @@ public class TestConnectExec {
         Mockito.when(proxyAuthStrategy.select(
                 Mockito.eq(ChallengeType.PROXY),
                 Mockito.any(),
-                Mockito.<HttpClientContext>any())).thenReturn(Collections.singletonList(new BasicScheme()));
+                Mockito.any())).thenReturn(Collections.singletonList(new BasicScheme()));
 
         final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context);
         exec.execute(request, scope, execChain);
@@ -286,10 +286,6 @@ public class TestConnectExec {
         final ConnectionState connectionState = new ConnectionState();
         Mockito.doAnswer(connectionState.connectAnswer()).when(execRuntime).connectEndpoint(Mockito.any());
         Mockito.when(execRuntime.isEndpointConnected()).thenAnswer(connectionState.isConnectedAnswer());
-        Mockito.when(reuseStrategy.keepAlive(
-                Mockito.same(request),
-                Mockito.any(),
-                Mockito.<HttpClientContext>any())).thenReturn(Boolean.FALSE);
         Mockito.when(execRuntime.execute(
                 Mockito.anyString(),
                 Mockito.any(),
@@ -298,14 +294,14 @@ public class TestConnectExec {
         Mockito.when(proxyAuthStrategy.select(
                 Mockito.eq(ChallengeType.PROXY),
                 Mockito.any(),
-                Mockito.<HttpClientContext>any())).thenReturn(Collections.singletonList(new BasicScheme()));
+                Mockito.any())).thenReturn(Collections.singletonList(new BasicScheme()));
 
         final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context);
         exec.execute(request, scope, execChain);
 
         Mockito.verify(execRuntime).connectEndpoint(context);
         Mockito.verify(inStream1, Mockito.never()).close();
-        Mockito.verify(execRuntime).disconnectEndpoint();
+        Mockito.verify(execRuntime, Mockito.atLeastOnce()).disconnectEndpoint();
     }
 
     @Test