On 7/9/14, 2:29 PM, ma...@apache.org wrote:
> 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);
> +            }
>          }

Another option here might be to modify LinkedBlockingDeque to
support close() semantics and have close on the pool close the deque
(so addLast/First would return false on a closed deque and
returnObject would just destroy the one object instead of having to
add it back and then clear it).  I did some benchmarking a while
back to see if ripping out the capacity checking made any difference
to performance and was surprised to find that I could not measure
this.  A closed check might similarly have minimal performance
impact; or at least swapping out the needless capacity check for a
closed check might be net zero.
>  
>          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.

I played with this a little and unfortunately I think you are
right.  There is no way to make this happen reliably with the
current code.

Phil
> +     */
> +    //@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
>
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Reply via email to