diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index 38de972b6..ec42dfee8 100644 --- a/RELEASE_NOTES.txt +++ b/RELEASE_NOTES.txt @@ -1,6 +1,14 @@ Changes since 4.0 Alpha 3 ------------------- +* [HTTPCLIENT-759] Ensure release of connections back to the connection manager + on exceptions. + Contributed by Sam Berlin + +* [HTTPCLIENT-759] DefaultClientRequestDirector now releases connections back + to the connection manager on exceptions. + Contributed by Sam Berlin + * [HTTPCLIENT-758] Fixed the use of generics in AbstractHttpClient #removeRequestInterceptorByClass and #removeResponseInterceptorByClass Contributed by Johannes Koch diff --git a/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java b/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java index eaac6b171..301c701ff 100644 --- a/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java +++ b/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java @@ -403,8 +403,7 @@ public class DefaultClientRequestDirector managedConn.close(); } // check if we can use the same connection for the followup - if ((managedConn != null) && - !followup.getRoute().equals(roureq.getRoute())) { + if (!followup.getRoute().equals(roureq.getRoute())) { // the followup has a different route, release conn //@@@ need to consume response body first? //@@@ or let that be done in handleResponse(...)? @@ -941,6 +940,8 @@ public class DefaultClientRequestDirector // we got here as the result of an exception // no response will be returned, release the connection managedConn = null; + // ensure the connection manager properly releases this connection + connManager.releaseConnection(mcc); //@@@ is the connection in a re-usable state? consume response? //@@@ for now, just shut it down try { diff --git a/module-client/src/test/java/org/apache/http/impl/client/TestAllHttpClientImpl.java b/module-client/src/test/java/org/apache/http/impl/client/TestAllHttpClientImpl.java index bbf224895..da8e91ef2 100644 --- a/module-client/src/test/java/org/apache/http/impl/client/TestAllHttpClientImpl.java +++ b/module-client/src/test/java/org/apache/http/impl/client/TestAllHttpClientImpl.java @@ -44,6 +44,7 @@ public class TestAllHttpClientImpl extends TestCase { TestSuite suite = new TestSuite(); suite.addTest(TestBasicCredentialsProvider.suite()); suite.addTest(TestRequestWrapper.suite()); + suite.addTest(TestDefaultClientRequestDirector.suite()); return suite; } diff --git a/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java b/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java new file mode 100644 index 000000000..8c0fa4fef --- /dev/null +++ b/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java @@ -0,0 +1,194 @@ +/* + * $HeadURL:$ + * $Revision:$ + * $Date:$ + * ==================================================================== + * + * 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.ConnectException; +import java.util.concurrent.TimeUnit; + +import junit.framework.Test; +import junit.framework.TestSuite; + +import org.apache.http.HttpException; +import org.apache.http.HttpRequest; +import org.apache.http.HttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.conn.ClientConnectionManager; +import org.apache.http.conn.ConnectionPoolTimeoutException; +import org.apache.http.conn.ManagedClientConnection; +import org.apache.http.conn.PlainSocketFactory; +import org.apache.http.conn.Scheme; +import org.apache.http.conn.SchemeRegistry; +import org.apache.http.conn.routing.HttpRoute; +import org.apache.http.impl.conn.ClientConnAdapterMockup; +import org.apache.http.impl.conn.SingleClientConnManager; +import org.apache.http.localserver.ServerTestBase; +import org.apache.http.mockup.SocketFactoryMockup; +import org.apache.http.params.BasicHttpParams; +import org.apache.http.params.HttpParams; +import org.apache.http.protocol.HttpContext; +import org.apache.http.protocol.HttpRequestHandler; + +/** + * Unit tests for {@link DefaultClientRequestDirector} + */ +public class TestDefaultClientRequestDirector extends ServerTestBase { + + public TestDefaultClientRequestDirector(final String testName) throws IOException { + super(testName); + } + + public static void main(String args[]) { + String[] testCaseName = { TestDefaultClientRequestDirector.class.getName() }; + junit.textui.TestRunner.main(testCaseName); + } + + public static Test suite() { + return new TestSuite(TestDefaultClientRequestDirector.class); + } + + /** + * Tests that if a socket fails to connect, the allocated connection is + * properly released back to the connection manager. + */ + public void testSocketConnectFailureReleasesConnection() throws Exception { + final ConnMan2 conMan = new ConnMan2(); + final DefaultHttpClient client = new DefaultHttpClient(conMan, new BasicHttpParams()); + final HttpContext context = client.getDefaultContext(); + final HttpGet httpget = new HttpGet("http://www.example.com/a"); + + try { + client.execute(httpget, context); + fail("expected IOException"); + } catch(IOException expected) {} + + assertNotNull(conMan.allocatedConnection); + assertSame(conMan.allocatedConnection, conMan.releasedConnection); + } + + public void testRequestFailureReleasesConnection() throws Exception { + this.localServer.register("*", new ThrowingService()); + + SchemeRegistry registry = new SchemeRegistry(); + registry.register(new Scheme("http", PlainSocketFactory.getSocketFactory(), 80)); + + ConnMan3 conMan = new ConnMan3(new BasicHttpParams(), registry); + DefaultHttpClient client = new DefaultHttpClient(conMan, new BasicHttpParams()); + HttpGet httpget = new HttpGet("/a"); + + try { + client.execute(getServerHttp(), httpget); + fail("expected IOException"); + } catch (IOException expected) {} + + assertNotNull(conMan.allocatedConnection); + assertSame(conMan.allocatedConnection, conMan.releasedConnection); + } + + private static class ThrowingService implements HttpRequestHandler { + public void handle( + final HttpRequest request, + final HttpResponse response, + final HttpContext context) throws HttpException, IOException { + throw new IOException(); + } + } + + private static class ConnMan3 extends SingleClientConnManager { + private ManagedClientConnection allocatedConnection; + private ManagedClientConnection releasedConnection; + + public ConnMan3(HttpParams params, SchemeRegistry schreg) { + super(params, schreg); + } + + @Override + public ManagedClientConnection getConnection(HttpRoute route) { + allocatedConnection = super.getConnection(route); + return allocatedConnection; + } + + @Override + public void releaseConnection(ManagedClientConnection conn) { + releasedConnection = conn; + super.releaseConnection(conn); + } + + + } + + private static class ConnMan2 implements ClientConnectionManager { + private ManagedClientConnection allocatedConnection; + private ManagedClientConnection releasedConnection; + + public ConnMan2() { + } + + public void closeIdleConnections(long idletime, TimeUnit tunit) { + throw new UnsupportedOperationException("just a mockup"); + } + + public ManagedClientConnection getConnection(HttpRoute route) + throws InterruptedException { + throw new UnsupportedOperationException("just a mockup"); + } + + public ManagedClientConnection getConnection(HttpRoute route, + long timeout, TimeUnit tunit) + throws ConnectionPoolTimeoutException, InterruptedException { + allocatedConnection = new ClientConnAdapterMockup() { + @Override + public void open(HttpRoute route, HttpContext context, + HttpParams params) throws IOException { + throw new ConnectException(); + } + }; + return allocatedConnection; + } + + public HttpParams getParams() { + throw new UnsupportedOperationException("just a mockup"); + } + + public SchemeRegistry getSchemeRegistry() { + SchemeRegistry registry = new SchemeRegistry(); + registry.register(new Scheme("http", new SocketFactoryMockup(null), 80)); + return registry; + } + + public void releaseConnection(ManagedClientConnection conn) { + this.releasedConnection = conn; + } + + public void shutdown() { + throw new UnsupportedOperationException("just a mockup"); + } + } + +} diff --git a/module-client/src/test/java/org/apache/http/impl/conn/ClientConnAdapterMockup.java b/module-client/src/test/java/org/apache/http/impl/conn/ClientConnAdapterMockup.java index b31f4f14d..5ddc3ea31 100644 --- a/module-client/src/test/java/org/apache/http/impl/conn/ClientConnAdapterMockup.java +++ b/module-client/src/test/java/org/apache/http/impl/conn/ClientConnAdapterMockup.java @@ -30,6 +30,8 @@ package org.apache.http.impl.conn; +import java.io.IOException; + import org.apache.http.HttpHost; import org.apache.http.conn.routing.HttpRoute; import org.apache.http.params.HttpParams; @@ -57,7 +59,7 @@ public class ClientConnAdapterMockup extends AbstractClientConnAdapter { throw new UnsupportedOperationException("just a mockup"); } - public void open(HttpRoute route, HttpContext context, HttpParams params) { + public void open(HttpRoute route, HttpContext context, HttpParams params) throws IOException { throw new UnsupportedOperationException("just a mockup"); }