HTTPCLIENT-1482: added ExpectContinueTrigger, extended RequestConfig, and modified RequestExpectContinue so that Expect: 100-continue is emitted only when the underlying connection has already processed at least one request; default behaviour (ALWAYS) preserved, API remains binary-compatible. (#683)

Author: arturobernalg

httpclient5/src/main/java/org/apache/hc/client5/http/config/ExpectContinueTrigger.java

@@ -0,0 +1,50 @@
+/*
+ * ====================================================================
+ * 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.config;
+
+/**
+ * Enumeration of strategies that govern automatic inclusion of the
+ * {@code Expect: 100-continue} request header when
+ * {@link org.apache.hc.client5.http.config.RequestConfig#isExpectContinueEnabled()
+ * expect-continue support} is enabled.
+ *
+ * @since 5.6
+ */
+public enum ExpectContinueTrigger {
+
+    /**
+     * Always add {@code Expect: 100-continue} to every entity-enclosing request.
+     */
+    ALWAYS,
+
+    /**
+     * Add {@code Expect: 100-continue} <em>only</em> when the underlying
+     * connection has already processed at least one request (that is, when the
+     * socket has been taken from the connection pool and may be stale).
+     */
+    IF_REUSED
+}

httpclient5/src/main/java/org/apache/hc/client5/http/config/RequestConfig.java

@@ -34,6 +34,7 @@
 import org.apache.hc.core5.annotation.Contract;
 import org.apache.hc.core5.annotation.ThreadingBehavior;
 import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.util.Args;
 import org.apache.hc.core5.util.TimeValue;
 import org.apache.hc.core5.util.Timeout;
 
@@ -66,12 +67,15 @@ public class RequestConfig implements Cloneable {
     private final boolean protocolUpgradeEnabled;
     private final Path unixDomainSocket;
 
+    private final ExpectContinueTrigger expectContinueTrigger;
+
     /**
      * Intended for CDI compatibility
     */
     protected RequestConfig() {
         this(false, null, null, false, false, 0, false, null, null,
-                DEFAULT_CONNECTION_REQUEST_TIMEOUT, null, null, DEFAULT_CONN_KEEP_ALIVE, false, false, false, null);
+                DEFAULT_CONNECTION_REQUEST_TIMEOUT, null, null, DEFAULT_CONN_KEEP_ALIVE, false, false, false, null,
+                ExpectContinueTrigger.ALWAYS);
     }
 
     RequestConfig(
@@ -91,7 +95,8 @@ protected RequestConfig() {
             final boolean contentCompressionEnabled,
             final boolean hardCancellationEnabled,
             final boolean protocolUpgradeEnabled,
-            final Path unixDomainSocket) {
+            final Path unixDomainSocket,
+            final ExpectContinueTrigger expectContinueTrigger) {
         super();
         this.expectContinueEnabled = expectContinueEnabled;
         this.proxy = proxy;
@@ -110,6 +115,7 @@ protected RequestConfig() {
         this.hardCancellationEnabled = hardCancellationEnabled;
         this.protocolUpgradeEnabled = protocolUpgradeEnabled;
         this.unixDomainSocket = unixDomainSocket;
+        this.expectContinueTrigger = expectContinueTrigger;
     }
 
     /**
@@ -238,6 +244,10 @@ public Path getUnixDomainSocket() {
         return unixDomainSocket;
     }
 
+    public ExpectContinueTrigger getExpectContinueTrigger() {
+        return expectContinueTrigger;
+    }
+
     @Override
     protected RequestConfig clone() throws CloneNotSupportedException {
         return (RequestConfig) super.clone();
@@ -312,6 +322,7 @@ public static class Builder {
         private boolean hardCancellationEnabled;
         private boolean protocolUpgradeEnabled;
         private Path unixDomainSocket;
+        private ExpectContinueTrigger expectContinueTrigger;
 
         Builder() {
             super();
@@ -322,6 +333,7 @@ public static class Builder {
             this.contentCompressionEnabled = true;
             this.hardCancellationEnabled = true;
             this.protocolUpgradeEnabled = true;
+            this.expectContinueTrigger = ExpectContinueTrigger.ALWAYS;
         }
 
         /**
@@ -668,6 +680,20 @@ public Builder setUnixDomainSocket(final Path unixDomainSocket) {
             return this;
         }
 
+        /**
+         * Defines under which circumstances the client should add the
+         * {@code Expect: 100-continue} header to entity-enclosing requests.
+         *
+         * @param trigger expectation-continue trigger strategy
+         * @return this builder
+         * @see ExpectContinueTrigger
+         * @since 5.6
+         */
+        public Builder setExpectContinueTrigger(final ExpectContinueTrigger trigger) {
+            this.expectContinueTrigger = Args.notNull(trigger, "ExpectContinueTrigger");
+            return this;
+        }
+
         public RequestConfig build() {
             return new RequestConfig(
                     expectContinueEnabled,
@@ -686,7 +712,8 @@ public RequestConfig build() {
                     contentCompressionEnabled,
                     hardCancellationEnabled,
                     protocolUpgradeEnabled,
-                    unixDomainSocket);
+                    unixDomainSocket,
+                    expectContinueTrigger);
         }
 
     }

httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RequestExpectContinue.java

@@ -29,9 +29,11 @@
 
 import java.io.IOException;
 
+import org.apache.hc.client5.http.config.ExpectContinueTrigger;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.core5.annotation.Contract;
 import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.EndpointDetails;
 import org.apache.hc.core5.http.EntityDetails;
 import org.apache.hc.core5.http.HeaderElements;
 import org.apache.hc.core5.http.HttpException;
@@ -41,6 +43,7 @@
 import org.apache.hc.core5.http.HttpVersion;
 import org.apache.hc.core5.http.ProtocolVersion;
 import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.http.protocol.HttpCoreContext;
 import org.apache.hc.core5.util.Args;
 
 /**
@@ -68,13 +71,20 @@ public void process(final HttpRequest request, final EntityDetails entity, final
         if (!request.containsHeader(HttpHeaders.EXPECT)) {
             final HttpClientContext clientContext = HttpClientContext.cast(context);
             final ProtocolVersion version = request.getVersion() != null ? request.getVersion() : clientContext.getProtocolVersion();
+            final RequestConfig config = clientContext.getRequestConfigOrDefault();
+            if (!config.isExpectContinueEnabled()) {
+                return;
+            }
+            if (config.getExpectContinueTrigger() == ExpectContinueTrigger.IF_REUSED) {
+                final EndpointDetails details = HttpCoreContext.cast(context).getEndpointDetails();
+                if (details != null && details.getRequestCount() == 0) {
+                    return;
+                }
+            }
             // Do not send the expect header if request body is known to be empty
             if (entity != null
                     && entity.getContentLength() != 0 && !version.lessEquals(HttpVersion.HTTP_1_0)) {
-                final RequestConfig config = clientContext.getRequestConfigOrDefault();
-                if (config.isExpectContinueEnabled()) {
-                    request.addHeader(HttpHeaders.EXPECT, HeaderElements.CONTINUE);
-                }
+                request.addHeader(HttpHeaders.EXPECT, HeaderElements.CONTINUE);
             }
         }
     }

httpclient5/src/test/java/org/apache/hc/client5/http/protocol/TestRequestExpectContinue.java

@@ -27,16 +27,21 @@
 
 package org.apache.hc.client5.http.protocol;
 
+import java.net.InetSocketAddress;
 import java.nio.charset.StandardCharsets;
 
+import org.apache.hc.client5.http.config.ExpectContinueTrigger;
 import org.apache.hc.client5.http.config.RequestConfig;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HeaderElements;
+import org.apache.hc.core5.http.HttpConnectionMetrics;
 import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.hc.core5.http.HttpVersion;
+import org.apache.hc.core5.http.impl.BasicEndpointDetails;
 import org.apache.hc.core5.http.io.entity.StringEntity;
 import org.apache.hc.core5.http.message.BasicClassicHttpRequest;
+import org.apache.hc.core5.util.Timeout;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
@@ -119,4 +124,94 @@ void testRequestExpectContinueIgnoreNonenclosingRequests() throws Exception {
         Assertions.assertEquals(0, request.getHeaders().length);
     }
 
+
+    @Test
+    void testRequestExpectContinueIfReused() throws Exception {
+        final HttpClientContext context = HttpClientContext.create();
+        final RequestConfig config = RequestConfig.custom()
+                .setExpectContinueEnabled(true)
+                .setExpectContinueTrigger(ExpectContinueTrigger.IF_REUSED)
+                .build();
+        context.setRequestConfig(config);
+
+        final HttpConnectionMetrics metrics = new HttpConnectionMetrics() {
+            @Override
+            public long getRequestCount() {
+                return 1;
+            }
+
+            @Override
+            public long getResponseCount() {
+                return 0;
+            }
+
+            @Override
+            public long getSentBytesCount() {
+                return 0;
+            }
+
+            @Override
+            public long getReceivedBytesCount() {
+                return 0;
+            }
+        };
+
+        final BasicEndpointDetails reused = new BasicEndpointDetails(
+                new InetSocketAddress("localhost", 0),
+                new InetSocketAddress("localhost", 80),
+                metrics,
+                Timeout.ofSeconds(30));
+        context.setEndpointDetails(reused);
+
+        final ClassicHttpRequest request = new BasicClassicHttpRequest("POST", "/");
+        request.setEntity(new StringEntity("data", StandardCharsets.US_ASCII));
+
+        new RequestExpectContinue().process(request, request.getEntity(), context);
+
+        final Header header = request.getFirstHeader(HttpHeaders.EXPECT);
+        Assertions.assertNotNull(header);
+        Assertions.assertEquals(HeaderElements.CONTINUE, header.getValue());
+    }
+
+    @Test
+    void testNoExpectContinueFreshConnectionWithIfReused() throws Exception {
+        final HttpClientContext context = HttpClientContext.create();
+        final RequestConfig cfg = RequestConfig.custom()
+                .setExpectContinueEnabled(true)
+                .setExpectContinueTrigger(ExpectContinueTrigger.IF_REUSED)
+                .build();
+        context.setRequestConfig(cfg);
+
+        // fresh endpoint: requestCount == 0
+        context.setEndpointDetails(new BasicEndpointDetails(
+                new InetSocketAddress("localhost", 0),
+                new InetSocketAddress("localhost", 80),
+                null,
+                Timeout.ofSeconds(30)));
+
+        final ClassicHttpRequest req = new BasicClassicHttpRequest("POST", "/");
+        req.setEntity(new StringEntity("data", StandardCharsets.US_ASCII));
+
+        new RequestExpectContinue().process(req, req.getEntity(), context);
+
+        Assertions.assertNull(req.getFirstHeader(HttpHeaders.EXPECT));
+    }
+
+    @Test
+    void testHeaderAlreadyPresentIsNotDuplicated() throws Exception {
+        final HttpClientContext context = HttpClientContext.create();
+        final RequestConfig cfg = RequestConfig.custom()
+                .setExpectContinueEnabled(true)
+                .build();                      // default trigger = ALWAYS
+        context.setRequestConfig(cfg);
+
+        final ClassicHttpRequest req = new BasicClassicHttpRequest("POST", "/");
+        req.setEntity(new StringEntity("data", StandardCharsets.US_ASCII));
+        req.addHeader(HttpHeaders.EXPECT, HeaderElements.CONTINUE); // pre-existing
+
+        new RequestExpectContinue().process(req, req.getEntity(), context);
+
+        final Header[] headers = req.getHeaders(HttpHeaders.EXPECT);
+        Assertions.assertEquals(1, headers.length);                 // no duplicates
+    }
 }