From 97bb41b6873b6842ed4bf3eddd3a628d681500d0 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Fri, 21 Mar 2008 11:28:15 +0000 Subject: [PATCH] GTTPCLIENT-760: Improved handling of request abort in fringe situations such as request abort immediately after a connection has been allocated but before the release trigger has been set Contributed by Sam Berlin Reviewed by Oleg Kalnichevski git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@639600 13f79535-47bb-0310-9956-ffa450edef68 --- .../http/client/ClientRequestDirector.java | 4 +- .../org/apache/http/client/HttpClient.java | 16 +- .../client/methods/AbortableHttpRequest.java | 43 +++- .../http/client/methods/HttpRequestBase.java | 62 +++-- .../http/impl/client/AbstractHttpClient.java | 8 +- .../client/DefaultClientRequestDirector.java | 16 +- .../TestDefaultClientRequestDirector.java | 242 +++++++++++++++++- .../apache/http/impl/conn/ExecReqThread.java | 2 +- 8 files changed, 343 insertions(+), 50 deletions(-) diff --git a/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java b/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java index bfb45fbbc..3ec5f0ae2 100644 --- a/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java +++ b/module-client/src/main/java/org/apache/http/client/ClientRequestDirector.java @@ -92,11 +92,11 @@ public interface ClientRequestDirector { * * @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 ; diff --git a/module-client/src/main/java/org/apache/http/client/HttpClient.java b/module-client/src/main/java/org/apache/http/client/HttpClient.java index d5e635bef..9cd029380 100644 --- a/module-client/src/main/java/org/apache/http/client/HttpClient.java +++ b/module-client/src/main/java/org/apache/http/client/HttpClient.java @@ -103,11 +103,11 @@ ClientConnectionManager getConnectionManager() * * @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 @@ HttpResponse execute(HttpUriRequest request) * * @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 @@ HttpResponse execute(HttpUriRequest request, HttpContext context) * * @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 @@ HttpResponse execute(HttpHost target, HttpRequest request) * * @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 ; diff --git a/module-client/src/main/java/org/apache/http/client/methods/AbortableHttpRequest.java b/module-client/src/main/java/org/apache/http/client/methods/AbortableHttpRequest.java index 943fad401..bb3188cf0 100644 --- a/module-client/src/main/java/org/apache/http/client/methods/AbortableHttpRequest.java +++ b/module-client/src/main/java/org/apache/http/client/methods/AbortableHttpRequest.java @@ -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); - - void setReleaseTrigger(ConnectionReleaseTrigger releaseTrigger); + /** + * 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; + /** + * 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(); } + diff --git a/module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java b/module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java index 386d29e81..b4e360330 100644 --- a/module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java +++ b/module-client/src/main/java/org/apache/http/client/methods/HttpRequestBase.java @@ -58,7 +58,7 @@ abstract class HttpRequestBase extends AbstractHttpMessage private final Lock abortLock; - private volatile boolean aborted; + private boolean aborted; private URI uri; private ClientConnectionRequest connRequest; @@ -97,24 +97,29 @@ public void setURI(final URI uri) { 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,24 +128,35 @@ public void setReleaseTrigger(final ConnectionReleaseTrigger releaseTrigger) { } 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 + } } } diff --git a/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java b/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java index 6ef78d1e9..93a4a422d 100644 --- a/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java +++ b/module-client/src/main/java/org/apache/http/impl/client/AbstractHttpClient.java @@ -416,7 +416,7 @@ public void removeRequestInterceptorByClass(Class 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); } @@ -160,6 +301,81 @@ public void handle( } } + 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 @@ public void shutdown() { } } + 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); + } + + } + } diff --git a/module-client/src/test/java/org/apache/http/impl/conn/ExecReqThread.java b/module-client/src/test/java/org/apache/http/impl/conn/ExecReqThread.java index a31c46f95..091b98318 100644 --- a/module-client/src/test/java/org/apache/http/impl/conn/ExecReqThread.java +++ b/module-client/src/test/java/org/apache/http/impl/conn/ExecReqThread.java @@ -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;