diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index f63f99176..5192a0ac3 100644 --- a/RELEASE_NOTES.txt +++ b/RELEASE_NOTES.txt @@ -1,3 +1,11 @@ +Changes since 4.1 ALPHA1 +------------------- + +* [HTTPCLIENT-902] HttpRequestRetryHandler not called on I/O exceptions + thrown when opening a new connection. + Contributed by Olivier Lamy and + Oleg Kalnichevski + Release 4.1 ALPHA1 ------------------- diff --git a/httpclient/src/main/java/org/apache/http/impl/client/DefaultRequestDirector.java b/httpclient/src/main/java/org/apache/http/impl/client/DefaultRequestDirector.java index 3ad872293..0f537de29 100644 --- a/httpclient/src/main/java/org/apache/http/impl/client/DefaultRequestDirector.java +++ b/httpclient/src/main/java/org/apache/http/impl/client/DefaultRequestDirector.java @@ -183,6 +183,8 @@ public class DefaultRequestDirector implements RequestDirector { protected final AuthState targetAuthState; protected final AuthState proxyAuthState; + + private int execCount; private int redirectCount; @@ -297,6 +299,7 @@ public DefaultRequestDirector( this.managedConn = null; + this.execCount = 0; this.redirectCount = 0; this.maxRedirects = this.params.getIntParameter(ClientPNames.MAX_REDIRECTS, 100); this.targetAuthState = new AuthState(); @@ -361,8 +364,6 @@ public HttpResponse execute(HttpHost target, HttpRequest request, long timeout = ConnManagerParams.getTimeout(params); - int execCount = 0; - boolean reuse = false; boolean done = false; try { @@ -412,15 +413,8 @@ public HttpResponse execute(HttpHost target, HttpRequest request, ((AbortableHttpRequest) orig).setReleaseTrigger(managedConn); } - // Reopen connection if needed - if (!managedConn.isOpen()) { - managedConn.open(route, context, params); - } else { - managedConn.setSocketTimeout(HttpConnectionParams.getSoTimeout(params)); - } - try { - establishRoute(route, context); + tryConnect(roureq, context); } catch (TunnelRefusedException ex) { if (this.log.isDebugEnabled()) { this.log.debug(ex.getMessage()); @@ -459,68 +453,7 @@ public HttpResponse execute(HttpHost target, HttpRequest request, // Run request protocol interceptors requestExec.preProcess(wrapper, httpProcessor, context); - boolean retrying = true; - Exception retryReason = null; - while (retrying) { - // Increment total exec count (with redirects) - execCount++; - // Increment exec count for this particular request - wrapper.incrementExecCount(); - if (wrapper.getExecCount() > 1 && !wrapper.isRepeatable()) { - this.log.debug("Cannot retry non-repeatable request"); - if (retryReason != null) { - throw new NonRepeatableRequestException("Cannot retry request " + - "with a non-repeatable request entity. The cause lists the " + - "reason the original request failed.", retryReason); - } else { - throw new NonRepeatableRequestException("Cannot retry request " + - "with a non-repeatable request entity."); - } - } - - try { - if (this.log.isDebugEnabled()) { - this.log.debug("Attempt " + execCount + " to execute request"); - } - response = requestExec.execute(wrapper, managedConn, context); - retrying = false; - - } catch (IOException ex) { - this.log.debug("Closing the connection."); - try { - managedConn.close(); - } catch (IOException ignore) { - } - if (retryHandler.retryRequest(ex, execCount, context)) { - if (this.log.isInfoEnabled()) { - this.log.info("I/O exception ("+ ex.getClass().getName() + - ") caught when processing request: " - + ex.getMessage()); - } - if (this.log.isDebugEnabled()) { - this.log.debug(ex.getMessage(), ex); - } - this.log.info("Retrying request"); - retryReason = ex; - } else { - throw ex; - } - - // If we have a direct route to the target host - // just re-open connection and re-try the request - if (!route.isTunnelled()) { - this.log.debug("Reopening the direct connection."); - managedConn.open(route, context, params); - } else { - // otherwise give up - this.log.debug("Proxied connection. Need to start over."); - retrying = false; - } - - } - - } - + response = tryExecute(roureq, context); if (response == null) { // Need to start over continue; @@ -614,6 +547,120 @@ public HttpResponse execute(HttpHost target, HttpRequest request, } } // execute + /** + * Establish connection either directly or through a tunnel and retry in case of + * a recoverable I/O failure + */ + private void tryConnect( + final RoutedRequest req, final HttpContext context) throws HttpException, IOException { + HttpRoute route = req.getRoute(); + + boolean retrying = true; + while (retrying) { + // Increment total exec count + execCount++; + try { + if (!managedConn.isOpen()) { + managedConn.open(route, context, params); + } else { + managedConn.setSocketTimeout(HttpConnectionParams.getSoTimeout(params)); + } + establishRoute(route, context); + retrying = false; + } catch (IOException ex) { + try { + managedConn.close(); + } catch (IOException ignore) { + } + if (retryHandler.retryRequest(ex, execCount, context)) { + if (this.log.isInfoEnabled()) { + this.log.info("I/O exception ("+ ex.getClass().getName() + + ") caught when connecting to the target host: " + + ex.getMessage()); + } + if (this.log.isDebugEnabled()) { + this.log.debug(ex.getMessage(), ex); + } + this.log.info("Retrying connect"); + } else { + throw ex; + } + } + } + } + + /** + * Execute request and retry in case of a recoverable I/O failure + */ + private HttpResponse tryExecute( + final RoutedRequest req, final HttpContext context) throws HttpException, IOException { + RequestWrapper wrapper = req.getRequest(); + HttpRoute route = req.getRoute(); + HttpResponse response = null; + + boolean retrying = true; + Exception retryReason = null; + while (retrying) { + // Increment total exec count (with redirects) + execCount++; + // Increment exec count for this particular request + wrapper.incrementExecCount(); + if (wrapper.getExecCount() > 1 && !wrapper.isRepeatable()) { + this.log.debug("Cannot retry non-repeatable request"); + if (retryReason != null) { + throw new NonRepeatableRequestException("Cannot retry request " + + "with a non-repeatable request entity. The cause lists the " + + "reason the original request failed.", retryReason); + } else { + throw new NonRepeatableRequestException("Cannot retry request " + + "with a non-repeatable request entity."); + } + } + + try { + if (this.log.isDebugEnabled()) { + this.log.debug("Attempt " + execCount + " to execute request"); + } + response = requestExec.execute(wrapper, managedConn, context); + retrying = false; + + } catch (IOException ex) { + this.log.debug("Closing the connection."); + try { + managedConn.close(); + } catch (IOException ignore) { + } + if (retryHandler.retryRequest(ex, execCount, context)) { + if (this.log.isInfoEnabled()) { + this.log.info("I/O exception ("+ ex.getClass().getName() + + ") caught when processing request: " + + ex.getMessage()); + } + if (this.log.isDebugEnabled()) { + this.log.debug(ex.getMessage(), ex); + } + this.log.info("Retrying request"); + retryReason = ex; + } else { + throw ex; + } + + // If we have a direct route to the target host + // just re-open connection and re-try the request + if (!route.isTunnelled()) { + this.log.debug("Reopening the direct connection."); + managedConn.open(route, context, params); + } else { + // otherwise give up + this.log.debug("Proxied connection. Need to start over."); + retrying = false; + } + + } + } + return response; + } + /** * Returns the connection back to the connection manager * and prepares for retrieving a new connection during diff --git a/httpclient/src/test/java/org/apache/http/impl/client/TestRequestRetryHandler.java b/httpclient/src/test/java/org/apache/http/impl/client/TestRequestRetryHandler.java new file mode 100644 index 000000000..fff1df80f --- /dev/null +++ b/httpclient/src/test/java/org/apache/http/impl/client/TestRequestRetryHandler.java @@ -0,0 +1,98 @@ +/* + * ==================================================================== + * + * 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.http.impl.client; + +import java.io.IOException; +import java.net.UnknownHostException; + +import junit.framework.TestCase; + +import org.apache.http.client.HttpRequestRetryHandler; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpRequestBase; +import org.apache.http.conn.ClientConnectionManager; +import org.apache.http.conn.scheme.PlainSocketFactory; +import org.apache.http.conn.scheme.Scheme; +import org.apache.http.conn.scheme.SchemeRegistry; +import org.apache.http.impl.conn.SingleClientConnManager; +import org.apache.http.impl.conn.tsccm.ThreadSafeClientConnManager; +import org.apache.http.params.HttpConnectionParams; +import org.apache.http.protocol.HttpContext; + +public class TestRequestRetryHandler extends TestCase { + + public void testUseRetryHandlerInConnectionTimeOutWithThreadSafeClientConnManager() + throws Exception { + + SchemeRegistry schemeRegistry = new SchemeRegistry(); + schemeRegistry.register(new Scheme("http", PlainSocketFactory.getSocketFactory(), 80)); + ClientConnectionManager connManager = new ThreadSafeClientConnManager(schemeRegistry); + + assertOnRetry(connManager); + } + + public void testUseRetryHandlerInConnectionTimeOutWithSingleClientConnManager() + throws Exception { + + SchemeRegistry schemeRegistry = new SchemeRegistry(); + schemeRegistry.register(new Scheme("http", PlainSocketFactory.getSocketFactory(), 80)); + ClientConnectionManager connManager = new SingleClientConnManager(schemeRegistry); + assertOnRetry(connManager); + } + + protected void assertOnRetry(ClientConnectionManager connManager) throws Exception { + SchemeRegistry schemeRegistry = new SchemeRegistry(); + schemeRegistry.register(new Scheme("http", PlainSocketFactory.getSocketFactory(), 80)); + + DefaultHttpClient client = new DefaultHttpClient(connManager); + TestHttpRequestRetryHandler testRetryHandler = new TestHttpRequestRetryHandler(); + client.setHttpRequestRetryHandler(testRetryHandler); + + HttpRequestBase request = new HttpGet("http://www.complete.garbage"); + + HttpConnectionParams.setConnectionTimeout(request.getParams(), 1); + try { + client.execute(request); + } catch (UnknownHostException ex) { + assertEquals(2, testRetryHandler.retryNumber); + } + } + + static class TestHttpRequestRetryHandler implements HttpRequestRetryHandler { + + int retryNumber = 0; + + public boolean retryRequest(IOException exception, int executionCount, HttpContext context) { + retryNumber++; + if (executionCount < 2) { + return true; + } + return false; + } + + } + +}