Index: C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/test/java/org/apache/http/impl/conn/ExecReqThread.java =================================================================== --- C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/test/java/org/apache/http/impl/conn/ExecReqThread.java (revision 639523) +++ C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/test/java/org/apache/http/impl/conn/ExecReqThread.java (working copy) @@ -51,7 +51,7 @@ public class ExecReqThread extends GetConnThread { protected final ClientConnectionManager conn_manager; - protected final RequestSpec request_spec; + protected final RequestSpec request_spec; protected volatile HttpResponse response; protected volatile byte[] response_data; Index: C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java =================================================================== --- C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java (revision 639523) +++ C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/test/java/org/apache/http/impl/client/TestDefaultClientRequestDirector.java (working copy) @@ -30,6 +30,7 @@ import java.io.IOException; import java.net.ConnectException; +import java.net.URISyntaxException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; @@ -38,20 +39,28 @@ import junit.framework.TestSuite; import org.apache.http.HttpException; +import org.apache.http.HttpHost; import org.apache.http.HttpRequest; import org.apache.http.HttpResponse; +import org.apache.http.HttpStatus; +import org.apache.http.ProtocolVersion; +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.ClientConnectionRequest; import org.apache.http.conn.ConnectionPoolTimeoutException; +import org.apache.http.conn.ConnectionReleaseTrigger; 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.entity.StringEntity; import org.apache.http.impl.conn.ClientConnAdapterMockup; import org.apache.http.impl.conn.SingleClientConnManager; +import org.apache.http.impl.conn.tsccm.ThreadSafeClientConnManager; import org.apache.http.localserver.ServerTestBase; +import org.apache.http.message.BasicHeader; import org.apache.http.mockup.SocketFactoryMockup; import org.apache.http.params.BasicHttpParams; import org.apache.http.params.HttpParams; @@ -108,12 +117,144 @@ 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); + assertTrue("should be instanceof IOException, was: " + throwableRef.get(), + throwableRef.get() instanceof IOException); + assertTrue("cause should be InterruptedException, was: " + throwableRef.get().getCause(), + throwableRef.get().getCause() instanceof InterruptedException); } + /** + * Tests that an abort called after the connection has been retrieved + * but before a release trigger is set does still abort the request. + */ + public void testAbortAfterAllocateBeforeRequest() throws Exception { + this.localServer.register("*", new BasicService()); + + CountDownLatch releaseLatch = new CountDownLatch(1); + SchemeRegistry registry = new SchemeRegistry(); + registry.register(new Scheme("http", PlainSocketFactory.getSocketFactory(), 80)); + + SingleClientConnManager conMan = new SingleClientConnManager(new BasicHttpParams(), registry); + 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 CustomGet("a", releaseLatch); + + new Thread(new Runnable() { + public void run() { + try { + client.execute(getServerHttp(), httpget, context); + } catch(Throwable t) { + throwableRef.set(t); + } finally { + getLatch.countDown(); + } + } + }).start(); + + Thread.sleep(100); // Give it a little time to proceed to release... + + httpget.abort(); + + releaseLatch.countDown(); + + assertTrue("should have finished get request", getLatch.await(1, TimeUnit.SECONDS)); + assertTrue("should be instanceof IOException, was: " + throwableRef.get(), + throwableRef.get() instanceof IOException); + } /** + * Tests that an abort called completely before execute + * still aborts the request. + */ + public void testAbortBeforeExecute() throws Exception { + this.localServer.register("*", new BasicService()); + + SchemeRegistry registry = new SchemeRegistry(); + registry.register(new Scheme("http", PlainSocketFactory.getSocketFactory(), 80)); + + SingleClientConnManager conMan = new SingleClientConnManager(new BasicHttpParams(), registry); + final AtomicReference throwableRef = new AtomicReference(); + final CountDownLatch getLatch = new CountDownLatch(1); + final CountDownLatch startLatch = new CountDownLatch(1); + final DefaultHttpClient client = new DefaultHttpClient(conMan, new BasicHttpParams()); + final HttpContext context = client.getDefaultContext(); + final HttpGet httpget = new HttpGet("a"); + + new Thread(new Runnable() { + public void run() { + try { + try { + if(!startLatch.await(1, TimeUnit.SECONDS)) + throw new RuntimeException("Took too long to start!"); + } catch(InterruptedException interrupted) { + throw new RuntimeException("Never started!", interrupted); + } + client.execute(getServerHttp(), httpget, context); + } catch(Throwable t) { + throwableRef.set(t); + } finally { + getLatch.countDown(); + } + } + }).start(); + + httpget.abort(); + startLatch.countDown(); + + assertTrue("should have finished get request", getLatch.await(1, TimeUnit.SECONDS)); + assertTrue("should be instanceof IOException, was: " + throwableRef.get(), + throwableRef.get() instanceof IOException); + } + + /** + * Tests that an abort called after a redirect has found a new host + * still aborts in the correct place (while trying to get the new + * host's route, not while doing the subsequent request). + */ + public void testAbortAfterRedirectedRoute() throws Exception { + final int port = this.localServer.getServicePort(); + this.localServer.register("*", new BasicRedirectService(port)); + + SchemeRegistry registry = new SchemeRegistry(); + registry.register(new Scheme("http", PlainSocketFactory.getSocketFactory(), 80)); + + CountDownLatch connLatch = new CountDownLatch(1); + CountDownLatch awaitLatch = new CountDownLatch(1); + ConnMan4 conMan = new ConnMan4(new BasicHttpParams(), registry, 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("a"); + + new Thread(new Runnable() { + public void run() { + try { + HttpHost host = new HttpHost("127.0.0.1", port); + client.execute(host, 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 IOException, was: " + throwableRef.get(), + throwableRef.get() instanceof IOException); + assertTrue("cause should be InterruptedException, was: " + throwableRef.get().getCause(), + throwableRef.get().getCause() instanceof InterruptedException); + } + + + /** * Tests that if a socket fails to connect, the allocated connection is * properly released back to the connection manager. */ @@ -160,6 +301,81 @@ } } + private static class BasicService implements HttpRequestHandler { + public void handle(final HttpRequest request, + final HttpResponse response, + final HttpContext context) throws HttpException, IOException { + response.setStatusCode(200); + response.setEntity(new StringEntity("Hello World")); + } + } + + private class BasicRedirectService implements HttpRequestHandler { + private int statuscode = HttpStatus.SC_SEE_OTHER; + private int port; + + public BasicRedirectService(int port) { + this.port = port; + } + + public void handle(final HttpRequest request, + final HttpResponse response, final HttpContext context) + throws HttpException, IOException { + ProtocolVersion ver = request.getRequestLine().getProtocolVersion(); + response.setStatusLine(ver, this.statuscode); + response.addHeader(new BasicHeader("Location", "http://localhost:" + + this.port + "/newlocation/")); + response.addHeader(new BasicHeader("Connection", "close")); + } + } + + private static class ConnMan4 extends ThreadSafeClientConnManager { + private final CountDownLatch connLatch; + private final CountDownLatch awaitLatch; + + public ConnMan4(HttpParams params, SchemeRegistry schreg, + CountDownLatch connLatch, CountDownLatch awaitLatch) { + super(params, schreg); + this.connLatch = connLatch; + this.awaitLatch = awaitLatch; + } + + @Override + public ClientConnectionRequest requestConnection(HttpRoute route) { + // If this is the redirect route, stub the return value + // so-as to pretend the host is waiting on a slot... + if(route.getTargetHost().getHostName().equals("localhost")) { + final Thread currentThread = Thread.currentThread(); + + return new ClientConnectionRequest() { + + public void abortRequest() { + currentThread.interrupt(); + } + + public ManagedClientConnection getConnection( + long timeout, TimeUnit tunit) + throws InterruptedException, + ConnectionPoolTimeoutException { + 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(); + } + }; + } else { + return super.requestConnection(route); + } + } + } + + private static class ConnMan3 extends SingleClientConnManager { private ManagedClientConnection allocatedConnection; private ManagedClientConnection releasedConnection; @@ -318,4 +534,26 @@ } } + private static class CustomGet extends HttpGet { + private final CountDownLatch releaseTriggerLatch; + + public CustomGet(String uri, CountDownLatch releaseTriggerLatch) throws URISyntaxException { + super(uri); + this.releaseTriggerLatch = releaseTriggerLatch; + } + + @Override + public void setReleaseTrigger(ConnectionReleaseTrigger releaseTrigger) throws IOException { + try { + if(!releaseTriggerLatch.await(1, TimeUnit.SECONDS)) + throw new RuntimeException("Waited too long..."); + } catch(InterruptedException ie) { + throw new RuntimeException(ie); + } + + super.setReleaseTrigger(releaseTrigger); + } + + } + } Index: C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java =================================================================== --- C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java (revision 639523) +++ C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java (working copy) @@ -416,7 +416,7 @@ // non-javadoc, see interface HttpClient public final HttpResponse execute(HttpUriRequest request) - throws HttpException, IOException, InterruptedException { + throws HttpException, IOException { return execute(request, null); } @@ -433,7 +433,7 @@ */ public final HttpResponse execute(HttpUriRequest request, HttpContext context) - throws HttpException, IOException, InterruptedException { + throws HttpException, IOException { if (request == null) { throw new IllegalArgumentException @@ -458,7 +458,7 @@ // non-javadoc, see interface HttpClient public final HttpResponse execute(HttpHost target, HttpRequest request) - throws HttpException, IOException, InterruptedException { + throws HttpException, IOException { return execute(target, request, null); } @@ -467,7 +467,7 @@ // non-javadoc, see interface HttpClient public final HttpResponse execute(HttpHost target, HttpRequest request, HttpContext context) - throws HttpException, IOException, InterruptedException { + throws HttpException, IOException { if (request == null) { throw new IllegalArgumentException Index: C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java =================================================================== --- C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java (revision 639523) +++ C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/impl/client/DefaultClientRequestDirector.java (working copy) @@ -267,7 +267,7 @@ // non-javadoc, see interface ClientRequestDirector public HttpResponse execute(HttpHost target, HttpRequest request, HttpContext context) - throws HttpException, IOException, InterruptedException { + throws HttpException, IOException { RoutedRequest roureq = determineRoute(target, request, context); @@ -294,7 +294,14 @@ if (orig instanceof AbortableHttpRequest) { ((AbortableHttpRequest) orig).setConnectionRequest(connRequest); } - managedConn = connRequest.getConnection(timeout, TimeUnit.MILLISECONDS); + + try { + managedConn = connRequest.getConnection(timeout, TimeUnit.MILLISECONDS); + } catch(InterruptedException interrupted) { + IOException iox = new IOException(); + iox.initCause(interrupted); + throw iox; + } } if (orig instanceof AbortableHttpRequest) { @@ -444,9 +451,6 @@ } catch (IOException ex) { abortConnection(); throw ex; - } catch (InterruptedException ex) { - abortConnection(); - throw ex; } catch (RuntimeException ex) { abortConnection(); throw ex; @@ -916,7 +920,6 @@ * @throws IOException in case of an IO problem */ private void abortConnection() throws IOException { - ManagedClientConnection mcc = managedConn; if (mcc != null) { // we got here as the result of an exception Index: C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/HttpClient.java =================================================================== --- C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/HttpClient.java (revision 639523) +++ C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/HttpClient.java (working copy) @@ -103,11 +103,11 @@ * * @throws HttpException in case of a problem * @throws IOException in case of an IO problem - * @throws InterruptedException in case of an interrupt + * or the connection was aborted *
timeout exceptions? */ HttpResponse execute(HttpUriRequest request) - throws HttpException, IOException, InterruptedException + throws HttpException, IOException ; @@ -128,11 +128,11 @@ * * @throws HttpException in case of a problem * @throws IOException in case of an IO problem - * @throws InterruptedException in case of an interrupt + * or the connection was aborted *
timeout exceptions? */ HttpResponse execute(HttpUriRequest request, HttpContext context) - throws HttpException, IOException, InterruptedException + throws HttpException, IOException ; @@ -155,11 +155,11 @@ * * @throws HttpException in case of a problem * @throws IOException in case of an IO problem - * @throws InterruptedException in case of an interrupt + * or the connection was aborted *
timeout exceptions? */ HttpResponse execute(HttpHost target, HttpRequest request) - throws HttpException, IOException, InterruptedException + throws HttpException, IOException ; @@ -183,12 +183,12 @@ * * @throws HttpException in case of a problem * @throws IOException in case of an IO problem - * @throws InterruptedException in case of an interrupt + * or the connection was aborted *
timeout exceptions? */ HttpResponse execute(HttpHost target, HttpRequest request, HttpContext context) - throws HttpException, IOException, InterruptedException + throws HttpException, IOException ; Index: C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java =================================================================== --- C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java (revision 639523) +++ C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java (working copy) @@ -92,11 +92,11 @@ * * @throws HttpException in case of a problem * @throws IOException in case of an IO problem - * @throws InterruptedException in case of an interrupt + * or if the connection was aborted */ HttpResponse execute(HttpHost target, HttpRequest request, HttpContext context) - throws HttpException, IOException, InterruptedException + throws HttpException, IOException ; Index: C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java =================================================================== --- C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java (revision 639523) +++ C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java (working copy) @@ -58,7 +58,7 @@ private final Lock abortLock; - private volatile boolean aborted; + private boolean aborted; private URI uri; private ClientConnectionRequest connRequest; @@ -97,24 +97,29 @@ this.uri = uri; } - public void setConnectionRequest(final ClientConnectionRequest connRequest) { - if (this.aborted) { - return; - } + public void setConnectionRequest(final ClientConnectionRequest connRequest) + throws IOException { this.abortLock.lock(); try { + if (this.aborted) { + throw new IOException("Request already aborted"); + } + + this.releaseTrigger = null; this.connRequest = connRequest; } finally { this.abortLock.unlock(); } } - public void setReleaseTrigger(final ConnectionReleaseTrigger releaseTrigger) { - if (this.aborted) { - return; - } + public void setReleaseTrigger(final ConnectionReleaseTrigger releaseTrigger) + throws IOException { this.abortLock.lock(); try { + if (this.aborted) { + throw new IOException("Request already aborted"); + } + this.connRequest = null; this.releaseTrigger = releaseTrigger; } finally { @@ -123,25 +128,36 @@ } public void abort() { - if (this.aborted) { - return; - } - this.aborted = true; + ClientConnectionRequest localRequest; + ConnectionReleaseTrigger localTrigger; + this.abortLock.lock(); try { - if (this.connRequest != null) { - this.connRequest.abortRequest(); - } - if (this.releaseTrigger != null) { - try { - this.releaseTrigger.abortConnection(); - } catch (IOException ex) { - // ignore - } - } + if (this.aborted) { + return; + } + this.aborted = true; + + localRequest = connRequest; + localTrigger = releaseTrigger; } finally { this.abortLock.unlock(); + } + + // Trigger the callbacks outside of the lock, to prevent + // deadlocks in the scenario where the callbacks have + // their own locks that may be used while calling + // setReleaseTrigger or setConnectionRequest. + if (localRequest != null) { + localRequest.abortRequest(); } + if (localTrigger != null) { + try { + localTrigger.abortConnection(); + } catch (IOException ex) { + // ignore + } + } } } Index: C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/methods/AbortableHttpRequest.java =================================================================== --- C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/methods/AbortableHttpRequest.java (revision 639523) +++ C:/Documents and Settings/sberlin/workspace/httpcomponents-client/module-client/src/main/java/org/apache/http/client/methods/AbortableHttpRequest.java (working copy) @@ -31,12 +31,18 @@ package org.apache.http.client.methods; +import java.io.IOException; + +import org.apache.http.client.HttpClient; +import org.apache.http.conn.ClientConnectionManager; import org.apache.http.conn.ClientConnectionRequest; import org.apache.http.conn.ConnectionReleaseTrigger; +import org.apache.http.conn.ManagedClientConnection; +import org.apache.http.impl.conn.tsccm.ThreadSafeClientConnManager; /** * Interface representing an HTTP request that can be aborted by shutting - * donw the underying HTTP connection. + * down the underlying HTTP connection. * * @author Oleg Kalnichevski * @@ -47,10 +53,39 @@ */ public interface AbortableHttpRequest { - void setConnectionRequest(ClientConnectionRequest connRequest); + /** + * Sets the {@link ClientConnectionRequest} callback that can be + * used to abort a long-lived request for a connection. + * If the request is already aborted, throws an {@link IOException}. + * + * @see ClientConnectionManager + * @see ThreadSafeClientConnManager + */ + void setConnectionRequest(ClientConnectionRequest connRequest) throws IOException; - void setReleaseTrigger(ConnectionReleaseTrigger releaseTrigger); - + /** + * Sets the {@link ConnectionReleaseTrigger} callback that can + * be used to abort an active connection. + * Typically, this will be the {@link ManagedClientConnection} itself. + * If the request is already aborted, throws an {@link IOException}. + */ + void setReleaseTrigger(ConnectionReleaseTrigger releaseTrigger) throws IOException; + + /** + * Aborts this http request. Any active execution of this method should + * return immediately. If the request has not started, it will abort after + * the next execution. Aborting this request will cause all subsequent + * executions with this request to fail. + * + * @see HttpClient#execute(HttpUriRequest) + * @see HttpClient#execute(org.apache.http.HttpHost, + * org.apache.http.HttpRequest) + * @see HttpClient#execute(HttpUriRequest, + * org.apache.http.protocol.HttpContext) + * @see HttpClient#execute(org.apache.http.HttpHost, + * org.apache.http.HttpRequest, org.apache.http.protocol.HttpContext) + */ void abort(); } +