Index: module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java =================================================================== --- module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java (revision 0) +++ module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java (revision 0) @@ -0,0 +1,131 @@ +package org.apache.http.impl.client; + +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import junit.framework.Test; +import junit.framework.TestCase; +import junit.framework.TestSuite; + +import org.apache.http.client.methods.AbortableHttpRequest; +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.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.mockup.SocketFactoryMockup; +import org.apache.http.params.BasicHttpParams; +import org.apache.http.params.HttpParams; +import org.apache.http.protocol.HttpContext; + +/** + * Unit tests for {@link DefaultClientRequestDirector} + */ +public class TestDefaultClientRequestDirector extends TestCase { + + 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 abort is called on an {@link AbortableHttpRequest} while + * {@link DefaultClientRequestDirector} is allocating a connection, that the + * connection is properly aborted. + */ + public void testAbortInAllocate() throws Exception { + CountDownLatch connLatch = new CountDownLatch(1); + CountDownLatch awaitLatch = new CountDownLatch(1); + final ConMan conMan = new ConMan(connLatch, awaitLatch); + final AtomicReference throwableRef = new AtomicReference(); + final CountDownLatch getLatch = new CountDownLatch(1); + final DefaultHttpClient client = new DefaultHttpClient(conMan, new BasicHttpParams()); + final HttpContext context = client.getDefaultContext(); + final HttpGet httpget = new HttpGet("http://www.example.com/a"); + + new Thread(new Runnable() { + public void run() { + try { + client.execute(httpget, context); + } catch(Throwable t) { + throwableRef.set(t); + } finally { + getLatch.countDown(); + } + } + }).start(); + + assertTrue("should have tried to get a connection", connLatch.await(1, TimeUnit.SECONDS)); + + httpget.abort(); + + assertTrue("should have finished get request", getLatch.await(1, TimeUnit.SECONDS)); + assertTrue("should be instanceof InterruptedException, was: " + throwableRef.get(), + throwableRef.get() instanceof InterruptedException); + } + + private static class ConMan implements ClientConnectionManager { + private final CountDownLatch connLatch; + private final CountDownLatch awaitLatch; + + public ConMan(CountDownLatch connLatch, CountDownLatch awaitLatch) { + this.connLatch = connLatch; + this.awaitLatch = awaitLatch; + } + + 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 { + connLatch.countDown(); // notify waiter that we're getting a connection + + // zero usually means sleep forever, but CountDownLatch doesn't interpret it that way. + if(timeout == 0) + timeout = Integer.MAX_VALUE; + + if(!awaitLatch.await(timeout, tunit)) + throw new ConnectionPoolTimeoutException(); + + return new ClientConnAdapterMockup(); + } + + 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) { + throw new UnsupportedOperationException("just a mockup"); + } + + public void shutdown() { + throw new UnsupportedOperationException("just a mockup"); + } + } +} \ No newline at end of file Property changes on: module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java ___________________________________________________________________ Name: svn:executable + * Index: module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java =================================================================== --- module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java (revision 637924) +++ module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java (working copy) @@ -47,8 +47,8 @@ import org.apache.http.HttpHost; import org.apache.http.HttpRequest; import org.apache.http.HttpResponse; +import org.apache.http.ProtocolException; import org.apache.http.ProtocolVersion; -import org.apache.http.ProtocolException; import org.apache.http.auth.AuthScheme; import org.apache.http.auth.AuthScope; import org.apache.http.auth.AuthenticationException; @@ -70,20 +70,21 @@ import org.apache.http.conn.BasicManagedEntity; import org.apache.http.conn.ClientConnectionManager; import org.apache.http.conn.ConnectionPoolTimeoutException; +import org.apache.http.conn.ConnectionReleaseTrigger; +import org.apache.http.conn.ManagedClientConnection; +import org.apache.http.conn.Scheme; +import org.apache.http.conn.routing.BasicRouteDirector; import org.apache.http.conn.routing.HttpRoute; +import org.apache.http.conn.routing.HttpRouteDirector; import org.apache.http.conn.routing.HttpRoutePlanner; -import org.apache.http.conn.routing.HttpRouteDirector; -import org.apache.http.conn.routing.BasicRouteDirector; -import org.apache.http.conn.Scheme; -import org.apache.http.conn.ManagedClientConnection; import org.apache.http.entity.BufferedHttpEntity; import org.apache.http.message.BasicHttpRequest; import org.apache.http.params.HttpConnectionParams; import org.apache.http.params.HttpParams; import org.apache.http.params.HttpProtocolParams; +import org.apache.http.protocol.ExecutionContext; import org.apache.http.protocol.HTTP; import org.apache.http.protocol.HttpContext; -import org.apache.http.protocol.ExecutionContext; import org.apache.http.protocol.HttpProcessor; import org.apache.http.protocol.HttpRequestExecutor; @@ -287,15 +288,18 @@ // request is still available in 'orig'. HttpRoute route = roureq.getRoute(); + + ReleaseTrigger releaseTrigger = new ReleaseTrigger(); + if (orig instanceof AbortableHttpRequest) { + ((AbortableHttpRequest) orig).setReleaseTrigger(releaseTrigger); + } // Allocate connection if needed if (managedConn == null) { managedConn = allocateConnection(route, timeout); } - if (orig instanceof AbortableHttpRequest) { - ((AbortableHttpRequest) orig).setReleaseTrigger(managedConn); - } + releaseTrigger.setTrigger(managedConn); // Reopen connection if needed if (!managedConn.isOpen()) { @@ -1026,4 +1030,52 @@ authState.setCredentials(creds); } + /** + * A {@link ConnectionReleaseTrigger} that supports aborting before a delegate + * ConnectionReleaseTrigger has been set. If the connection is aborted prior + * to a delegate trigger being set, the active thread is interrupted. + * If a trigger is set afterwards, an IOException is immediately thrown. + */ + private static class ReleaseTrigger implements ConnectionReleaseTrigger { + private boolean aborted = false; + private ConnectionReleaseTrigger trigger; + private final Thread currentThread; + + public ReleaseTrigger() { + this.currentThread = Thread.currentThread(); + } + + public void abortConnection() throws IOException { + ConnectionReleaseTrigger releaseTrigger; + synchronized(this) { + if(aborted) + throw new IOException("already aborted"); + aborted = true; + releaseTrigger = trigger; // capture reference within lock + } + + if(releaseTrigger == null) { + currentThread.interrupt(); + } else { + releaseTrigger.abortConnection(); + } + } + + public void releaseConnection() throws IOException { + if(trigger != null) + trigger.releaseConnection(); + } + + public void setTrigger(ConnectionReleaseTrigger trigger) throws IOException { + synchronized(this) { + if(aborted) { + // clear the interrupted status, since it's not needed anymore + Thread.interrupted(); + throw new IOException("already aborted!"); + } + this.trigger = trigger; + } + } + } + } // class DefaultClientRequestDirector Index: module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java =================================================================== --- module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java (revision 637924) +++ module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java (working copy) @@ -54,7 +54,9 @@ implements HttpUriRequest, AbortableHttpRequest { private URI uri; - private ConnectionReleaseTrigger releaseTrigger; + // volatile because the trigger is intended to be used from different threads + // than which it's set. + private volatile ConnectionReleaseTrigger releaseTrigger; public HttpRequestBase() { super(); @@ -94,7 +96,7 @@ public void abort() { if (this.releaseTrigger != null) { - try { + try { this.releaseTrigger.abortConnection(); } catch (IOException ex) { // ignore