From c4c6cfc4138d89fc2dba031f145c7e885619272a Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Wed, 2 Jun 2010 18:52:05 +0000 Subject: [PATCH] HTTPCLIENT-948: works around the race condition described in HTTPCLIENT-948 git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@950717 13f79535-47bb-0310-9956-ffa450edef68 --- .../http/impl/conn/AbstractPoolEntry.java | 2 + .../http/impl/conn/tsccm/BasicPoolEntry.java | 2 + .../http/impl/conn/tsccm/ConnPoolByRoute.java | 6 +- .../tsccm/ThreadSafeClientConnManager.java | 27 ++- .../impl/conn/TestIdleConnectionEviction.java | 181 ++++++++++++++++++ 5 files changed, 206 insertions(+), 12 deletions(-) create mode 100644 httpclient/src/test/java/org/apache/http/impl/conn/TestIdleConnectionEviction.java diff --git a/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPoolEntry.java b/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPoolEntry.java index 9da52f0ea..4e8ad0ded 100644 --- a/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPoolEntry.java +++ b/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPoolEntry.java @@ -31,6 +31,7 @@ import java.io.IOException; import org.apache.http.HttpHost; import org.apache.http.params.HttpParams; import org.apache.http.protocol.HttpContext; +import org.apache.http.annotation.NotThreadSafe; import org.apache.http.conn.routing.HttpRoute; import org.apache.http.conn.routing.RouteTracker; import org.apache.http.conn.ClientConnectionOperator; @@ -52,6 +53,7 @@ import org.apache.http.conn.OperatedClientConnection; * * @since 4.0 */ +@NotThreadSafe public abstract class AbstractPoolEntry { /** The connection operator. */ diff --git a/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java b/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java index a7565ae58..719f16df8 100644 --- a/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java +++ b/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/BasicPoolEntry.java @@ -28,6 +28,7 @@ package org.apache.http.impl.conn.tsccm; import java.lang.ref.ReferenceQueue; +import org.apache.http.annotation.NotThreadSafe; import org.apache.http.conn.OperatedClientConnection; import org.apache.http.conn.ClientConnectionOperator; import org.apache.http.conn.routing.HttpRoute; @@ -38,6 +39,7 @@ import org.apache.http.impl.conn.AbstractPoolEntry; * * @since 4.0 */ +@NotThreadSafe public class BasicPoolEntry extends AbstractPoolEntry { /** diff --git a/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java b/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java index adbb25bb2..2565b9e24 100644 --- a/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java +++ b/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ConnPoolByRoute.java @@ -302,9 +302,9 @@ public class ConnPoolByRoute extends AbstractConnPool { } if (log.isDebugEnabled()) { - log.debug("Total connections kept alive: " + freeConnections.size()); - log.debug("Total issued connections: " + leasedConnections.size()); - log.debug("Total allocated connection: " + numConnections + " out of " + maxTotalConnections); + log.debug("[" + route + "] kept alive: " + freeConnections.size() + + ", issued: " + leasedConnections.size() + + ", allocated: " + numConnections + " out of " + maxTotalConnections); } // the cases to check for: diff --git a/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java b/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java index 1fa99f954..ba0dbe42d 100644 --- a/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java +++ b/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java @@ -196,8 +196,7 @@ public class ThreadSafeClientConnManager implements ClientConnectionManager { } if (log.isDebugEnabled()) { - log.debug("ThreadSafeClientConnManager.getConnection: " - + route + ", timeout = " + timeout); + log.debug("Get connection: " + route + ", timeout = " + timeout); } BasicPoolEntry entry = poolRequest.getPoolEntry(timeout, tunit); @@ -285,11 +284,11 @@ public class ThreadSafeClientConnManager implements ClientConnectionManager { * @return the total number of pooled connections */ public int getConnectionsInPool() { - pool.poolLock.lock(); + connectionPool.poolLock.lock(); try { - return pool.numConnections; + return connectionPool.numConnections; } finally { - pool.poolLock.unlock(); + connectionPool.poolLock.unlock(); } } @@ -297,14 +296,24 @@ public class ThreadSafeClientConnManager implements ClientConnectionManager { if (log.isDebugEnabled()) { log.debug("Closing connections idle for " + idleTimeout + " " + tunit); } - pool.closeIdleConnections(idleTimeout, tunit); - pool.deleteClosedConnections(); + connectionPool.poolLock.lock(); + try { + connectionPool.closeIdleConnections(idleTimeout, tunit); + connectionPool.deleteClosedConnections(); + } finally { + connectionPool.poolLock.unlock(); + } } public void closeExpiredConnections() { log.debug("Closing expired connections"); - pool.closeExpiredConnections(); - pool.deleteClosedConnections(); + connectionPool.poolLock.lock(); + try { + connectionPool.closeExpiredConnections(); + connectionPool.deleteClosedConnections(); + } finally { + connectionPool.poolLock.unlock(); + } } /** diff --git a/httpclient/src/test/java/org/apache/http/impl/conn/TestIdleConnectionEviction.java b/httpclient/src/test/java/org/apache/http/impl/conn/TestIdleConnectionEviction.java new file mode 100644 index 000000000..d12b9a1c1 --- /dev/null +++ b/httpclient/src/test/java/org/apache/http/impl/conn/TestIdleConnectionEviction.java @@ -0,0 +1,181 @@ +/* + * ==================================================================== + * 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.conn; + +import java.net.InetSocketAddress; +import java.util.concurrent.TimeUnit; + +import org.apache.http.HttpEntity; +import org.apache.http.HttpHost; +import org.apache.http.HttpResponse; +import org.apache.http.client.ClientProtocolException; +import org.apache.http.client.HttpClient; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.conn.ClientConnectionManager; +import org.apache.http.conn.scheme.PlainSocketFactory; +import org.apache.http.conn.scheme.Scheme; +import org.apache.http.conn.scheme.SchemeRegistry; +import org.apache.http.impl.client.DefaultHttpClient; +import org.apache.http.impl.conn.tsccm.ThreadSafeClientConnManager; +import org.apache.http.localserver.LocalTestServer; +import org.apache.http.localserver.ServerTestBase; +import org.apache.http.params.BasicHttpParams; +import org.apache.http.params.HttpConnectionParams; +import org.apache.http.params.HttpParams; +import org.junit.Before; +import org.junit.Test; + +public class TestIdleConnectionEviction extends ServerTestBase { + + @Before + public void setUp() throws Exception { + this.localServer = new LocalTestServer(null, null); + this.localServer.registerDefaultHandlers(); + this.localServer.start(); + } + + @Test + public void testIdleConnectionEviction() throws Exception { + HttpParams params = new BasicHttpParams(); + HttpConnectionParams.setStaleCheckingEnabled(params, false); + + SchemeRegistry schemeRegistry = new SchemeRegistry(); + schemeRegistry.register(new Scheme("http", 80, PlainSocketFactory.getSocketFactory())); + + ThreadSafeClientConnManager cm = new ThreadSafeClientConnManager(schemeRegistry); + cm.setDefaultMaxPerRoute(10); + cm.setMaxTotalConnections(50); + + DefaultHttpClient httpclient = new DefaultHttpClient(cm, params); + + IdleConnectionMonitor idleConnectionMonitor = new IdleConnectionMonitor(cm); + idleConnectionMonitor.start(); + + InetSocketAddress address = this.localServer.getServiceAddress(); + HttpHost target = new HttpHost(address.getHostName(), address.getPort()); + HttpGet httpget = new HttpGet("/random/1024"); + WorkerThread[] workers = new WorkerThread[5]; + for (int i = 0; i < workers.length; i++) { + workers[i] = new WorkerThread(httpclient, target, httpget, 2000); + } + for (int i = 0; i < workers.length; i++) { + workers[i].start(); + } + for (int i = 0; i < workers.length; i++) { + workers[i].join(); + Exception ex = workers[i].getException(); + if (ex != null) { + throw ex; + } + } + idleConnectionMonitor.shutdown(); + } + + static class WorkerThread extends Thread { + + private final HttpClient httpclient; + private final HttpHost target; + private final HttpUriRequest request; + private final int count; + + private volatile Exception ex; + + public WorkerThread( + final HttpClient httpclient, + final HttpHost target, + final HttpUriRequest request, + int count) { + super(); + this.httpclient = httpclient; + this.target = target; + this.request = request; + this.count = count; + } + + @Override + public void run() { + try { + for (int i = 0; i < this.count; i++) { + HttpResponse response = this.httpclient.execute(this.target, this.request); + int status = response.getStatusLine().getStatusCode(); + if (status != 200) { + this.request.abort(); + throw new ClientProtocolException("Unexpected status code: " + status); + } + HttpEntity entity = response.getEntity(); + if (entity != null) { + entity.consumeContent(); + } + } + } catch (Exception ex) { + this.ex = ex; + } + } + + public Exception getException() { + return ex; + } + + } + + public static class IdleConnectionMonitor extends Thread { + + private final ClientConnectionManager cm; + private volatile boolean shutdown; + + public IdleConnectionMonitor(final ClientConnectionManager cm) { + super(); + this.cm = cm; + setDaemon(true); + } + + @Override + public void run() { + try { + while (!this.shutdown) { + synchronized (this) { + wait(250); + this.cm.closeIdleConnections(1, TimeUnit.MILLISECONDS); + } + } + } catch (InterruptedException ex) { + // terminate + } + } + + public void shutdown() { + this.shutdown = true; + synchronized (this) { + notifyAll(); + } + } + + } + +}