HTTPCLIENT-2013: revised handling of connect exceptions; improved consistency in behavior of the classic and async clients; ConnectTimeoutException now extends SocketTimeoutException

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

@@ -0,0 +1,87 @@
+/*
+ * ====================================================================
+ * 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 java.io.IOException;
+import java.net.ConnectException;
+import java.net.InetAddress;
+import java.net.SocketTimeoutException;
+import java.util.Arrays;
+
+import org.apache.hc.core5.annotation.Internal;
+import org.apache.hc.core5.net.NamedEndpoint;
+
+/**
+ * Connect exception support methods.
+ *
+ * @since 5.0
+ */
+@Internal
+public final class ConnectExceptionSupport {
+
+    public static ConnectTimeoutException createConnectTimeoutException(
+            final IOException cause, final NamedEndpoint namedEndpoint, final InetAddress... remoteAddresses) {
+        final String message = "Connect to " +
+                (namedEndpoint != null ? namedEndpoint : "remote endpoint") +
+                (remoteAddresses != null && remoteAddresses.length > 0 ? " " + Arrays.asList(remoteAddresses) : "") +
+                ((cause != null && cause.getMessage() != null) ? " failed: " + cause.getMessage() : " timed out");
+        return new ConnectTimeoutException(message, namedEndpoint);
+    }
+
+    public static HttpHostConnectException createHttpHostConnectException(
+            final IOException cause,
+            final NamedEndpoint namedEndpoint,
+            final InetAddress... remoteAddresses) {
+        final String message = "Connect to " +
+                (namedEndpoint != null ? namedEndpoint : "remote endpoint") +
+                (remoteAddresses != null && remoteAddresses.length > 0 ? " " + Arrays.asList(remoteAddresses) : "") +
+                ((cause != null && cause.getMessage() != null) ? " failed: " + cause.getMessage() : " refused");
+        return new HttpHostConnectException(message, namedEndpoint);
+    }
+
+    public static IOException enhance(
+            final IOException cause, final NamedEndpoint namedEndpoint, final InetAddress... remoteAddresses) {
+        if (cause instanceof SocketTimeoutException) {
+            final IOException ex = createConnectTimeoutException(cause, namedEndpoint, remoteAddresses);
+            ex.setStackTrace(cause.getStackTrace());
+            return ex;
+        } else if (cause instanceof ConnectException) {
+            if ("Connection timed out".equals(cause.getMessage())) {
+                final IOException ex = createConnectTimeoutException(cause, namedEndpoint, remoteAddresses);
+                ex.initCause(cause);
+                return ex;
+            } else {
+                final IOException ex = createHttpHostConnectException(cause, namedEndpoint, remoteAddresses);
+                ex.setStackTrace(cause.getStackTrace());
+                return ex;
+            }
+        } else {
+            return cause;
+        }
+    }
+
+}

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

@@ -27,33 +27,21 @@
 
 package org.apache.hc.client5.http;
 
-import java.io.IOException;
+import java.net.SocketTimeoutException;
-import java.io.InterruptedIOException;
-import java.net.InetAddress;
-import java.util.Arrays;
 
 import org.apache.hc.core5.net.NamedEndpoint;
 
 /**
- * A timeout while connecting to an HTTP server or waiting for an
+ * A timeout while connecting to an HTTP server or waiting for an available connection from a connection manager.
- * available connection from an HttpConnectionManager.
  *
  * @since 4.0
  */
-public class ConnectTimeoutException extends InterruptedIOException {
+public class ConnectTimeoutException extends SocketTimeoutException {
 
     private static final long serialVersionUID = -4816682903149535989L;
 
     private final NamedEndpoint namedEndpoint;
 
-    /**
-     * Creates a ConnectTimeoutException with a {@code null} detail message.
-     */
-    public ConnectTimeoutException() {
-        super();
-        this.namedEndpoint = null;
-    }
-
     /**
      * Creates a ConnectTimeoutException with the specified detail message.
      */
@@ -62,23 +50,9 @@ public class ConnectTimeoutException extends InterruptedIOException {
         this.namedEndpoint = null;
     }
 
-    /**
+    public ConnectTimeoutException(final String message, final NamedEndpoint namedEndpoint) {
-     * Creates a ConnectTimeoutException based on original {@link IOException}.
+        super(message);
-     *
-     * @since 4.3
-     */
-    public ConnectTimeoutException(
-            final IOException cause,
-            final NamedEndpoint namedEndpoint,
-            final InetAddress... remoteAddresses) {
-        super("Connect to " +
-                (namedEndpoint != null ? namedEndpoint : "remote endpoint") +
-                (remoteAddresses != null && remoteAddresses.length > 0 ?
-                        " " + Arrays.asList(remoteAddresses) : "") +
-                ((cause != null && cause.getMessage() != null) ?
-                        " failed: " + cause.getMessage() : " timed out"));
         this.namedEndpoint = namedEndpoint;
-        initCause(cause);
     }
 
     /**

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

@@ -26,16 +26,12 @@
  */
 package org.apache.hc.client5.http;
 
-import java.io.IOException;
 import java.net.ConnectException;
-import java.net.InetAddress;
-import java.util.Arrays;
 
 import org.apache.hc.core5.net.NamedEndpoint;
 
 /**
- * A {@link ConnectException} that specifies the {@link NamedEndpoint} that was
+ * A {@link ConnectException} that specifies the {@link NamedEndpoint} that was being connected to.
- * being connected to.
  *
  * @since 4.0
  */
@@ -46,22 +42,16 @@ public class HttpHostConnectException extends ConnectException {
     private final NamedEndpoint namedEndpoint;
 
     /**
-     * Creates a HttpHostConnectException based on original {@link java.io.IOException}.
+     * Creates a HttpHostConnectException with the specified detail message.
-     *
-     * @since 5.0
      */
-    public HttpHostConnectException(
+    public HttpHostConnectException(final String message) {
-            final IOException cause,
+        super(message);
-            final NamedEndpoint namedEndpoint,
+        this.namedEndpoint = null;
-            final InetAddress... remoteAddresses) {
+    }
-        super("Connect to " +
+
-                (namedEndpoint != null ? namedEndpoint : "remote endpoint") +
+    public HttpHostConnectException(final String message, final NamedEndpoint namedEndpoint) {
-                (remoteAddresses != null && remoteAddresses .length > 0 ?
+        super(message);
-                        " " + Arrays.asList(remoteAddresses) : "") +
-                ((cause != null && cause.getMessage() != null) ?
-                        " failed: " + cause.getMessage() : " refused"));
         this.namedEndpoint = namedEndpoint;
-        initCause(cause);
     }
 
     /**

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

@@ -27,16 +27,12 @@
 package org.apache.hc.client5.http.impl.io;
 
 import java.io.IOException;
-import java.net.ConnectException;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
-import java.net.NoRouteToHostException;
 import java.net.Socket;
-import java.net.SocketTimeoutException;
 
-import org.apache.hc.client5.http.ConnectTimeoutException;
+import org.apache.hc.client5.http.ConnectExceptionSupport;
 import org.apache.hc.client5.http.DnsResolver;
-import org.apache.hc.client5.http.HttpHostConnectException;
 import org.apache.hc.client5.http.SchemePortResolver;
 import org.apache.hc.client5.http.SystemDefaultDnsResolver;
 import org.apache.hc.client5.http.UnsupportedSchemeException;
@@ -155,21 +151,9 @@ public class DefaultHttpClientConnectionOperator implements HttpClientConnection
                     this.log.debug(ConnPoolSupport.getId(conn) + ": connection established " + conn);
                 }
                 return;
-            } catch (final SocketTimeoutException ex) {
+            } catch (final IOException ex) {
                 if (last) {
-                    throw new ConnectTimeoutException(ex, host, addresses);
+                    throw ConnectExceptionSupport.enhance(ex, host, addresses);
-                }
-            } catch (final ConnectException ex) {
-                if (last) {
-                    final String msg = ex.getMessage();
-                    if ("Connection timed out".equals(msg)) {
-                        throw new ConnectTimeoutException(ex, host, addresses);
-                    }
-                    throw new HttpHostConnectException(ex, host, addresses);
-                }
-            } catch (final NoRouteToHostException ex) {
-                if (last) {
-                    throw ex;
                 }
             }
             if (this.log.isDebugEnabled()) {

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

@@ -36,8 +36,8 @@ import java.util.Arrays;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import org.apache.hc.client5.http.ConnectExceptionSupport;
 import org.apache.hc.client5.http.DnsResolver;
-import org.apache.hc.client5.http.HttpHostConnectException;
 import org.apache.hc.client5.http.SystemDefaultDnsResolver;
 import org.apache.hc.core5.concurrent.ComplexFuture;
 import org.apache.hc.core5.concurrent.FutureCallback;
@@ -129,7 +129,7 @@ final class MultihomeIOSessionRequester {
                                                 "(" + cause.getClass() + "); terminating operation");
                                     }
                                     if (cause instanceof IOException) {
-                                        future.failed(new HttpHostConnectException((IOException) cause, remoteEndpoint, remoteAddresses));
+                                        future.failed(ConnectExceptionSupport.enhance((IOException) cause, remoteEndpoint, remoteAddresses));
                                     } else {
                                         future.failed(cause);
                                     }

httpclient5/src/test/java/org/apache/hc/client5/http/ConnectExceptionSupportTest.java

@@ -38,30 +38,18 @@ import org.junit.Test;
  * Unit tests for exceptions.
  * Trivial, but it looks better in the Clover reports.
  */
-public class TestExceptions {
+public class ConnectExceptionSupportTest {
 
     @Test
-    public void testConnectTimeoutExceptionNullMessage() {
+    public void testConnectTimeoutExceptionFromNullMessageAndHost() {
-        final ConnectTimeoutException ctx = new ConnectTimeoutException();
+        final ConnectTimeoutException ctx = ConnectExceptionSupport.createConnectTimeoutException(null, null);
-        Assert.assertNull(ctx.getMessage());
-    }
-
-    @Test
-    public void testConnectTimeoutExceptionSimpleMessage() {
-        final ConnectTimeoutException ctx = new ConnectTimeoutException("sample exception message");
-        Assert.assertEquals("sample exception message", ctx.getMessage());
-    }
-
-    @Test
-    public void testConnectTimeoutExceptionFromNullCause() {
-        final ConnectTimeoutException ctx = new ConnectTimeoutException(null, null);
         Assert.assertEquals("Connect to remote endpoint timed out", ctx.getMessage());
     }
 
     @Test
     public void testConnectTimeoutExceptionFromCause() {
         final IOException cause = new IOException("something awful");
-        final ConnectTimeoutException ctx = new ConnectTimeoutException(cause, null);
+        final ConnectTimeoutException ctx = ConnectExceptionSupport.createConnectTimeoutException(cause, null);
         Assert.assertEquals("Connect to remote endpoint failed: something awful", ctx.getMessage());
     }
 
@@ -69,7 +57,7 @@ public class TestExceptions {
     public void testConnectTimeoutExceptionFromCauseAndHost() {
         final HttpHost target = new HttpHost("localhost");
         final IOException cause = new IOException();
-        final ConnectTimeoutException ctx = new ConnectTimeoutException(cause, target);
+        final ConnectTimeoutException ctx = ConnectExceptionSupport.createConnectTimeoutException(cause, target);
         Assert.assertEquals("Connect to http://localhost timed out", ctx.getMessage());
     }
 
@@ -78,13 +66,13 @@ public class TestExceptions {
         final HttpHost target = new HttpHost("localhost");
         final InetAddress remoteAddress = InetAddress.getByAddress(new byte[] {1,2,3,4});
         final IOException cause = new IOException();
-        final ConnectTimeoutException ctx = new ConnectTimeoutException(cause, target, remoteAddress);
+        final ConnectTimeoutException ctx = ConnectExceptionSupport.createConnectTimeoutException(cause, target, remoteAddress);
         Assert.assertEquals("Connect to http://localhost [/1.2.3.4] timed out", ctx.getMessage());
     }
 
     @Test
     public void testHttpHostConnectExceptionFromNullCause() {
-        final HttpHostConnectException ctx = new HttpHostConnectException(null, null,
+        final HttpHostConnectException ctx = ConnectExceptionSupport.createHttpHostConnectException(null, null,
                 (InetAddress [])null);
         Assert.assertEquals("Connect to remote endpoint refused", ctx.getMessage());
     }
@@ -92,7 +80,7 @@ public class TestExceptions {
     @Test
     public void testHttpHostConnectExceptionFromCause() {
         final IOException cause = new IOException("something awful");
-        final HttpHostConnectException ctx = new HttpHostConnectException(cause, null);
+        final HttpHostConnectException ctx = ConnectExceptionSupport.createHttpHostConnectException(cause, null);
         Assert.assertEquals("Connect to remote endpoint failed: something awful", ctx.getMessage());
     }
 
@@ -100,7 +88,7 @@ public class TestExceptions {
     public void testHttpHostConnectExceptionFromCauseAndHost() {
         final HttpHost target = new HttpHost("localhost");
         final IOException cause = new IOException();
-        final HttpHostConnectException ctx = new HttpHostConnectException(cause, target);
+        final HttpHostConnectException ctx = ConnectExceptionSupport.createHttpHostConnectException(cause, target);
         Assert.assertEquals("Connect to http://localhost refused", ctx.getMessage());
     }
 
@@ -110,7 +98,7 @@ public class TestExceptions {
         final InetAddress remoteAddress1 = InetAddress.getByAddress(new byte[] {1,2,3,4});
         final InetAddress remoteAddress2 = InetAddress.getByAddress(new byte[] {5,6,7,8});
         final IOException cause = new IOException();
-        final HttpHostConnectException ctx = new HttpHostConnectException(cause, target,
+        final HttpHostConnectException ctx = ConnectExceptionSupport.createHttpHostConnectException(cause, target,
                 remoteAddress1, remoteAddress2);
         Assert.assertEquals("Connect to http://localhost [/1.2.3.4, /5.6.7.8] refused", ctx.getMessage());
     }

httpclient5/src/test/java/org/apache/hc/client5/http/examples/ClientExecuteSOCKS.java

@@ -31,9 +31,7 @@ import java.io.IOException;
 import java.net.InetSocketAddress;
 import java.net.Proxy;
 import java.net.Socket;
-import java.net.SocketTimeoutException;
 
-import org.apache.hc.client5.http.ConnectTimeoutException;
 import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
 import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
@@ -106,11 +104,7 @@ public class ClientExecuteSOCKS {
             if (localAddress != null) {
                 sock.bind(localAddress);
             }
-            try {
+            sock.connect(remoteAddress, connectTimeout != null ? connectTimeout.toMillisIntBound() : 0);
-                sock.connect(remoteAddress, connectTimeout != null ? connectTimeout.toMillisIntBound() : 0);
-            } catch (final SocketTimeoutException ex) {
-                throw new ConnectTimeoutException(ex, host, remoteAddress.getAddress());
-            }
             return sock;
         }
 

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

@@ -27,9 +27,9 @@
 package org.apache.hc.client5.http.impl;
 
 import java.io.IOException;
+import java.net.SocketTimeoutException;
 import java.net.UnknownHostException;
 
-import org.apache.hc.client5.http.ConnectTimeoutException;
 import org.apache.hc.client5.http.classic.methods.HttpGet;
 import org.junit.Assert;
 import org.junit.Test;
@@ -45,7 +45,7 @@ public class TestDefaultHttpRequestRetryHandler {
         final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler();
         Assert.assertEquals(3, retryHandler.getRetryCount());
 
-        Assert.assertFalse(retryHandler.retryRequest(request, new ConnectTimeoutException(), 1, null));
+        Assert.assertFalse(retryHandler.retryRequest(request, new SocketTimeoutException(), 1, null));
     }
 
     @Test
@@ -82,7 +82,7 @@ public class TestDefaultHttpRequestRetryHandler {
 
         final DefaultHttpRequestRetryHandler retryHandler = new DefaultHttpRequestRetryHandler();
 
-        Assert.assertFalse(retryHandler.retryRequest(request, new ConnectTimeoutException(), 3, null));
+        Assert.assertFalse(retryHandler.retryRequest(request, new SocketTimeoutException(), 3, null));
     }
 
 }