From d5e1392840fcbd9ff9d9c5b19d8ed9da2883c224 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sat, 24 Aug 2019 15:34:22 +0200 Subject: [PATCH] HTTPCLIENT-2013: revised handling of connect exceptions; improved consistency in behavior of the classic and async clients; ConnectTimeoutException now extends SocketTimeoutException --- .../client5/http/ConnectExceptionSupport.java | 87 +++++++++++++++++++ .../client5/http/ConnectTimeoutException.java | 36 ++------ .../http/HttpHostConnectException.java | 28 ++---- .../DefaultHttpClientConnectionOperator.java | 22 +---- .../impl/nio/MultihomeIOSessionRequester.java | 4 +- ....java => ConnectExceptionSupportTest.java} | 32 +++---- .../http/examples/ClientExecuteSOCKS.java | 8 +- .../TestDefaultHttpRequestRetryHandler.java | 6 +- 8 files changed, 120 insertions(+), 103 deletions(-) create mode 100644 httpclient5/src/main/java/org/apache/hc/client5/http/ConnectExceptionSupport.java rename httpclient5/src/test/java/org/apache/hc/client5/http/{TestExceptions.java => ConnectExceptionSupportTest.java} (75%) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ConnectExceptionSupport.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ConnectExceptionSupport.java new file mode 100644 index 000000000..49ab3972a --- /dev/null +++ b/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 + * . + * + */ +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; + } + } + +} diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ConnectTimeoutException.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ConnectTimeoutException.java index 9b7437718..efb0f0a91 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ConnectTimeoutException.java +++ b/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.io.InterruptedIOException; -import java.net.InetAddress; -import java.util.Arrays; +import java.net.SocketTimeoutException; import org.apache.hc.core5.net.NamedEndpoint; /** - * A timeout while connecting to an HTTP server or waiting for an - * available connection from an HttpConnectionManager. + * A timeout while connecting to an HTTP server or waiting for an available connection from a connection manager. * * @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; } - /** - * Creates a ConnectTimeoutException based on original {@link IOException}. - * - * @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")); + public ConnectTimeoutException(final String message, final NamedEndpoint namedEndpoint) { + super(message); this.namedEndpoint = namedEndpoint; - initCause(cause); } /** diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/HttpHostConnectException.java b/httpclient5/src/main/java/org/apache/hc/client5/http/HttpHostConnectException.java index b2612660f..8ec2be2f3 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/HttpHostConnectException.java +++ b/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 - * being connected to. + * A {@link ConnectException} that specifies the {@link NamedEndpoint} that was 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}. - * - * @since 5.0 + * Creates a HttpHostConnectException with the specified detail message. */ - public HttpHostConnectException( - 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() : " refused")); + public HttpHostConnectException(final String message) { + super(message); + this.namedEndpoint = null; + } + + public HttpHostConnectException(final String message, final NamedEndpoint namedEndpoint) { + super(message); this.namedEndpoint = namedEndpoint; - initCause(cause); } /** diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultHttpClientConnectionOperator.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultHttpClientConnectionOperator.java index 7313ebcd8..2f0b5f28b 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/DefaultHttpClientConnectionOperator.java +++ b/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); - } - } 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; + throw ConnectExceptionSupport.enhance(ex, host, addresses); } } if (this.log.isDebugEnabled()) { diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/MultihomeIOSessionRequester.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/MultihomeIOSessionRequester.java index 9eb518620..db047b4bd 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/MultihomeIOSessionRequester.java +++ b/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); } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/TestExceptions.java b/httpclient5/src/test/java/org/apache/hc/client5/http/ConnectExceptionSupportTest.java similarity index 75% rename from httpclient5/src/test/java/org/apache/hc/client5/http/TestExceptions.java rename to httpclient5/src/test/java/org/apache/hc/client5/http/ConnectExceptionSupportTest.java index bd25fe566..88421237c 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/TestExceptions.java +++ b/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() { - final ConnectTimeoutException ctx = new ConnectTimeoutException(); - 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); + public void testConnectTimeoutExceptionFromNullMessageAndHost() { + final ConnectTimeoutException ctx = ConnectExceptionSupport.createConnectTimeoutException(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()); } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/examples/ClientExecuteSOCKS.java b/httpclient5/src/test/java/org/apache/hc/client5/http/examples/ClientExecuteSOCKS.java index 9c1abf835..29f04a64e 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/examples/ClientExecuteSOCKS.java +++ b/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); - } catch (final SocketTimeoutException ex) { - throw new ConnectTimeoutException(ex, host, remoteAddress.getAddress()); - } + sock.connect(remoteAddress, connectTimeout != null ? connectTimeout.toMillisIntBound() : 0); return sock; } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultHttpRequestRetryHandler.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultHttpRequestRetryHandler.java index 3a77524b5..faf13afb0 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultHttpRequestRetryHandler.java +++ b/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)); } }