commons-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1609308 - in /commons/proper/pool/trunk/src: changes/ main/java/org/apache/commons/pool2/impl/ test/java/org/apache/commons/pool2/impl/
Date Wed, 09 Jul 2014 21:29:44 GMT
Author: markt
Date: Wed Jul  9 21:29:44 2014
New Revision: 1609308

URL: http://svn.apache.org/r1609308
Log:
POOL-263
Fix a threading issue that meant that concurrent calls to close() and returnObject() could
result in some returned objects not being destroyed.

Modified:
    commons/proper/pool/trunk/src/changes/changes.xml
    commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
    commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
    commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java

Modified: commons/proper/pool/trunk/src/changes/changes.xml
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/changes/changes.xml?rev=1609308&r1=1609307&r2=1609308&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/changes/changes.xml (original)
+++ commons/proper/pool/trunk/src/changes/changes.xml Wed Jul  9 21:29:44 2014
@@ -45,6 +45,10 @@ The <action> type attribute can be add,u
   <body>
   <release version="2.3" date="TBD" description=
 "TBD">
+    <action dev="markt" type="fix" issue="POOL-263">
+      Fix a threading issue that meant that concurrent calls to close() and
+      returnObject() could result in some returned objects not being destroyed.
+    </action>
     <action dev="psteitz" type="add" issue="POOL-262">
       Made fairness configurable for GenericObjectPool, GenericKeyedObjectPool.
     </action>

Modified: commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java?rev=1609308&r1=1609307&r2=1609308&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericKeyedObjectPool.java
Wed Jul  9 21:29:44 2014
@@ -535,6 +535,12 @@ public class GenericKeyedObjectPool<K,T>
             } else {
                 idleObjects.addLast(p);
             }
+            if (isClosed()) {
+                // Pool closed while object was being added to idle objects.
+                // Make sure the returned object is destroyed rather than left
+                // in the idle object pool (which would effectively be a leak)
+                clear(key);
+            }
         }
 
         if (hasBorrowWaiters()) {
@@ -1430,9 +1436,8 @@ public class GenericKeyedObjectPool<K,T>
          */
         public ObjectDeque(boolean fairness) {
             idleObjects = new LinkedBlockingDeque<PooledObject<S>>(fairness);
-            
         }
-        
+
         /**
          * Obtain the idle objects for the current key.
          *

Modified: commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1609308&r1=1609307&r2=1609308&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/main/java/org/apache/commons/pool2/impl/GenericObjectPool.java
Wed Jul  9 21:29:44 2014
@@ -111,7 +111,7 @@ public class GenericObjectPool<T> extend
             throw new IllegalArgumentException("factory may not be null");
         }
         this.factory = factory;
-        
+
         idleObjects = new LinkedBlockingDeque<PooledObject<T>>(config.getFairness());
 
         setConfig(config);
@@ -609,6 +609,12 @@ public class GenericObjectPool<T> extend
             } else {
                 idleObjects.addLast(p);
             }
+            if (isClosed()) {
+                // Pool closed while object was being added to idle objects.
+                // Make sure the returned object is destroyed rather than left
+                // in the idle object pool (which would effectively be a leak)
+                clear();
+            }
         }
         updateStatsReturn(activeTime);
     }
@@ -904,6 +910,12 @@ public class GenericObjectPool<T> extend
                 idleObjects.addLast(p);
             }
         }
+        if (isClosed()) {
+            // Pool closed while object was being added to idle objects.
+            // Make sure the returned object is destroyed rather than left
+            // in the idle object pool (which would effectively be a leak)
+            clear();
+        }
     }
 
     /**

Modified: commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java?rev=1609308&r1=1609307&r2=1609308&view=diff
==============================================================================
--- commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
(original)
+++ commons/proper/pool/trunk/src/test/java/org/apache/commons/pool2/impl/TestGenericObjectPool.java
Wed Jul  9 21:29:44 2014
@@ -754,6 +754,45 @@ public class TestGenericObjectPool exten
         }
     }
 
+    /**
+     * This is the test case for POOL-263. It is disabled since it will always
+     * pass without artificial delay being injected into GOP.returnObject() and
+     * a way to this this hasn't currently been found that doesn't involve
+     * polluting the GOP implementation. The artificial delay needs to be
+     * inserted just before the final call to isLifo() in the returnObject()
+     * method.
+     */
+    //@Test(timeout=60000)
+    public void testReturnObject() throws Exception {
+
+        pool.setMaxTotal(1);
+        pool.setMaxIdle(-1);
+        String active = pool.borrowObject();
+
+        assertEquals(1, pool.getNumActive());
+        assertEquals(0, pool.getNumIdle());
+
+        Thread t = new Thread() {
+
+            @Override
+            public void run() {
+                pool.close();
+            }
+
+        };
+        t.start();
+
+        pool.returnObject(active);
+
+        // Wait for the close() thread to complete
+        while (t.isAlive()) {
+            Thread.sleep(50);
+        }
+
+        assertEquals(0, pool.getNumIdle());
+    }
+
+
     @Test(timeout=60000)
     public void testSettersAndGetters() throws Exception {
         {
@@ -1396,7 +1435,7 @@ public class TestGenericObjectPool exten
             */
         }
     }
-    
+
     /**
      * Verifies that concurrent threads never "share" instances
      */
@@ -1516,7 +1555,7 @@ public class TestGenericObjectPool exten
                 boolean randomDelay, Object obj) {
             this(pool, iter, delay, delay, randomDelay, obj);
         }
-        
+
         public TestThread(ObjectPool<T> pool, int iter, int startDelay,
             int holdTime, boolean randomDelay, Object obj) {
         _pool = pool;
@@ -1827,7 +1866,7 @@ public class TestGenericObjectPool exten
             }
         }
     }
-    
+
     protected static class AtomicIntegerFactory
     extends BasePooledObjectFactory<AtomicInteger> {
 
@@ -1836,7 +1875,7 @@ public class TestGenericObjectPool exten
     private long createLatency = 0;
     private long destroyLatency = 0;
     private long validateLatency = 0;
-    
+
     @Override
     public AtomicInteger create() {
         try {
@@ -1873,7 +1912,7 @@ public class TestGenericObjectPool exten
         } catch (InterruptedException ex) {}
         return instance.getObject().intValue() == 1;
     }
-    
+
     @Override
     public void destroyObject(PooledObject<AtomicInteger> p) {
         try {
@@ -1881,7 +1920,7 @@ public class TestGenericObjectPool exten
         } catch (InterruptedException ex) {}
     }
 
-    
+
     /**
      * @param activateLatency the activateLatency to set
      */
@@ -1889,7 +1928,7 @@ public class TestGenericObjectPool exten
         this.activateLatency = activateLatency;
     }
 
-    
+
     /**
      * @param passivateLatency the passivateLatency to set
      */
@@ -1897,7 +1936,7 @@ public class TestGenericObjectPool exten
         this.passivateLatency = passivateLatency;
     }
 
-    
+
     /**
      * @param createLatency the createLatency to set
      */
@@ -1905,7 +1944,7 @@ public class TestGenericObjectPool exten
         this.createLatency = createLatency;
     }
 
-    
+
     /**
      * @param destroyLatency the destroyLatency to set
      */
@@ -1913,7 +1952,7 @@ public class TestGenericObjectPool exten
         this.destroyLatency = destroyLatency;
     }
 
-    
+
     /**
      * @param validateLatency the validateLatency to set
      */
@@ -1942,7 +1981,7 @@ public class TestGenericObjectPool exten
     })
     @Test(timeout=60000)
     public void testBorrowObjectFairness() throws Exception {
-        
+
         int numThreads = 40;
         int maxTotal = 40;
 
@@ -1951,9 +1990,9 @@ public class TestGenericObjectPool exten
         config.setMaxIdle(maxTotal);
         config.setFairness(true);
         config.setLifo(false);
-        
+
         pool = new GenericObjectPool(factory, config);
-        
+
         // Exhaust the pool
         String[] objects = new String[maxTotal];
         for (int i = 0; i < maxTotal; i++) {
@@ -1973,7 +2012,7 @@ public class TestGenericObjectPool exten
                 fail(e.toString());
             }
         }
-        
+
         // Return objects, other threads should get served in order
         for (int i = 0; i < maxTotal; i++) {
             pool.returnObject(objects[i]);
@@ -2310,7 +2349,7 @@ public class TestGenericObjectPool exten
         Assert.assertEquals(0, pool.getNumActive());
         Assert.assertEquals(1, pool.getNumIdle());
     }
-    
+
     // POOL-259
     @Test
     public void testClientWaitStats() throws Exception {
@@ -2327,8 +2366,8 @@ public class TestGenericObjectPool exten
         pool.borrowObject();
         // Second borrow does not have to wait on create, average should be about 50
         Assert.assertTrue(pool.getMaxBorrowWaitTimeMillis() >= 100);
-        Assert.assertTrue(pool.getMeanBorrowWaitTimeMillis() < 60);  
-        Assert.assertTrue(pool.getMeanBorrowWaitTimeMillis() > 40);  
+        Assert.assertTrue(pool.getMeanBorrowWaitTimeMillis() < 60);
+        Assert.assertTrue(pool.getMeanBorrowWaitTimeMillis() > 40);
     }
 
     private static final class DummyFactory



Mime
View raw message