Revised handling of non-repeatable requests

httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java

@@ -35,8 +35,6 @@
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicLong;
 
-import org.apache.hc.client5.http.ClientProtocolException;
-import org.apache.hc.client5.http.NonRepeatableRequestException;
 import org.apache.hc.client5.http.auth.AuthCache;
 import org.apache.hc.client5.http.auth.AuthChallenge;
 import org.apache.hc.client5.http.auth.AuthScheme;
@@ -215,7 +213,7 @@ public void testBasicAuthenticationSuccessOnNonRepeatablePutExpectContinue() thr
         Assert.assertNotNull(entity);
     }
 
-    @Test(expected=ClientProtocolException.class)
+    @Test
     public void testBasicAuthenticationFailureOnNonRepeatablePutDontExpectContinue() throws Exception {
         this.server.registerHandler("*", new EchoHandler());
         final HttpHost target = start();
@@ -233,15 +231,11 @@ public void testBasicAuthenticationFailureOnNonRepeatablePutDontExpectContinue()
                 new UsernamePasswordCredentials("test", "boom".toCharArray()));
         context.setCredentialsProvider(credsProvider);
 
-        try {
+        final CloseableHttpResponse response = this.httpclient.execute(target, httpput, context);
-            this.httpclient.execute(target, httpput, context);
+        final HttpEntity entity = response.getEntity();
-            Assert.fail("ClientProtocolException should have been thrown");
+        Assert.assertEquals(401, response.getCode());
-        } catch (final ClientProtocolException ex) {
+        Assert.assertNotNull(entity);
-            final Throwable cause = ex.getCause();
+        EntityUtils.consume(entity);
-            Assert.assertNotNull(cause);
-            Assert.assertTrue(cause instanceof NonRepeatableRequestException);
-            throw ex;
-        }
     }
 
     @Test
@@ -267,7 +261,7 @@ public void testBasicAuthenticationSuccessOnRepeatablePost() throws Exception {
         Assert.assertEquals("test realm", authscope.getRealm());
     }
 
-    @Test(expected=ClientProtocolException.class)
+    @Test
     public void testBasicAuthenticationFailureOnNonRepeatablePost() throws Exception {
         this.server.registerHandler("*", new EchoHandler());
         final HttpHost target = start();
@@ -278,19 +272,18 @@ public void testBasicAuthenticationFailureOnNonRepeatablePost() throws Exception
                         new byte[] { 0,1,2,3,4,5,6,7,8,9 }), -1));
 
         final HttpClientContext context = HttpClientContext.create();
+        context.setRequestConfig(RequestConfig.custom()
+                .setExpectContinueEnabled(false)
+                .build());
         final TestCredentialsProvider credsProvider = new TestCredentialsProvider(
                 new UsernamePasswordCredentials("test", "test".toCharArray()));
         context.setCredentialsProvider(credsProvider);
 
-        try {
+        final CloseableHttpResponse response = this.httpclient.execute(target, httppost, context);
-            this.httpclient.execute(target, httppost, context);
+        final HttpEntity entity = response.getEntity();
-            Assert.fail("ClientProtocolException should have been thrown");
+        Assert.assertEquals(401, response.getCode());
-        } catch (final ClientProtocolException ex) {
+        Assert.assertNotNull(entity);
-            final Throwable cause = ex.getCause();
+        EntityUtils.consume(entity);
-            Assert.assertNotNull(cause);
-            Assert.assertTrue(cause instanceof NonRepeatableRequestException);
-            throw ex;
-        }
     }
 
     static class TestTargetAuthenticationStrategy extends DefaultAuthenticationStrategy {

httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java

@@ -31,9 +31,7 @@
 import java.net.URI;
 import java.util.List;
 
-import org.apache.hc.client5.http.ClientProtocolException;
 import org.apache.hc.client5.http.HttpRequestRetryHandler;
-import org.apache.hc.client5.http.NonRepeatableRequestException;
 import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.apache.hc.client5.http.classic.methods.HttpPost;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
@@ -161,7 +159,7 @@ public boolean retryRequest(
         Assert.assertEquals(1, myheaders.length);
     }
 
-    @Test(expected=ClientProtocolException.class)
+    @Test(expected=IOException.class)
     public void testNonRepeatableEntity() throws Exception {
         this.server.registerHandler("*", new SimpleService());
 
@@ -192,16 +190,7 @@ public boolean retryRequest(
                 new ByteArrayInputStream(
                         new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9 } ),
                         -1));
-
+        this.httpclient.execute(target, httppost, context);
-        try {
-            this.httpclient.execute(target, httppost, context);
-        } catch (final ClientProtocolException ex) {
-            Assert.assertTrue(ex.getCause() instanceof NonRepeatableRequestException);
-            final NonRepeatableRequestException nonRepeat = (NonRepeatableRequestException)ex.getCause();
-            Assert.assertTrue(nonRepeat.getCause() instanceof IOException);
-            Assert.assertEquals("a message showing that this failed", nonRepeat.getCause().getMessage());
-            throw ex;
-        }
     }
 
     @Test

httpclient5/src/main/java/org/apache/hc/client5/http/NonRepeatableRequestException.java

@@ -1,70 +0,0 @@
-/*
- * ====================================================================
- * 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;
-
-import org.apache.hc.core5.http.ProtocolException;
-
-/**
- * Signals failure to retry the request due to non-repeatable request
- * entity.
- *
- *
- * @since 4.0
- */
-public class NonRepeatableRequestException extends ProtocolException {
-
-    private static final long serialVersionUID = 82685265288806048L;
-
-    /**
-     * Creates a new NonRepeatableEntityException with a {@code null} detail message.
-     */
-    public NonRepeatableRequestException() {
-        super();
-    }
-
-    /**
-     * Creates a new NonRepeatableEntityException with the specified detail message.
-     *
-     * @param message The exception detail message
-     */
-    public NonRepeatableRequestException(final String message) {
-        super(message);
-    }
-
-    /**
-     * Creates a new NonRepeatableEntityException with the specified detail message.
-     *
-     * @param message The exception detail message
-     * @param cause the cause
-     */
-    public NonRepeatableRequestException(final String message, final Throwable cause) {
-        super(message, cause);
-    }
-
-
-
-}

httpclient5/src/main/java/org/apache/hc/client5/http/async/methods/SimpleRequestProducer.java

@@ -47,16 +47,7 @@ public static SimpleRequestProducer create(final SimpleHttpRequest request, fina
             if (body.isText()) {
                 entityProducer = new StringAsyncEntityProducer(body.getBodyText(), body.getContentType());
             } else {
-                entityProducer = new BasicAsyncEntityProducer(body.getBodyBytes(), body.getContentType()) {
+                entityProducer = new BasicAsyncEntityProducer(body.getBodyBytes(), body.getContentType());
-
-                    //TODO: return the actual content length once
-                    // the entity producers made repeatable in HttpCore
-                    @Override
-                    public long getContentLength() {
-                        return -1;
-                    }
-
-                };
             }
         } else {
             entityProducer = null;

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

@@ -207,16 +207,24 @@ public void completed() {
                 }
 
                 if (challenged.get()) {
-                    // Reset request headers
+                    if (entityProducer != null && !entityProducer.isRepeatable()) {
-                    final HttpRequest original = scope.originalRequest;
+                        log.debug("Cannot retry non-repeatable request");
-                    request.setHeaders();
+                        asyncExecCallback.completed();
-                    for (final Iterator<Header> it = original.headerIterator(); it.hasNext(); ) {
+                    } else {
-                        request.addHeader(it.next());
+                        // Reset request headers
-                    }
+                        final HttpRequest original = scope.originalRequest;
-                    try {
+                        request.setHeaders();
-                        internalExecute(challenged, request, entityProducer, scope, chain, asyncExecCallback);
+                        for (final Iterator<Header> it = original.headerIterator(); it.hasNext(); ) {
-                    } catch (final HttpException | IOException ex) {
+                            request.addHeader(it.next());
-                        asyncExecCallback.failed(ex);
+                        }
+                        try {
+                            if (entityProducer != null) {
+                                entityProducer.releaseResources();
+                            }
+                            internalExecute(challenged, request, entityProducer, scope, chain, asyncExecCallback);
+                        } catch (final HttpException | IOException ex) {
+                            asyncExecCallback.failed(ex);
+                        }
                     }
                 } else {
                     asyncExecCallback.completed();

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

@@ -166,13 +166,19 @@ public void completed() {
                 if (state.redirectURI == null) {
                     asyncExecCallback.completed();
                 } else {
-                    try {
+                    final AsyncEntityProducer entityProducer = state.currentEntityProducer;
-                        if (state.reroute) {
+                    if (entityProducer != null && !entityProducer.isRepeatable()) {
-                            scope.execRuntime.releaseConnection();
+                        log.debug("Cannot redirect non-repeatable request");
+                        asyncExecCallback.completed();
+                    } else {
+                        try {
+                            if (state.reroute) {
+                                scope.execRuntime.releaseConnection();
+                            }
+                            internalExecute(state, chain, asyncExecCallback);
+                        } catch (final IOException | HttpException ex) {
+                            asyncExecCallback.failed(ex);
                         }
-                        internalExecute(state, chain, asyncExecCallback);
-                    } catch (final IOException | HttpException ex) {
-                        asyncExecCallback.failed(ex);
                     }
                 }
             }

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

@@ -83,7 +83,9 @@ public void failed(final Exception cause) {
                 if (cause instanceof IOException) {
                     final HttpRoute route = scope.route;
                     final HttpClientContext clientContext = scope.clientContext;
-                    if (retryHandler.retryRequest(request, (IOException) cause, execCount, clientContext)) {
+                    if (entityProducer != null && !entityProducer.isRepeatable()) {
+                        log.debug("Cannot retry non-repeatable request");
+                    } else if (retryHandler.retryRequest(request, (IOException) cause, execCount, clientContext)) {
                         if (log.isInfoEnabled()) {
                             log.info("I/O exception ("+ cause.getClass().getName() +
                                     ") caught when processing request to "
@@ -99,6 +101,9 @@ public void failed(final Exception cause) {
                         }
                         try {
                             scope.execRuntime.discardConnection();
+                            if (entityProducer != null) {
+                                entityProducer.releaseResources();
+                            }
                             internalExecute(execCount + 1, request, entityProducer, scope, chain, asyncExecCallback);
                             return;
                         } catch (final IOException | HttpException ex) {

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

@@ -38,6 +38,7 @@
 import org.apache.hc.client5.http.async.AsyncExecCallback;
 import org.apache.hc.client5.http.async.AsyncExecChain;
 import org.apache.hc.client5.http.async.AsyncExecRuntime;
+import org.apache.hc.client5.http.async.methods.SimpleRequestProducer;
 import org.apache.hc.client5.http.auth.AuthSchemeProvider;
 import org.apache.hc.client5.http.auth.CredentialsProvider;
 import org.apache.hc.client5.http.config.Configurable;
@@ -201,7 +202,8 @@ public void failed(final Exception cause) {
 
                                 @Override
                                 public boolean isRepeatable() {
-                                    return false;
+                                    //TODO: use AsyncRequestProducer#isRepeatable once available
+                                    return requestProducer instanceof SimpleRequestProducer;
                                 }
 
                                 @Override

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

@@ -34,7 +34,6 @@
 
 import org.apache.hc.client5.http.AuthenticationStrategy;
 import org.apache.hc.client5.http.HttpRoute;
-import org.apache.hc.client5.http.NonRepeatableRequestException;
 import org.apache.hc.client5.http.StandardMethods;
 import org.apache.hc.client5.http.auth.AuthExchange;
 import org.apache.hc.client5.http.auth.ChallengeType;
@@ -138,15 +137,7 @@ public ClassicHttpResponse execute(
             final AuthExchange targetAuthExchange = context.getAuthExchange(target);
             final AuthExchange proxyAuthExchange = proxy != null ? context.getAuthExchange(proxy) : new AuthExchange();
 
-            for (int execCount = 1;; execCount++) {
+            for (;;) {
-
-                if (execCount > 1) {
-                    final HttpEntity entity = request.getEntity();
-                    if (entity != null && !entity.isRepeatable()) {
-                        throw new NonRepeatableRequestException("Cannot retry request " +
-                                "with a non-repeatable request entity.");
-                    }
-                }
 
                 // Run request protocol interceptors
                 context.setAttribute(HttpClientContext.HTTP_ROUTE, route);
@@ -176,12 +167,16 @@ public ClassicHttpResponse execute(
                     // Do not perform authentication for TRACE request
                     return response;
                 }
-
+                final HttpEntity requestEntity = request.getEntity();
+                if (requestEntity != null && !requestEntity.isRepeatable()) {
+                    log.debug("Cannot retry non-repeatable request");
+                    return response;
+                }
                 if (needAuthentication(targetAuthExchange, proxyAuthExchange, route, request, response, context)) {
                     // Make sure the response body is fully consumed, if present
-                    final HttpEntity entity = response.getEntity();
+                    final HttpEntity responseEntity = response.getEntity();
                     if (execRuntime.isConnectionReusable()) {
-                        EntityUtils.consume(entity);
+                        EntityUtils.consume(responseEntity);
                     } else {
                         execRuntime.disconnect();
                         if (proxyAuthExchange.getState() == AuthExchange.State.SUCCESS

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

@@ -49,6 +49,7 @@
 import org.apache.hc.core5.annotation.ThreadingBehavior;
 import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.ClassicHttpResponse;
+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.HttpStatus;
@@ -110,7 +111,11 @@ public ClassicHttpResponse execute(
             final ClassicHttpResponse response = chain.proceed(currentRequest, currentScope);
             try {
                 if (config.isRedirectsEnabled() && this.redirectStrategy.isRedirected(request, response, context)) {
-
+                    final HttpEntity requestEntity = request.getEntity();
+                    if (requestEntity != null && !requestEntity.isRepeatable()) {
+                        this.log.debug("Cannot redirect non-repeatable request");
+                        return response;
+                    }
                     if (redirectCount >= maxRedirects) {
                         throw new RedirectException("Maximum redirects ("+ maxRedirects + ") exceeded");
                     }

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

@@ -31,7 +31,6 @@
 
 import org.apache.hc.client5.http.HttpRequestRetryHandler;
 import org.apache.hc.client5.http.HttpRoute;
-import org.apache.hc.client5.http.NonRepeatableRequestException;
 import org.apache.hc.client5.http.classic.ExecChain;
 import org.apache.hc.client5.http.classic.ExecChainHandler;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
@@ -88,6 +87,11 @@ public ClassicHttpResponse execute(
                 if (scope.execRuntime.isExecutionAborted()) {
                     throw new RequestFailedException("Request aborted");
                 }
+                final HttpEntity requestEntity = request.getEntity();
+                if (requestEntity != null && !requestEntity.isRepeatable()) {
+                    this.log.debug("Cannot retry non-repeatable request");
+                    throw ex;
+                }
                 if (retryHandler.retryRequest(request, ex, execCount, context)) {
                     if (this.log.isInfoEnabled()) {
                         this.log.info("I/O exception ("+ ex.getClass().getName() +
@@ -99,12 +103,6 @@ public ClassicHttpResponse execute(
                     if (this.log.isDebugEnabled()) {
                         this.log.debug(ex.getMessage(), ex);
                     }
-                    final HttpEntity entity = request.getEntity();
-                    if (entity != null && !entity.isRepeatable()) {
-                        this.log.debug("Cannot retry non-repeatable request");
-                        throw new NonRepeatableRequestException("Cannot retry request " +
-                                "with a non-repeatable request entity", ex);
-                    }
                     currentRequest = ClassicRequestCopier.INSTANCE.copy(scope.originalRequest);
                     if (this.log.isInfoEnabled()) {
                         this.log.info("Retrying request to " + route);

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

@@ -36,7 +36,6 @@
 
 import org.apache.hc.client5.http.AuthenticationStrategy;
 import org.apache.hc.client5.http.HttpRoute;
-import org.apache.hc.client5.http.NonRepeatableRequestException;
 import org.apache.hc.client5.http.auth.AuthChallenge;
 import org.apache.hc.client5.http.auth.AuthExchange;
 import org.apache.hc.client5.http.auth.AuthScheme;
@@ -295,7 +294,7 @@ public void testExecEntityEnclosingRequestRetryOnAuthChallenge() throws Exceptio
         Assert.assertNull(proxyAuthExchange.getAuthScheme());
     }
 
-    @Test(expected = NonRepeatableRequestException.class)
+    @Test
     public void testExecEntityEnclosingRequest() throws Exception {
         final HttpRoute route = new HttpRoute(target);
         final HttpClientContext context = new HttpClientContext();
@@ -335,7 +334,8 @@ public HttpResponse answer(final InvocationOnMock invocationOnMock) throws Throw
                 Mockito.<HttpClientContext>any())).thenReturn(Collections.<AuthScheme>singletonList(new BasicScheme()));
 
         final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context);
-        protocolExec.execute(request, scope, chain);
+        final ClassicHttpResponse response = protocolExec.execute(request, scope, chain);
+        Assert.assertEquals(401, response.getCode());
     }
 
 }

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

@@ -32,7 +32,6 @@
 
 import org.apache.hc.client5.http.HttpRequestRetryHandler;
 import org.apache.hc.client5.http.HttpRoute;
-import org.apache.hc.client5.http.NonRepeatableRequestException;
 import org.apache.hc.client5.http.classic.ExecChain;
 import org.apache.hc.client5.http.classic.ExecRuntime;
 import org.apache.hc.client5.http.classic.methods.HttpGet;
@@ -144,7 +143,7 @@ public void testAbortedRequest() throws Exception {
         }
     }
 
-    @Test(expected = NonRepeatableRequestException.class)
+    @Test(expected = IOException.class)
     public void testNonRepeatableRequest() throws Exception {
         final HttpRoute route = new HttpRoute(target);
         final HttpPost originalRequest = new HttpPost("/test");