HTTPCLIENT-1362: better error messages for connect timed out and connection refused exceptions (second cut)

git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1489438 13f79535-47bb-0310-9956-ffa450edef68

httpclient/src/main/java/org/apache/http/conn/ConnectTimeoutException.java

@@ -29,7 +29,9 @@ package org.apache.http.conn;
 
 import java.io.IOException;
 import java.io.InterruptedIOException;
+import java.net.InetAddress;
 import java.net.InetSocketAddress;
+import java.util.Arrays;
 
 import org.apache.http.HttpHost;
 import org.apache.http.annotation.Immutable;
@@ -47,7 +49,6 @@ public class ConnectTimeoutException extends InterruptedIOException {
     private static final long serialVersionUID = -4816682903149535989L;
 
     private final HttpHost host;
-    private final InetSocketAddress remoteAddress;
 
     /**
      * Creates a ConnectTimeoutException with a <tt>null</tt> detail message.
@@ -55,7 +56,6 @@ public class ConnectTimeoutException extends InterruptedIOException {
     public ConnectTimeoutException() {
         super();
         this.host = null;
-        this.remoteAddress = null;
     }
 
     /**
@@ -64,7 +64,6 @@ public class ConnectTimeoutException extends InterruptedIOException {
     public ConnectTimeoutException(final String message) {
         super(message);
         this.host = null;
-        this.remoteAddress = null;
     }
 
     /**
@@ -73,15 +72,14 @@ public class ConnectTimeoutException extends InterruptedIOException {
      * @since 4.3
      */
     public ConnectTimeoutException(
+            final IOException cause,
             final HttpHost host,
-            final InetSocketAddress remoteAddress,
-            final IOException cause) {
+            final InetAddress... remoteAddresses) {
         super("Connect to " +
                 (host != null ? host.toHostString() : "remote host") +
-                (remoteAddress != null ? " (" + remoteAddress.getAddress() + ")" : "")
+                (remoteAddresses != null ? " " + Arrays.asList(remoteAddresses) : "")
                 + ((cause != null && cause.getMessage() != null) ? " failed: " + cause.getMessage() : " timed out"));
         this.host = host;
-        this.remoteAddress = remoteAddress;
         initCause(cause);
     }
 
@@ -92,11 +90,4 @@ public class ConnectTimeoutException extends InterruptedIOException {
         return host;
     }
 
-    /**
-     * @since 4.3
-     */
-    public InetSocketAddress getRemoteAddress() {
-        return remoteAddress;
-    }
-
 }

httpclient/src/main/java/org/apache/http/conn/HttpHostConnectException.java

@@ -28,7 +28,9 @@ package org.apache.http.conn;
 
 import java.io.IOException;
 import java.net.ConnectException;
+import java.net.InetAddress;
 import java.net.InetSocketAddress;
+import java.util.Arrays;
 
 import org.apache.http.HttpHost;
 import org.apache.http.annotation.Immutable;
@@ -45,10 +47,14 @@ public class HttpHostConnectException extends ConnectException {
     private static final long serialVersionUID = -3194482710275220224L;
 
     private final HttpHost host;
-    private final InetSocketAddress remoteAddress;
 
+    /**
+     * @deprecated (4.3) use {@link #HttpHostConnectException(java.io.IOException, org.apache.http.HttpHost,
+     *   java.net.InetAddress...)}
+     */
+    @Deprecated
     public HttpHostConnectException(final HttpHost host, final ConnectException cause) {
-        this(host, null, cause);
+        this(cause, host, null);
     }
 
     /**
@@ -57,15 +63,14 @@ public class HttpHostConnectException extends ConnectException {
      * @since 4.3
      */
     public HttpHostConnectException(
+            final IOException cause,
             final HttpHost host,
-            final InetSocketAddress remoteAddress,
-            final IOException cause) {
+            final InetAddress... remoteAddresses) {
         super("Connect to " +
                 (host != null ? host.toHostString() : "remote host") +
-                (remoteAddress != null ? " (" + remoteAddress.getAddress() + ")" : "")
+                (remoteAddresses != null ? " " + Arrays.asList(remoteAddresses) : "")
                 + ((cause != null && cause.getMessage() != null) ? " failed: " + cause.getMessage() : " refused"));
         this.host = host;
-        this.remoteAddress = remoteAddress;
         initCause(cause);
     }
 
@@ -73,11 +78,4 @@ public class HttpHostConnectException extends ConnectException {
         return this.host;
     }
 
-    /**
-     * @since 4.3
-     */
-    public InetSocketAddress getRemoteAddress() {
-        return remoteAddress;
-    }
-
 }

httpclient/src/main/java/org/apache/http/conn/socket/AbstractConnectionSocketFactory.java

@@ -1,104 +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.http.conn.socket;
-
-import org.apache.http.HttpHost;
-import org.apache.http.annotation.Immutable;
-import org.apache.http.conn.ConnectTimeoutException;
-import org.apache.http.conn.HttpHostConnectException;
-import org.apache.http.protocol.HttpContext;
-
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.InetSocketAddress;
-import java.net.Socket;
-import java.net.SocketTimeoutException;
-
-/**
- * This is a base class for {@link ConnectionSocketFactory} implementations.
- * This class provides a common exception handling logic for connect operations.
- *
- * @since 4.3
- */
-public abstract class AbstractConnectionSocketFactory implements ConnectionSocketFactory {
-
-     /**
-     * Connects a socket to the target host with the given resolved remote address.
-     *
-     * @param connectTimeout connect timeout.
-     * @param socket the socket to connect, as obtained from {@link #createSocket(HttpContext)}.
-     * <code>null</code> indicates that a new socket should be created and connected.
-     * @param host target host as specified by the caller (end user).
-     * @param remoteAddress the resolved remote address to connect to.
-     * @param localAddress the local address to bind the socket to, or <code>null</code> for any.
-     * @param context the actual HTTP context.
-     *
-      * @return  the connected socket. The returned object may be different
-      *          from the <code>sock</code> argument if this factory supports
-      *          a layered protocol.
-     * @throws org.apache.http.conn.ConnectTimeoutException if the socket cannot be connected
-     *          within the time limit defined by connectTimeout parameter.
-     * @throws org.apache.http.conn.HttpHostConnectException if the connection is refused
-     *          by the opposite endpoint.
-     */
-    public Socket connectSocket(
-            final int connectTimeout,
-            final Socket socket,
-            final HttpHost host,
-            final InetSocketAddress remoteAddress,
-            final InetSocketAddress localAddress,
-            final HttpContext context) throws IOException {
-        final Socket sock = socket != null ? socket : createSocket(context);
-        if (localAddress != null) {
-            sock.bind(localAddress);
-        }
-        try {
-            sock.connect(remoteAddress, connectTimeout);
-        } catch (final SocketTimeoutException ex) {
-            closeSocket(socket);
-            throw new ConnectTimeoutException(host, remoteAddress, ex);
-        } catch (final ConnectException ex) {
-            closeSocket(socket);
-            String msg = ex.getMessage();
-            if ("Connection timed out".equals(msg)) {
-                throw new ConnectTimeoutException(host, remoteAddress, ex);
-            } else {
-                throw new HttpHostConnectException(host, remoteAddress, ex);
-            }
-        }
-        return sock;
-    }
-
-    private void closeSocket(final Socket sock) {
-        try {
-            sock.close();
-        } catch (final IOException ignore) {
-        }
-    }
-
-}

httpclient/src/main/java/org/apache/http/conn/socket/PlainSocketFactory.java

@@ -28,8 +28,10 @@
 package org.apache.http.conn.socket;
 
 import java.io.IOException;
+import java.net.InetSocketAddress;
 import java.net.Socket;
 
+import org.apache.http.HttpHost;
 import org.apache.http.annotation.Immutable;
 import org.apache.http.protocol.HttpContext;
 
@@ -39,7 +41,7 @@ import org.apache.http.protocol.HttpContext;
  * @since 4.3
  */
 @Immutable
-public class PlainSocketFactory extends AbstractConnectionSocketFactory {
+public class PlainSocketFactory implements ConnectionSocketFactory {
 
     public static final PlainSocketFactory INSTANCE = new PlainSocketFactory();
 
@@ -55,4 +57,27 @@ public class PlainSocketFactory extends AbstractConnectionSocketFactory {
         return new Socket();
     }
 
+    public Socket connectSocket(
+            final int connectTimeout,
+            final Socket socket,
+            final HttpHost host,
+            final InetSocketAddress remoteAddress,
+            final InetSocketAddress localAddress,
+            final HttpContext context) throws IOException {
+        final Socket sock = socket != null ? socket : createSocket(context);
+        if (localAddress != null) {
+            sock.bind(localAddress);
+        }
+        try {
+            sock.connect(remoteAddress, connectTimeout);
+        } catch (final IOException ex) {
+            try {
+                sock.close();
+            } catch (final IOException ignore) {
+            }
+            throw ex;
+        }
+        return sock;
+    }
+
 }

httpclient/src/main/java/org/apache/http/conn/ssl/SSLSocketFactory.java

@@ -28,6 +28,7 @@
 package org.apache.http.conn.ssl;
 
 import java.io.IOException;
+import java.net.ConnectException;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.Socket;
@@ -51,7 +52,6 @@ import org.apache.http.conn.scheme.HostNameResolver;
 import org.apache.http.conn.scheme.LayeredSchemeSocketFactory;
 import org.apache.http.conn.scheme.LayeredSocketFactory;
 import org.apache.http.conn.scheme.SchemeLayeredSocketFactory;
-import org.apache.http.conn.socket.AbstractConnectionSocketFactory;
 import org.apache.http.conn.socket.LayeredConnectionSocketFactory;
 import org.apache.http.params.HttpConnectionParams;
 import org.apache.http.params.HttpParams;
@@ -92,8 +92,7 @@ import org.apache.http.util.TextUtils;
  */
 @SuppressWarnings("deprecation")
 @ThreadSafe
-public class SSLSocketFactory extends AbstractConnectionSocketFactory
-                              implements LayeredConnectionSocketFactory, SchemeLayeredSocketFactory,
+public class SSLSocketFactory implements LayeredConnectionSocketFactory, SchemeLayeredSocketFactory,
                                          LayeredSchemeSocketFactory, LayeredSocketFactory {
 
     public static final String TLS   = "TLS";
@@ -553,8 +552,19 @@ public class SSLSocketFactory extends AbstractConnectionSocketFactory
             final HttpContext context) throws IOException {
         Args.notNull(host, "HTTP host");
         Args.notNull(remoteAddress, "Remote address");
-        final Socket sock = super.connectSocket(
-                connectTimeout, socket, host, remoteAddress, localAddress, context);
+        final Socket sock = socket != null ? socket : createSocket(context);
+        if (localAddress != null) {
+            sock.bind(localAddress);
+        }
+        try {
+            sock.connect(remoteAddress, connectTimeout);
+        } catch (final IOException ex) {
+            try {
+                sock.close();
+            } catch (final IOException ignore) {
+            }
+            throw ex;
+        }
         // Setup SSL layering if necessary
         if (sock instanceof SSLSocket) {
             final SSLSocket sslsock = (SSLSocket) sock;

httpclient/src/main/java/org/apache/http/impl/conn/HttpClientConnectionOperator.java

@@ -31,6 +31,7 @@ import java.net.ConnectException;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.Socket;
+import java.net.SocketTimeoutException;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -42,6 +43,7 @@ import org.apache.http.config.SocketConfig;
 import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.http.conn.DnsResolver;
 import org.apache.http.conn.HttpClientConnectionManager;
+import org.apache.http.conn.HttpHostConnectException;
 import org.apache.http.conn.ManagedHttpClientConnection;
 import org.apache.http.conn.SchemePortResolver;
 import org.apache.http.conn.UnsupportedSchemeException;
@@ -123,13 +125,18 @@ class HttpClientConnectionOperator {
                 }
                 conn.bind(sock);
                 return;
+            } catch (final SocketTimeoutException ex) {
+                if (last) {
+                    throw new ConnectTimeoutException(ex, host, addresses);
+                }
             } catch (final ConnectException ex) {
                 if (last) {
-                    throw ex;
-                }
-            } catch (final ConnectTimeoutException ex) {
-                if (last) {
-                    throw ex;
+                    final String msg = ex.getMessage();
+                    if ("Connection timed out".equals(msg)) {
+                        throw new ConnectTimeoutException(ex, host, addresses);
+                    } else {
+                        throw new HttpHostConnectException(ex, host, addresses);
+                    }
                 }
             }
             if (this.log.isDebugEnabled()) {

httpclient/src/test/java/org/apache/http/conn/TestExceptions.java

@@ -33,7 +33,6 @@ import org.junit.Test;
 
 import java.io.IOException;
 import java.net.InetAddress;
-import java.net.InetSocketAddress;
 
 /**
  * Unit tests for exceptions.
@@ -62,7 +61,7 @@ public class TestExceptions {
     @Test
     public void testConnectTimeoutExceptionFromCause() {
         final IOException cause = new IOException("something awful");
-        final ConnectTimeoutException ctx = new ConnectTimeoutException(null, null, cause);
+        final ConnectTimeoutException ctx = new ConnectTimeoutException(cause, null, null);
         Assert.assertEquals("Connect to remote host failed: something awful", ctx.getMessage());
     }
 
@@ -70,18 +69,17 @@ public class TestExceptions {
     public void testConnectTimeoutExceptionFromCauseAndHost() {
         final HttpHost target = new HttpHost("localhost");
         final IOException cause = new IOException();
-        final ConnectTimeoutException ctx = new ConnectTimeoutException(target, null, cause);
+        final ConnectTimeoutException ctx = new ConnectTimeoutException(cause, target, null);
         Assert.assertEquals("Connect to localhost timed out", ctx.getMessage());
     }
 
     @Test
     public void testConnectTimeoutExceptionFromCauseHostAndRemoteAddress() throws Exception {
         final HttpHost target = new HttpHost("localhost");
-        final InetSocketAddress remoteAddress = new InetSocketAddress(
-                InetAddress.getByAddress(new byte[] {1,2,3,4}), 1234);
+        final InetAddress remoteAddress = InetAddress.getByAddress(new byte[] {1,2,3,4});
         final IOException cause = new IOException();
-        final ConnectTimeoutException ctx = new ConnectTimeoutException(target, remoteAddress, cause);
-        Assert.assertEquals("Connect to localhost (/1.2.3.4) timed out", ctx.getMessage());
+        final ConnectTimeoutException ctx = new ConnectTimeoutException(cause, target, remoteAddress);
+        Assert.assertEquals("Connect to localhost [/1.2.3.4] timed out", ctx.getMessage());
     }
 
     @Test
@@ -93,7 +91,7 @@ public class TestExceptions {
     @Test
     public void testHttpHostConnectExceptionFromCause() {
         final IOException cause = new IOException("something awful");
-        final HttpHostConnectException ctx = new HttpHostConnectException(null, null, cause);
+        final HttpHostConnectException ctx = new HttpHostConnectException(cause, null, null);
         Assert.assertEquals("Connect to remote host failed: something awful", ctx.getMessage());
     }
 
@@ -101,18 +99,19 @@ public class TestExceptions {
     public void testHttpHostConnectExceptionFromCauseAndHost() {
         final HttpHost target = new HttpHost("localhost");
         final IOException cause = new IOException();
-        final HttpHostConnectException ctx = new HttpHostConnectException(target, null, cause);
+        final HttpHostConnectException ctx = new HttpHostConnectException(cause, target, null);
         Assert.assertEquals("Connect to localhost refused", ctx.getMessage());
     }
 
     @Test
     public void testHttpHostConnectExceptionFromCauseHostAndRemoteAddress() throws Exception {
         final HttpHost target = new HttpHost("localhost");
-        final InetSocketAddress remoteAddress = new InetSocketAddress(
-                InetAddress.getByAddress(new byte[] {1,2,3,4}), 1234);
+        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(target, remoteAddress, cause);
-        Assert.assertEquals("Connect to localhost (/1.2.3.4) refused", ctx.getMessage());
+        final HttpHostConnectException ctx = new HttpHostConnectException(cause, target,
+                remoteAddress1, remoteAddress2);
+        Assert.assertEquals("Connect to localhost [/1.2.3.4, /5.6.7.8] refused", ctx.getMessage());
     }
 
     @Test

httpclient/src/test/java/org/apache/http/impl/conn/TestHttpClientConnectionOperator.java

@@ -27,15 +27,18 @@
 
 package org.apache.http.impl.conn;
 
+import java.net.ConnectException;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.Socket;
+import java.net.SocketTimeoutException;
 
 import org.apache.http.HttpHost;
 import org.apache.http.config.Lookup;
 import org.apache.http.config.SocketConfig;
 import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.http.conn.DnsResolver;
+import org.apache.http.conn.HttpHostConnectException;
 import org.apache.http.conn.SchemePortResolver;
 import org.apache.http.conn.ManagedHttpClientConnection;
 import org.apache.http.conn.UnsupportedSchemeException;
@@ -119,6 +122,28 @@ public class TestHttpClientConnectionOperator {
     }
 
     @Test(expected=ConnectTimeoutException.class)
+    public void testConnectTimeout() throws Exception {
+        final HttpContext context = new BasicHttpContext();
+        final HttpHost host = new HttpHost("somehost");
+        final InetAddress ip1 = InetAddress.getByAddress(new byte[] {10, 0, 0, 1});
+        final InetAddress ip2 = InetAddress.getByAddress(new byte[] {10, 0, 0, 2});
+
+        Mockito.when(dnsResolver.resolve("somehost")).thenReturn(new InetAddress[] { ip1, ip2 });
+        Mockito.when(socketFactoryRegistry.lookup("http")).thenReturn(plainSocketFactory);
+        Mockito.when(schemePortResolver.resolve(host)).thenReturn(80);
+        Mockito.when(plainSocketFactory.createSocket(Mockito.<HttpContext>any())).thenReturn(socket);
+        Mockito.when(plainSocketFactory.connectSocket(
+                Mockito.anyInt(),
+                Mockito.<Socket>any(),
+                Mockito.<HttpHost>any(),
+                Mockito.<InetSocketAddress>any(),
+                Mockito.<InetSocketAddress>any(),
+                Mockito.<HttpContext>any())).thenThrow(new SocketTimeoutException());
+
+        connectionOperator.connect(conn, host, null, 1000, SocketConfig.DEFAULT, context);
+    }
+
+    @Test(expected=HttpHostConnectException.class)
     public void testConnectFailure() throws Exception {
         final HttpContext context = new BasicHttpContext();
         final HttpHost host = new HttpHost("somehost");
@@ -135,7 +160,7 @@ public class TestHttpClientConnectionOperator {
                 Mockito.<HttpHost>any(),
                 Mockito.<InetSocketAddress>any(),
                 Mockito.<InetSocketAddress>any(),
-                Mockito.<HttpContext>any())).thenThrow(new ConnectTimeoutException());
+                Mockito.<HttpContext>any())).thenThrow(new ConnectException());
 
         connectionOperator.connect(conn, host, null, 1000, SocketConfig.DEFAULT, context);
     }
@@ -158,7 +183,7 @@ public class TestHttpClientConnectionOperator {
                 Mockito.<HttpHost>any(),
                 Mockito.eq(new InetSocketAddress(ip1, 80)),
                 Mockito.<InetSocketAddress>any(),
-                Mockito.<HttpContext>any())).thenThrow(new ConnectTimeoutException());
+                Mockito.<HttpContext>any())).thenThrow(new ConnectException());
         Mockito.when(plainSocketFactory.connectSocket(
                 Mockito.anyInt(),
                 Mockito.<Socket>any(),