commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Phil Steitz <phil.ste...@gmail.com>
Subject Re: svn commit: r1101516 - in /commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl: GenericObjectPool.java PooledObject.java PooledObjectState.java
Date Wed, 11 May 2011 02:26:14 GMT
On 5/10/11 8:48 AM, markt@apache.org wrote:
> Author: markt
> Date: Tue May 10 15:48:22 2011
> New Revision: 1101516
>
> URL: http://svn.apache.org/viewvc?rev=1101516&view=rev
> Log:
> Move to using LinkedBlockingDeque for the queue of idle objects.

Definitely simpler, cleaner code, but seems there will be no easy
way to enable fairness and some badly "unfair" stuff can happen when
clients get instances under maintenance.  In theory, an unlucky
client could wait forever.  Do we have any data on how "unfair"
LinkedBlockingDeque can be?  ArrayBlockingQueue is an alternative
that does support fairness; but then I guess we lose LIFO/FIFO
control and probably performance.  Any ideas how we could get
fairness, or at least some control over fairness to work?  Another
thing to think about is whether clients are better off waiting for
the state change on instances under maintenance than getting back in
line for the next available instance. (I now see the fairness TODO
in TestGOP :)

Phil


> Added:
>     commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObject.java
  (with props)
>     commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObjectState.java
  (with props)
> Modified:
>     commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
>
> Modified: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
> URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java?rev=1101516&r1=1101515&r2=1101516&view=diff
> ==============================================================================
> --- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
(original)
> +++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/GenericObjectPool.java
Tue May 10 15:48:22 2011
> @@ -19,16 +19,17 @@ package org.apache.commons.pool2.impl;
>  
>  import java.util.ArrayList;
>  import java.util.Collection;
> +import java.util.Iterator;
>  import java.util.LinkedList;
>  import java.util.List;
>  import java.util.NoSuchElementException;
>  import java.util.TimerTask;
> +import java.util.concurrent.LinkedBlockingDeque;
>  
>  import org.apache.commons.pool2.BaseObjectPool;
>  import org.apache.commons.pool2.ObjectPool;
>  import org.apache.commons.pool2.PoolUtils;
>  import org.apache.commons.pool2.PoolableObjectFactory;
> -import org.apache.commons.pool2.impl.GenericKeyedObjectPool.ObjectTimestampPair;
>  
>  /**
>   * A configurable {@link ObjectPool} implementation.
> @@ -595,7 +596,7 @@ public class GenericObjectPool<T> extend
>          _softMinEvictableIdleTimeMillis = softMinEvictableIdleTimeMillis;
>          _testWhileIdle = testWhileIdle;
>  
> -        _pool = new CursorableLinkedList<ObjectTimestampPair<T>>();
> +        _pool = new LinkedBlockingDeque<PooledObject<T>>();
>          startEvictor(_timeBetweenEvictionRunsMillis);
>      }
>  
> @@ -1147,7 +1148,7 @@ public class GenericObjectPool<T> extend
>                                          // Case 3: An object has been allocated
>                                          _numInternalProcessing--;
>                                          _numActive++;
> -                                        returnObject(latch.getPair().getValue());
> +                                        returnObject(latch.getPair().getObject());
>                                      }
>                                  }
>                                  if (doAllocate) {
> @@ -1182,7 +1183,7 @@ public class GenericObjectPool<T> extend
>              if(null == latch.getPair()) {
>                  try {
>                      T obj = _factory.makeObject();
> -                    latch.setPair(new ObjectTimestampPair<T>(obj));
> +                    latch.setPair(new PooledObject<T>(obj));
>                      newlyCreated = true;
>                  } finally {
>                      if (!newlyCreated) {
> @@ -1197,22 +1198,22 @@ public class GenericObjectPool<T> extend
>              }
>              // activate & validate the object
>              try {
> -                _factory.activateObject(latch.getPair().getValue());
> +                _factory.activateObject(latch.getPair().getObject());
>                  if(_testOnBorrow &&
> -                        !_factory.validateObject(latch.getPair().getValue())) {
> +                        !_factory.validateObject(latch.getPair().getObject())) {
>                      throw new Exception("ValidateObject failed");
>                  }
>                  synchronized(this) {
>                      _numInternalProcessing--;
>                      _numActive++;
>                  }
> -                return latch.getPair().getValue();
> +                return latch.getPair().getObject();
>              }
>              catch (Throwable e) {
>                  PoolUtils.checkRethrow(e);
>                  // object cannot be activated or is invalid
>                  try {
> -                    _factory.destroyObject(latch.getPair().getValue());
> +                    _factory.destroyObject(latch.getPair().getObject());
>                  } catch (Throwable e2) {
>                      PoolUtils.checkRethrow(e2);
>                      // cannot destroy broken object
> @@ -1310,11 +1311,11 @@ public class GenericObjectPool<T> extend
>       */
>      @Override
>      public void clear() {
> -        List<ObjectTimestampPair<T>> toDestroy = new ArrayList<ObjectTimestampPair<T>>();
> +        List<PooledObject<T>> toDestroy = new ArrayList<PooledObject<T>>();
>  
>          synchronized(this) {
>              toDestroy.addAll(_pool);
> -            _numInternalProcessing = _numInternalProcessing + _pool._size;
> +            _numInternalProcessing = _numInternalProcessing + _pool.size();
>              _pool.clear();
>          }
>          destroy(toDestroy, _factory);
> @@ -1329,10 +1330,10 @@ public class GenericObjectPool<T> extend
>       * @param c Collection of objects to destroy
>       * @param factory PoolableConnectionFactory used to destroy the objects
>       */
> -    private void destroy(Collection<ObjectTimestampPair<T>> c, PoolableObjectFactory<T>
factory) {
> -        for (ObjectTimestampPair<T> pair : c) {
> +    private void destroy(Collection<PooledObject<T>> c, PoolableObjectFactory<T>
factory) {
> +        for (PooledObject<T> pair : c) {
>              try {
> -                factory.destroyObject(pair.getValue());
> +                factory.destroyObject(pair.getObject());
>              } catch (Exception e) {
>                  // ignore error, keep destroying the rest
>              } finally {
> @@ -1441,9 +1442,9 @@ public class GenericObjectPool<T> extend
>                      // borrowObject always takes the first element from the queue,
>                      // so for LIFO, push on top, FIFO add to end
>                      if (_lifo) {
> -                        _pool.addFirst(new ObjectTimestampPair<T>(obj));
> +                        _pool.addFirst(new PooledObject<T>(obj));
>                      } else {
> -                        _pool.addLast(new ObjectTimestampPair<T>(obj));
> +                        _pool.addLast(new PooledObject<T>(obj));
>                      }
>                      if (decrementNumActive) {
>                          _numActive--;
> @@ -1508,7 +1509,7 @@ public class GenericObjectPool<T> extend
>      @Override
>      @Deprecated
>      public void setFactory(PoolableObjectFactory<T> factory) throws IllegalStateException
{
> -        List<ObjectTimestampPair<T>> toDestroy = new ArrayList<ObjectTimestampPair<T>>();
> +        List<PooledObject<T>> toDestroy = new ArrayList<PooledObject<T>>();
>          final PoolableObjectFactory<T> oldFactory = _factory;
>          synchronized (this) {
>              assertOpen();
> @@ -1516,7 +1517,7 @@ public class GenericObjectPool<T> extend
>                  throw new IllegalStateException("Objects are already active");
>              } else {
>                  toDestroy.addAll(_pool);
> -                _numInternalProcessing = _numInternalProcessing + _pool._size;
> +                _numInternalProcessing = _numInternalProcessing + _pool.size();
>                  _pool.clear();
>              }
>              _factory = factory;
> @@ -1539,82 +1540,89 @@ public class GenericObjectPool<T> extend
>       */
>      public void evict() throws Exception {
>          assertOpen();
> -        synchronized (this) {
> -            if(_pool.isEmpty()) {
> -                return;
> -            }
> -            if (null == _evictionCursor) {
> -                _evictionCursor = (_pool.cursor(_lifo ? _pool.size() : 0));
> -            }
> -        }
>  
> -        for (int i=0,m=getNumTests();i<m;i++) {
> -            final ObjectTimestampPair<T> pair;
> -            synchronized (this) {
> -                if ((_lifo && !_evictionCursor.hasPrevious()) ||
> -                        !_lifo && !_evictionCursor.hasNext()) {
> -                    _evictionCursor.close();
> -                    _evictionCursor = _pool.cursor(_lifo ? _pool.size() : 0);
> +        if (_pool.size() == 0) {
> +            return;
> +        }
> +        
> +        PooledObject<T> underTest = null;
> +        
> +        for (int i = 0, m = getNumTests(); i < m; i++) {
> +            if (_evictionIterator == null || !_evictionIterator.hasNext()) {
> +                if (getLifo()) {
> +                    _evictionIterator = _pool.descendingIterator();
> +                } else {
> +                    _evictionIterator = _pool.iterator();
>                  }
> -
> -                pair = _lifo ?
> -                        _evictionCursor.previous() :
> -                        _evictionCursor.next();
> -
> -                _evictionCursor.remove();
> -                _numInternalProcessing++;
> -            }
> -
> -            boolean removeObject = false;
> -            final long idleTimeMilis = System.currentTimeMillis() - pair.getTstamp();
> -            if ((getMinEvictableIdleTimeMillis() > 0) &&
> -                    (idleTimeMilis > getMinEvictableIdleTimeMillis())) {
> -                removeObject = true;
> -            } else if ((getSoftMinEvictableIdleTimeMillis() > 0) &&
> -                    (idleTimeMilis > getSoftMinEvictableIdleTimeMillis()) &&
> -                    ((getNumIdle() + 1)> getMinIdle())) { // +1 accounts for object
we are processing
> -                removeObject = true;
>              }
> -            if(getTestWhileIdle() && !removeObject) {
> -                boolean active = false;
> +            if (!_evictionIterator.hasNext()) {
> +                // Pool exhausted, nothing to do here
> +                return;
> +            } else {
>                  try {
> -                    _factory.activateObject(pair.getValue());
> -                    active = true;
> -                } catch(Exception e) {
> -                    removeObject=true;
> -                }
> -                if(active) {
> -                    if(!_factory.validateObject(pair.getValue())) {
> -                        removeObject=true;
> -                    } else {
> -                        try {
> -                            _factory.passivateObject(pair.getValue());
> -                        } catch(Exception e) {
> -                            removeObject=true;
> -                        }
> -                    }
> +                    underTest = _evictionIterator.next();
> +                } catch (NoSuchElementException nsee) {
> +                    // Object was borrowed in another thread
> +                    // Don't count this as an eviction test so reduce i;
> +                    i--;
> +                    _evictionIterator = null;
> +                    continue;
>                  }
>              }
>  
> -            if (removeObject) {
> -                try {
> -                    _factory.destroyObject(pair.getValue());
> -                } catch(Exception e) {
> -                    // ignored
> -                }
> -            }
> -            synchronized (this) {
> -                if(!removeObject) {
> -                    _evictionCursor.add(pair);
> -                    if (_lifo) {
> -                        // Skip over the element we just added back
> -                        _evictionCursor.previous();
> +            if (!underTest.startEvictionTest()) {
> +                // Object was borrowed in another thread
> +                // Don't count this as an eviction test so reduce i;
> +                i--;
> +                continue;
> +            }
> +            
> +            if (getMinEvictableIdleTimeMillis() > 0 &&
> +                        getMinEvictableIdleTimeMillis() <
> +                            underTest.getIdleTimeMillis() ||
> +                    (getSoftMinEvictableIdleTimeMillis() > 0 &&
> +                            getSoftMinEvictableIdleTimeMillis() <
> +                                underTest.getIdleTimeMillis() &&
> +                            getMinIdle() < _pool.size())) {
> +                destroy(underTest);
> +            } else {
> +                if (getTestWhileIdle()) {
> +                    boolean active = false;
> +                    try {
> +                        _factory.activateObject(underTest.getObject());
> +                        active = true;
> +                    } catch(Exception e) {
> +                        destroy(underTest);
>                      }
> +                    if(active) {
> +                        if(!_factory.validateObject(underTest.getObject())) {
> +                            destroy(underTest);
> +                        } else {
> +                            try {
> +                                _factory.passivateObject(underTest.getObject());
> +                            } catch(Exception e) {
> +                                destroy(underTest);
> +                            }
> +                        }
> +                    }
> +                }
> +                if (!underTest.endEvictionTest()) {
> +                    // TODO - May need to add code here once additional states
> +                    // are used
>                  }
> -                _numInternalProcessing--;
>              }
>          }
> -        allocate();
> +
> +        return;
> +    }
> +
> +    private void destroy(PooledObject<T> toDestory) {
> +        _pool.remove(toDestory);
> +        try {
> +            _factory.destroyObject(toDestory.getObject());
> +        } catch (Exception e) {
> +            // Ignore
> +        }
>      }
>  
>      /**
> @@ -1720,9 +1728,8 @@ public class GenericObjectPool<T> extend
>          buf.append("Active: ").append(getNumActive()).append("\n");
>          buf.append("Idle: ").append(getNumIdle()).append("\n");
>          buf.append("Idle Objects:\n");
> -        long time = System.currentTimeMillis();
> -        for (ObjectTimestampPair<T> pair  : _pool) {
> -            buf.append("\t").append(pair.getValue()).append("\t").append(time - pair.getTstamp()).append("\n");
           
> +        for (PooledObject<T> pair  : _pool) {
> +            buf.append("\t").append(pair.toString());            
>          }
>          return buf.toString();
>      }
> @@ -1845,7 +1852,7 @@ public class GenericObjectPool<T> extend
>      private final class Latch {
>  
>          /** object timestamp pair allocated to this latch */
> -        private ObjectTimestampPair<T> _pair;
> +        private PooledObject<T> _pair;
>  
>          /** Whether or not this latch may create an object instance */
>          private boolean _mayCreate = false;
> @@ -1854,7 +1861,7 @@ public class GenericObjectPool<T> extend
>           * Returns ObjectTimestampPair allocated to this latch
>           * @return ObjectTimestampPair allocated to this latch
>           */
> -        private synchronized ObjectTimestampPair<T> getPair() {
> +        private synchronized PooledObject<T> getPair() {
>              return _pair;
>          }
>          
> @@ -1862,7 +1869,7 @@ public class GenericObjectPool<T> extend
>           * Sets ObjectTimestampPair on this latch
>           * @param pair ObjectTimestampPair allocated to this latch
>           */
> -        private synchronized void setPair(ObjectTimestampPair<T> pair) {
> +        private synchronized void setPair(PooledObject<T> pair) {
>              _pair = pair;
>          }
>  
> @@ -2042,10 +2049,9 @@ public class GenericObjectPool<T> extend
>      private boolean _lifo = DEFAULT_LIFO;
>  
>      /** My pool. */
> -    private CursorableLinkedList<ObjectTimestampPair<T>> _pool = null;
> +    private LinkedBlockingDeque<PooledObject<T>> _pool = null;
>  
> -    /** Eviction cursor - keeps track of idle object evictor position */
> -    private CursorableLinkedList<ObjectTimestampPair<T>>.Cursor _evictionCursor
= null;
> +    private Iterator<PooledObject<T>> _evictionIterator = null;
>  
>      /** My {@link PoolableObjectFactory}. */
>      private PoolableObjectFactory<T> _factory;
>
> Added: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObject.java
> URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObject.java?rev=1101516&view=auto
> ==============================================================================
> --- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObject.java
(added)
> +++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObject.java
Tue May 10 15:48:22 2011
> @@ -0,0 +1,79 @@
> +/*
> + * 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.
> + */
> +package org.apache.commons.pool2.impl;
> +
> +/**
> + * This wrapper is used to track the additional information, such as state, for
> + * the pooled objects.
> + */
> +public class PooledObject<T> {
> +
> +    private T _object = null;
> +    private volatile PooledObjectState _state = PooledObjectState.IDLE;
> +    private long _lastActiveTime = System.currentTimeMillis();
> +
> +    public PooledObject(T object) {
> +        _object = object;
> +    }
> +    
> +    /**
> +     * Obtain the underlying object that is wrapped by this instance of
> +     * {@link PooledObject}.
> +     */
> +    public T getObject() {
> +        return _object;
> +    }
> +
> +    /**
> +     * Obtain the time in milliseconds since this object was last active.
> +     */
> +    public long getIdleTimeMillis() {
> +        return System.currentTimeMillis() - _lastActiveTime;
> +    }
> +
> +    /**
> +     * Provides a String form of the wrapper for debug purposes. The format is
> +     * not fixed and may change at any time.
> +     */
> +    @Override
> +    public String toString() {
> +        StringBuilder result = new StringBuilder();
> +        result.append("Object: ");
> +        result.append(_object.toString());
> +        result.append(", State: ");
> +        result.append(_state.toString());
> +        return result.toString();
> +    }
> +
> +    public synchronized boolean startEvictionTest() {
> +        if (_state == PooledObjectState.IDLE) {
> +            _state = PooledObjectState.MAINTAIN_EVICTION;
> +            return true;
> +        }
> +        
> +        return false;
> +    }
> +
> +    public synchronized boolean endEvictionTest() {
> +        if (_state == PooledObjectState.MAINTAIN_EVICTION) {
> +            _state = PooledObjectState.IDLE;
> +            return true;
> +        }
> +        
> +        return false;
> +    }
> +}
>
> Propchange: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObject.java
> ------------------------------------------------------------------------------
>     svn:eol-style = native
>
> Added: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObjectState.java
> URL: http://svn.apache.org/viewvc/commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObjectState.java?rev=1101516&view=auto
> ==============================================================================
> --- commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObjectState.java
(added)
> +++ commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObjectState.java
Tue May 10 15:48:22 2011
> @@ -0,0 +1,54 @@
> +/*
> + * 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.
> + */
> +package org.apache.commons.pool2.impl;
> +
> +/**
> + * Provides the possible states that a {@link PooledObject} may be in.
> + * <ul>
> + * <li>{@link #IDLE}: In queue, not in use.</li>
> + * <li>{@link #ALLOCATED}: In use.</li>
> + * <li>{@link #MAINTAIN_EVICTION}: In queue, currently being tested for possible
> + *     eviction.</li>
> + * <li>{@link #MAINTAIN_EVICTION_RETURN_TO_HEAD}: Not in queue, currently being
> + *     tested for possible eviction. An attempt to borrow the object was made
> + *     while being tested which removed it from the queue. It should be returned
> + *     to the head of the queue once testing completes.</li>
> + * <li>{@link #MAINTAIN_VALIDATION}: In queue, currently being validated.</li>
> + * <li>{@link #MAINTAIN_VALIDATION_PREALLOCATED}: Not in queue, currently being
> + *     validated. The object was borrowed while being validated which removed it
> + *     from the queue. It should be allocated once validation completes.</li>
> + * <li>{@link #MAINTAIN_VALIDATION_RETURN_TO_HEAD}: Not in queue, currently
> + *     being validated. An attempt to borrow the object was made while
> + *     previously being tested for eviction which removed it from the queue. It
> + *     should be returned to the head of the queue once validation completes.
> + *     </li>
> + * <li>{@link #INVALID}: Failed maintenance (e.g. eviction test or validation)
> + *     and will be / has been destroyed.</li>
> + * </ul>
> + * 
> + * TODO: Find shorter names for these states without loss of meaning.
> + */
> +public enum PooledObjectState {
> +    IDLE,
> +    ALLOCATED,
> +    MAINTAIN_EVICTION,
> +    MAINTAIN_EVICTION_RETURN_TO_HEAD,
> +    MAINTAIN_VALIDATION,
> +    MAINTAIN_VALIDATION_PREALLOCATED,
> +    MAINTAIN_VALIDATION_RETURN_TO_HEAD,
> +    INVALID
> +}
>
> Propchange: commons/proper/pool/trunk/src/java/org/apache/commons/pool2/impl/PooledObjectState.java
> ------------------------------------------------------------------------------
>     svn:eol-style = native
>
>
>


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


Mime
View raw message