diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index 31f17ec47..74e1c05f9 100644 --- a/RELEASE_NOTES.txt +++ b/RELEASE_NOTES.txt @@ -1,6 +1,10 @@ Changes since 4.0 Alpha 2 ------------------- +* [HTTPCLIENT-677] Thread safe client connection manager no longer uses + Thread.interrupt() when a connection becomes available. + Contributed by Roland Weber + * [HTTPCLIENT-716] Allow application-defined routes. Contributed by Roland Weber diff --git a/module-client/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java b/module-client/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java index f6df4ad6c..325037353 100644 --- a/module-client/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java +++ b/module-client/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java @@ -71,10 +71,6 @@ public class ConnPoolByRoute extends AbstractConnPool { private final Log LOG = LogFactory.getLog(ConnPoolByRoute.class); - /** Temporary hack: @@@ a global condition that goes with the lock. */ - protected final Condition poolCondition; - - /** The list of free connections */ private Queue freeConnections; @@ -98,8 +94,6 @@ public class ConnPoolByRoute extends AbstractConnPool { public ConnPoolByRoute(ClientConnectionManager mgr) { super(mgr); - poolCondition = poolLock.newCondition(); //@@@ temporary hack - //@@@ use factory method, at least for waitingThreads freeConnections = new LinkedList(); waitingThreads = new LinkedList(); @@ -222,6 +216,7 @@ public class ConnPoolByRoute extends AbstractConnPool { // TODO: keep track of which routes have waiting threads, // so they avoid being sacrificed before necessary + boolean success = false; try { if (useTimeout && timeToWait <= 0) { throw new ConnectionPoolTimeoutException @@ -233,12 +228,9 @@ public class ConnPoolByRoute extends AbstractConnPool { } if (waitingThread == null) { + //@@@ use factory method? waitingThread = new WaitingThread (poolLock.newCondition(), rospl); - //@@@waitingThread.pool = rospl; - //@@@waitingThread.thread = Thread.currentThread(); - } else { - waitingThread.interruptedByConnectionPool = false;//@@@ } if (useTimeout) { @@ -247,21 +239,17 @@ public class ConnPoolByRoute extends AbstractConnPool { rospl.queueThread(waitingThread); waitingThreads.add(waitingThread); - //@@@ poolCondition.await(timeToWait, TimeUnit.MILLISECONDS); - waitingThread.await(timeToWait); //@@@, TimeUnit.MILLISECONDS); + success = waitingThread.await(timeToWait); //@@@, TimeUnit.MILLISECONDS); + //@@@ The 'success' flag is somewhat different from the + //@@@ previous technique using interrupts. If the CM is + //@@@ shutting down, we now get an InterruptedException + //@@@ and have no check for that special case. What we + //@@@ want to do is to let the exception fly through. + //@@@ Actually, we may want to have a special exception + //@@@ for the shutdown case, but that is goldplating. - } catch (InterruptedException e) { - if (!waitingThread.interruptedByConnectionPool) { - LOG.debug("Interrupted while waiting for connection.", e); - throw e; - } - // Else, do nothing, we were interrupted by the - // connection pool and should now have a connection - // waiting for us. Continue in the loop and get it. - // Or else we are shutting down, which is also - // detected in the loop. } finally { - if (!waitingThread.interruptedByConnectionPool) { + if (!success) { // Either we timed out, experienced a // "spurious wakeup", or were interrupted by an // external thread. Regardless we need to @@ -269,6 +257,9 @@ public class ConnPoolByRoute extends AbstractConnPool { rospl.removeThread(waitingThread); waitingThreads.remove(waitingThread); } + // In case of 'success', we were woken up by the + // connection pool and should now have a connection + // waiting for us. Continue in the loop and get it. if (useTimeout) { endWait = System.currentTimeMillis(); @@ -531,8 +522,7 @@ public class ConnPoolByRoute extends AbstractConnPool { } if (waitingThread != null) { - waitingThread.interruptedByConnectionPool = true; - waitingThread.getThread().interrupt(); //@@@ HTTPCLIENT-677 + waitingThread.wakeup(); } } finally { @@ -586,8 +576,7 @@ public class ConnPoolByRoute extends AbstractConnPool { while (iwth.hasNext()) { WaitingThread waiter = iwth.next(); iwth.remove(); - waiter.interruptedByConnectionPool = true; - waiter.getThread().interrupt(); //@@@ HTTPCLIENT-677 + waiter.getThread().interrupt(); } routeToPool.clear(); diff --git a/module-client/src/main/java/org/apache/http/impl/conn/tsccm/WaitingThread.java b/module-client/src/main/java/org/apache/http/impl/conn/tsccm/WaitingThread.java index 3500c82e0..409b38440 100644 --- a/module-client/src/main/java/org/apache/http/impl/conn/tsccm/WaitingThread.java +++ b/module-client/src/main/java/org/apache/http/impl/conn/tsccm/WaitingThread.java @@ -60,18 +60,6 @@ public class WaitingThread { private Thread waiter; - /** - * Indicates the source of an interruption. - * Set to true inside - * {@link #notifyWaitingThread(RouteSpecificPool)} - * and {@link #shutdown shutdown()} - * before the thread is interrupted. - * If not set, the thread was interrupted from the outside. - */ - //@@@ to be removed in HTTPCLIENT-677 - /*default@@@*/ boolean interruptedByConnectionPool; - - /** * Creates a new entry for a waiting thread. * @@ -99,9 +87,17 @@ public class WaitingThread { * * @param timeout the timeout in milliseconds, or 0 for no timeout * + * @return true if the condition was satisfied, + * false in case of a timeout. + * Typically, a call to {@link #wakeup} is used to indicate + * that the condition was satisfied. Since the condition can + * be accessed from outside, this cannot be guaranteed though. + * + * @throws InterruptedException if the waiting thread was interrupted + * * @see #wakeup */ - public void await(long timeout) + public boolean await(long timeout) throws InterruptedException { //@@@ check timeout for negative, or assume overflow? @@ -118,11 +114,14 @@ public class WaitingThread { this.waiter = Thread.currentThread(); + boolean success = false; try { - this.cond.await(timeout, TimeUnit.MILLISECONDS); + success = this.cond.await(timeout, TimeUnit.MILLISECONDS); } finally { this.waiter = null; } + return success; + } // await