hc-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ol...@apache.org
Subject svn commit: r828205 - in /httpcomponents/httpclient/branches/4.0.x: ./ httpclient-osgi/ httpclient/ httpclient/src/main/java/org/apache/http/impl/conn/ httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ httpclient/src/test/java/org/apache/http/i...
Date Wed, 21 Oct 2009 20:46:31 GMT
Author: olegk
Date: Wed Oct 21 20:46:31 2009
New Revision: 828205

URL: http://svn.apache.org/viewvc?rev=828205&view=rev
Log:
Backported fix for HTTPCLIENT-881

Modified:
    httpcomponents/httpclient/branches/4.0.x/   (props changed)
    httpcomponents/httpclient/branches/4.0.x/RELEASE_NOTES.txt
    httpcomponents/httpclient/branches/4.0.x/httpclient/   (props changed)
    httpcomponents/httpclient/branches/4.0.x/httpclient-osgi/   (props changed)
    httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java
    httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPooledConnAdapter.java
    httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java
    httpcomponents/httpclient/branches/4.0.x/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java
    httpcomponents/httpclient/branches/4.0.x/httpmime/   (props changed)
    httpcomponents/httpclient/branches/4.0.x/src/docbkx/fundamentals.xml
    httpcomponents/httpclient/branches/4.0.x/src/docbkx/resources/   (props changed)

Propchange: httpcomponents/httpclient/branches/4.0.x/
------------------------------------------------------------------------------
    svn:mergeinfo = /httpcomponents/httpclient/trunk:825864-828185

Modified: httpcomponents/httpclient/branches/4.0.x/RELEASE_NOTES.txt
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.0.x/RELEASE_NOTES.txt?rev=828205&r1=828204&r2=828205&view=diff
==============================================================================
--- httpcomponents/httpclient/branches/4.0.x/RELEASE_NOTES.txt (original)
+++ httpcomponents/httpclient/branches/4.0.x/RELEASE_NOTES.txt Wed Oct 21 20:46:31 2009
@@ -1,3 +1,17 @@
+
+Changes since 4.0
+-------------------
+
+* [HTTPCLIENT-881] Fixed race condition in AbstractClientConnAdapter that makes it
+  possible for an aborted connection to be returned to the pool.
+  Contributed by Tim Boemker <tboemker at elynx.com> and 
+  Oleg Kalnichevski <olegk at apache.org>
+
+* [HTTPCLIENT-866] Removed dependency on jcip-annotations.jar.
+  Contributed by Oleg Kalnichevski <olegk at apache.org> 
+  and Sebastian Bazley <sebb at apache.org>
+
+
 Release 4.0
 -------------------
 

Propchange: httpcomponents/httpclient/branches/4.0.x/httpclient/
            ('svn:mergeinfo' removed)

Propchange: httpcomponents/httpclient/branches/4.0.x/httpclient-osgi/
            ('svn:mergeinfo' removed)

Modified: httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java?rev=828205&r1=828204&r2=828205&view=diff
==============================================================================
--- httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java
(original)
+++ httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractClientConnAdapter.java
Wed Oct 21 20:46:31 2009
@@ -67,9 +67,6 @@
  */
 public abstract class AbstractClientConnAdapter implements ManagedClientConnection {
 
-    /** Thread that requested this connection. */
-    private final Thread executionThread; 
-    
     /**
      * The connection manager, if any.
      * This attribute MUST NOT be final, so the adapter can be detached
@@ -83,8 +80,8 @@
     /** The reusability marker. */
     private volatile boolean markedReusable;
 
-    /** True if the connection has been aborted. */
-    private volatile boolean aborted;
+    /** True if the connection has been shut down or released. */
+    private volatile boolean released;
     
     /** The duration this is valid for while idle (in ms). */
     private volatile long duration;
@@ -100,20 +97,18 @@
     protected AbstractClientConnAdapter(ClientConnectionManager mgr,
                                         OperatedClientConnection conn) {
         super();
-        executionThread = Thread.currentThread();
         connManager = mgr;
         wrappedConnection = conn;
         markedReusable = false;
-        aborted = false;
+        released = false;
         duration = Long.MAX_VALUE;
-    } // <constructor>
-
+    }
 
     /**
      * Detaches this adapter from the wrapped connection.
      * This adapter becomes useless.
      */
-    protected void detach() {
+    protected synchronized void detach() {
         wrappedConnection = null;
         connManager = null; // base class attribute
         duration = Long.MAX_VALUE;
@@ -127,14 +122,9 @@
         return connManager;
     }
     
-    /**
-     * Asserts that the connection has not been aborted.
-     *
-     * @throws InterruptedIOException   if the connection has been aborted
-     */
     protected final void assertNotAborted() throws InterruptedIOException {
-        if (aborted) {
-            throw new InterruptedIOException("Connection has been shut down.");
+        if (released) {
+            throw new InterruptedIOException("Connection has been shut down");
         }
     }
 
@@ -145,9 +135,9 @@
      *                                  or connection has been aborted
      */
     protected final void assertValid(
-            final OperatedClientConnection wrappedConn) {
+            final OperatedClientConnection wrappedConn) throws IllegalStateException {
         if (wrappedConn == null) {
-            throw new IllegalStateException("No wrapped connection.");
+            throw new IllegalStateException("No wrapped connection");
         }
     }
 
@@ -160,7 +150,7 @@
     }
 
     public boolean isStale() {
-        if (aborted)
+        if (released)
             return true;
         OperatedClientConnection conn = getWrappedConnection();
         if (conn == null)
@@ -187,66 +177,52 @@
         return conn.getMetrics();
     }
 
-    public void flush()
-        throws IOException {
-
+    public void flush() throws IOException {
         assertNotAborted();
         OperatedClientConnection conn = getWrappedConnection();
         assertValid(conn);
-
         conn.flush();
     }
 
-    public boolean isResponseAvailable(int timeout)
-        throws IOException {
-
+    public boolean isResponseAvailable(int timeout) throws IOException {
         assertNotAborted();
         OperatedClientConnection conn = getWrappedConnection();
         assertValid(conn);
-
         return conn.isResponseAvailable(timeout);
     }
 
     public void receiveResponseEntity(HttpResponse response)
         throws HttpException, IOException {
-
         assertNotAborted();
         OperatedClientConnection conn = getWrappedConnection();
         assertValid(conn);
-
         unmarkReusable();
         conn.receiveResponseEntity(response);
     }
 
     public HttpResponse receiveResponseHeader()
         throws HttpException, IOException {
-
         assertNotAborted();
         OperatedClientConnection conn = getWrappedConnection();
         assertValid(conn);
-
         unmarkReusable();
         return conn.receiveResponseHeader();
     }
 
     public void sendRequestEntity(HttpEntityEnclosingRequest request)
         throws HttpException, IOException {
-
         assertNotAborted();
         OperatedClientConnection conn = getWrappedConnection();
         assertValid(conn);
-
         unmarkReusable();
         conn.sendRequestEntity(request);
     }
 
     public void sendRequestHeader(HttpRequest request)
         throws HttpException, IOException {
-
         assertNotAborted();
         OperatedClientConnection conn = getWrappedConnection();
         assertValid(conn);
-        
         unmarkReusable();
         conn.sendRequestHeader(request);
     }
@@ -315,37 +291,28 @@
         }
     }
 
-    public void releaseConnection() {
+    public synchronized void releaseConnection() {
+        if (released) {
+            return;
+        }
+        released = true;
         if (connManager != null) {
             connManager.releaseConnection(this, duration, TimeUnit.MILLISECONDS);
         }
     }
 
-    public void abortConnection() {
-        if (aborted) {
+    public synchronized void abortConnection() {
+        if (released) {
             return;
         }
-        aborted = true;
+        released = true;
         unmarkReusable();
         try {
             shutdown();
         } catch (IOException ignore) {
         }
-        // Usually #abortConnection() is expected to be called from 
-        // a helper thread in order to unblock the main execution thread 
-        // blocked in an I/O operation. It may be unsafe to call 
-        // #releaseConnection() from the helper thread, so we have to rely
-        // on an IOException thrown by the closed socket on the main thread 
-        // to trigger the release of the connection back to the 
-        // connection manager.
-        // 
-        // However, if this method is called from the main execution thread 
-        // it should be safe to release the connection immediately. Besides, 
-        // this also helps ensure the connection gets released back to the 
-        // manager if #abortConnection() is called from the main execution 
-        // thread while there is no blocking I/O operation.
-        if (executionThread.equals(Thread.currentThread())) {
-            releaseConnection();
+        if (connManager != null) {
+            connManager.releaseConnection(this, duration, TimeUnit.MILLISECONDS);
         }
     }
 

Modified: httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPooledConnAdapter.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPooledConnAdapter.java?rev=828205&r1=828204&r2=828205&view=diff
==============================================================================
--- httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPooledConnAdapter.java
(original)
+++ httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/AbstractPooledConnAdapter.java
Wed Oct 21 20:46:31 2009
@@ -69,7 +69,9 @@
      *
      * @throws IllegalStateException
      *      if it is {@link #detach detach}ed
+     * @deprecated
      */
+    @Deprecated
     protected final void assertAttached() {
         if (poolEntry == null) {
             throw new IllegalStateException("Adapter is detached.");
@@ -81,50 +83,65 @@
      * This adapter becomes useless.
      */
     @Override
-    protected void detach() {
+    protected synchronized void detach() {
         super.detach();
         poolEntry = null;
     }
 
     public HttpRoute getRoute() {
-
-        assertAttached();
-        return (poolEntry.tracker == null) ?
-            null : poolEntry.tracker.toRoute();
+        AbstractPoolEntry entry = poolEntry;
+        if (entry == null) {
+            throw new IllegalStateException("Adapter is detached.");
+        }
+        return (entry.tracker == null) ?
+            null : entry.tracker.toRoute();
     }
 
     public void open(HttpRoute route,
                      HttpContext context, HttpParams params)
         throws IOException {
-
-        assertAttached();
-        poolEntry.open(route, context, params);
+        assertNotAborted();
+        AbstractPoolEntry entry = poolEntry;
+        if (entry == null) {
+            throw new IllegalStateException("Adapter is detached.");
+        }
+        entry.open(route, context, params);
     }
 
     public void tunnelTarget(boolean secure, HttpParams params)
         throws IOException {
-
-        assertAttached();
-        poolEntry.tunnelTarget(secure, params);
+        assertNotAborted();
+        AbstractPoolEntry entry = poolEntry;
+        if (entry == null) {
+            throw new IllegalStateException("Adapter is detached.");
+        }
+        entry.tunnelTarget(secure, params);
     }
 
     public void tunnelProxy(HttpHost next, boolean secure, HttpParams params)
         throws IOException {
-
-        assertAttached();
-        poolEntry.tunnelProxy(next, secure, params);
+        assertNotAborted();
+        AbstractPoolEntry entry = poolEntry;
+        if (entry == null) {
+            throw new IllegalStateException("Adapter is detached.");
+        }
+        entry.tunnelProxy(next, secure, params);
     }
 
     public void layerProtocol(HttpContext context, HttpParams params)
         throws IOException {
-
-        assertAttached();
-        poolEntry.layerProtocol(context, params);
+        assertNotAborted();
+        AbstractPoolEntry entry = poolEntry;
+        if (entry == null) {
+            throw new IllegalStateException("Adapter is detached.");
+        }
+        entry.layerProtocol(context, params);
     }
 
     public void close() throws IOException {
-        if (poolEntry != null)
-            poolEntry.shutdownEntry();
+        AbstractPoolEntry entry = poolEntry;
+        if (entry != null)
+            entry.shutdownEntry();
 
         OperatedClientConnection conn = getWrappedConnection();
         if (conn != null) {
@@ -133,8 +150,9 @@
     }
 
     public void shutdown() throws IOException {
-        if (poolEntry != null)
-            poolEntry.shutdownEntry();
+        AbstractPoolEntry entry = poolEntry;
+        if (entry != null)
+            entry.shutdownEntry();
 
         OperatedClientConnection conn = getWrappedConnection();
         if (conn != null) {
@@ -143,13 +161,19 @@
     }
 
     public Object getState() {
-        assertAttached();
-        return poolEntry.getState();
+        AbstractPoolEntry entry = poolEntry;
+        if (entry == null) {
+            throw new IllegalStateException("Adapter is detached.");
+        }
+        return entry.getState();
     }
 
     public void setState(final Object state) {
-        assertAttached();
-        poolEntry.setState(state);
+        AbstractPoolEntry entry = poolEntry;
+        if (entry == null) {
+            throw new IllegalStateException("Adapter is detached.");
+        }
+        entry.setState(state);
     }
 
 }

Modified: httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java?rev=828205&r1=828204&r2=828205&view=diff
==============================================================================
--- httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java
(original)
+++ httpcomponents/httpclient/branches/4.0.x/httpclient/src/main/java/org/apache/http/impl/conn/tsccm/ThreadSafeClientConnManager.java
Wed Oct 21 20:46:31 2009
@@ -189,37 +189,38 @@
             throw new IllegalArgumentException
                 ("Connection not obtained from this manager.");
         }
-
-        try {
-            // make sure that the response has been read completely
-            if (hca.isOpen() && !hca.isMarkedReusable()) {
-                // In MTHCM, there would be a call to
-                // SimpleHttpConnectionManager.finishLastResponse(conn);
-                // Consuming the response is handled outside in 4.0.
-
-                // make sure this connection will not be re-used
-                // Shut down rather than close, we might have gotten here
-                // because of a shutdown trigger.
-                // Shutdown of the adapter also clears the tracked route.
-                hca.shutdown();
-            }
-        } catch (IOException iox) {
-            //@@@ log as warning? let pass?
-            if (log.isDebugEnabled())
-                log.debug("Exception shutting down released connection.",
-                          iox);
-        } finally {
+        synchronized (hca) {
             BasicPoolEntry entry = (BasicPoolEntry) hca.getPoolEntry();
-            boolean reusable = hca.isMarkedReusable();
-            if (log.isDebugEnabled()) {
-                if (reusable) {
-                    log.debug("Released connection is reusable.");
-                } else {
-                    log.debug("Released connection is not reusable.");
-                }
+            if (entry == null) {
+                return;
             }
-            hca.detach();
-            if (entry != null) {
+            try {
+                // make sure that the response has been read completely
+                if (hca.isOpen() && !hca.isMarkedReusable()) {
+                    // In MTHCM, there would be a call to
+                    // SimpleHttpConnectionManager.finishLastResponse(conn);
+                    // Consuming the response is handled outside in 4.0.
+
+                    // make sure this connection will not be re-used
+                    // Shut down rather than close, we might have gotten here
+                    // because of a shutdown trigger.
+                    // Shutdown of the adapter also clears the tracked route.
+                    hca.shutdown();
+                }
+            } catch (IOException iox) {
+                if (log.isDebugEnabled())
+                    log.debug("Exception shutting down released connection.",
+                              iox);
+            } finally {
+                boolean reusable = hca.isMarkedReusable();
+                if (log.isDebugEnabled()) {
+                    if (reusable) {
+                        log.debug("Released connection is reusable.");
+                    } else {
+                        log.debug("Released connection is not reusable.");
+                    }
+                }
+                hca.detach();
                 connectionPool.freeEntry(entry, reusable, validDuration, timeUnit);
             }
         }

Modified: httpcomponents/httpclient/branches/4.0.x/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.0.x/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java?rev=828205&r1=828204&r2=828205&view=diff
==============================================================================
--- httpcomponents/httpclient/branches/4.0.x/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java
(original)
+++ httpcomponents/httpclient/branches/4.0.x/httpclient/src/test/java/org/apache/http/impl/conn/TestTSCCMWithServer.java
Wed Oct 21 20:46:31 2009
@@ -76,20 +76,6 @@
  */
 public class TestTSCCMWithServer extends ServerTestBase {
 
-    // Server-based tests not ported from 3.x TestHttpConnectionManager
-    //
-    // testWriteRequestReleaseConnection
-    //      This tests auto-release in case of an I/O error while writing.
-    //      It's a test of the execution framework, not of the manager.
-    // testConnectMethodFailureRelease
-    //      This tests method.fakeResponse() and auto-release. It's a
-    //      test of a 3.x specific hack and the execution framework.
-    // testResponseAutoRelease
-    //      Auto-release is not part of the connection manager anymore.
-    // testMaxConnectionsPerServer
-    //      Connection limits are already tested without a server.
-
-
     public TestTSCCMWithServer(String testName) {
         super(testName);
     }
@@ -582,18 +568,6 @@
         assertFalse(conn.isOpen());
         assertEquals(0, localServer.getAcceptedConnectionCount());
         
-        // check that there are no connections available
-        try {
-            // this should fail quickly, connection has not been released
-            getConnection(mgr, route, 100L, TimeUnit.MILLISECONDS);
-            fail("ConnectionPoolTimeoutException should have been thrown");
-        } catch (ConnectionPoolTimeoutException e) {
-            // expected
-        }
-
-        // return it back to the manager
-        ((AbstractClientConnAdapter) conn).releaseConnection();
-        
         // the connection is expected to be released back to the manager
         ManagedClientConnection conn2 = getConnection(mgr, route, 5L, TimeUnit.SECONDS);
         assertFalse("connection should have been closed", conn2.isOpen());
@@ -648,18 +622,6 @@
         assertFalse(conn.isOpen());
         assertEquals(0, localServer.getAcceptedConnectionCount());
         
-        // check that there are no connections available
-        try {
-            // this should fail quickly, connection has not been released
-            getConnection(mgr, route, 100L, TimeUnit.MILLISECONDS);
-            fail("ConnectionPoolTimeoutException should have been thrown");
-        } catch (ConnectionPoolTimeoutException e) {
-            // expected
-        }
-
-        // return it back to the manager
-        ((AbstractClientConnAdapter) conn).releaseConnection();
-        
         // the connection is expected to be released back to the manager
         ManagedClientConnection conn2 = getConnection(mgr, route, 5L, TimeUnit.SECONDS);
         assertFalse("connection should have been closed", conn2.isOpen());
@@ -719,18 +681,6 @@
         }
         assertEquals(1, localServer.getAcceptedConnectionCount());
         
-        // check that there are no connections available
-        try {
-            // this should fail quickly, connection has not been released
-            getConnection(mgr, route, 100L, TimeUnit.MILLISECONDS);
-            fail("ConnectionPoolTimeoutException should have been thrown");
-        } catch (ConnectionPoolTimeoutException e) {
-            // expected
-        }
-
-        // return it back to the manager
-        ((AbstractClientConnAdapter) conn).releaseConnection();
-        
         // the connection is expected to be released back to the manager
         ManagedClientConnection conn2 = getConnection(mgr, route, 5L, TimeUnit.SECONDS);
         assertFalse("connection should have been closed", conn2.isOpen());
@@ -797,18 +747,6 @@
         }
         assertEquals(1, localServer.getAcceptedConnectionCount());
         
-        // check that there are no connections available
-        try {
-            // this should fail quickly, connection has not been released
-            getConnection(mgr, route, 100L, TimeUnit.MILLISECONDS);
-            fail("ConnectionPoolTimeoutException should have been thrown");
-        } catch (ConnectionPoolTimeoutException e) {
-            // expected
-        }
-
-        // return it back to the manager
-        ((AbstractClientConnAdapter) conn).releaseConnection();
-        
         // the connection is expected to be released back to the manager
         ManagedClientConnection conn2 = getConnection(mgr, route, 5L, TimeUnit.SECONDS);
         assertFalse("connection should have been closed", conn2.isOpen());

Propchange: httpcomponents/httpclient/branches/4.0.x/httpmime/
            ('svn:mergeinfo' removed)

Modified: httpcomponents/httpclient/branches/4.0.x/src/docbkx/fundamentals.xml
URL: http://svn.apache.org/viewvc/httpcomponents/httpclient/branches/4.0.x/src/docbkx/fundamentals.xml?rev=828205&r1=828204&r2=828205&view=diff
==============================================================================
--- httpcomponents/httpclient/branches/4.0.x/src/docbkx/fundamentals.xml (original)
+++ httpcomponents/httpclient/branches/4.0.x/src/docbkx/fundamentals.xml Wed Oct 21 20:46:31
2009
@@ -335,7 +335,7 @@
                 static methods to more easily read the content or information from an entity.
                 Instead of reading the <classname>java.io.InputStream</classname>
directly, one can
                 retrieve the whole content body in a string / byte array by using the methods
from
-                this class. However, the use of <interfacename>HttpEntity</interfacename>
is
+                this class. However, the use of <classname>EntityUtils</classname>
is
                 strongly discouraged unless the response entities originate from a trusted
HTTP
                 server and are known to be of limited length.</para>
             <programlisting><![CDATA[

Propchange: httpcomponents/httpclient/branches/4.0.x/src/docbkx/resources/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Wed Oct 21 20:46:31 2009
@@ -1 +1,2 @@
+/httpcomponents/httpclient/trunk/src/docbkx/resources:825864-828185
 /httpcomponents/httpcore/branches/ibm_compat_branch/src/docbkx/resources:755687-758898



Mime
View raw message