commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John McNally <jmcna...@collab.net>
Subject [pool] faulty change of synchronization? - Re: cvs commit: jakarta-commons/pool/src/java/org/apache/commons/pool/impl GenericObjectPool.java
Date Wed, 13 Aug 2003 15:25:05 GMT
My quick read of this change is not favorable.  It looks like changes to
an unsynchronized list have been moved outside any synchronization. 
Also integer math like numActive++ outside a synchronization block has
got me into problems in the past.  A test with two threads using i++ and
i-- ran quite some time (couple hours) on a single processor box, but
did eventually fail.  Then the test was run on a dual processor box, it
failed pretty much immediately, repeatedly.

I'm -1 on this change.

john mcnally

On Wed, 2003-08-13 at 05:42, dirkv@apache.org wrote:
> dirkv       2003/08/13 05:42:28
> 
>   Modified:    pool/src/java/org/apache/commons/pool/impl
>                         GenericObjectPool.java
>   Log:
>   Bugzilla Bug 19614: [DBCP] Poor performance under load
>   Bugzilla Bug 22229: [DBCP] Foul connection causes livelock of all pool operations
>   
>   - remove synchronization on borrowObject
>   
>   Revision  Changes    Path
>   1.22      +76 -27    jakarta-commons/pool/src/java/org/apache/commons/pool/impl/GenericObjectPool.java
>   
>   Index: GenericObjectPool.java
>   ===================================================================
>   RCS file: /home/cvs/jakarta-commons/pool/src/java/org/apache/commons/pool/impl/GenericObjectPool.java,v
>   retrieving revision 1.21
>   retrieving revision 1.22
>   diff -u -r1.21 -r1.22
>   --- GenericObjectPool.java	12 Aug 2003 22:46:09 -0000	1.21
>   +++ GenericObjectPool.java	13 Aug 2003 12:42:28 -0000	1.22
>   @@ -163,6 +163,7 @@
>     *
>     * @see GenericKeyedObjectPool
>     * @author Rodney Waldhoff
>   + * @author Dirk Verbeeck
>     * @version $Revision$ $Date$
>     */
>    public class GenericObjectPool extends BaseObjectPool implements ObjectPool {
>   @@ -705,65 +706,113 @@
>    
>        //-- ObjectPool methods ------------------------------------------
>    
>   -    public synchronized Object borrowObject() throws Exception {
>   +    public Object borrowObject() throws Exception {
>   +        // Warning: because the method synchonization is gone
>   +        // _numActive should be incremented as soon as possible
>   +        // otherwise the pool can go over the limit
>   +        // decrement on error (do not forget to notifyAll)
>   +        
>            assertOpen();
>            long starttime = System.currentTimeMillis();
>            boolean newlyCreated = false;
>            for(;;) {
>                ObjectTimestampPair pair = null;
>                // if there are any sleeping, just grab one of those
>   -            try {
>   -                pair = (ObjectTimestampPair)(_pool.removeFirst());
>   -            } catch(NoSuchElementException e) {
>   -                ; /* ignored */
>   +            if (!_pool.isEmpty()) { // no need to synchronize when pool is empty
>   +                synchronized(this) {
>   +                    try {
>   +                        _numActive++;
>   +                        pair = (ObjectTimestampPair)(_pool.removeFirst());
>   +                    } catch(NoSuchElementException e) {
>   +                        ; /* ignored */
>   +                    }
>   +                    if(null == pair) { // someone took the last one before us
>   +                        _numActive--;
>   +                        notifyAll();
>   +                    }
>   +                }
>                }
>                // otherwise
>                if(null == pair) {
>                    // check if we can create one
>                    // (note we know that the num sleeping is 0, else we wouldn't be here)
>                    if(_maxActive <= 0 || _numActive < _maxActive) {
>   -                    Object obj = _factory.makeObject();
>   -                    pair = new ObjectTimestampPair(obj);
>   -                    newlyCreated = true;
>   +                    try {
>   +                        _numActive++;
>   +                        Object obj = _factory.makeObject();
>   +                        pair = new ObjectTimestampPair(obj);
>   +                        newlyCreated = true;
>   +                    }
>   +                    finally { 
>   +                        if(null == pair) {
>   +                            synchronized(this) {
>   +                                _numActive--;
>   +                                notifyAll();
>   +                            }
>   +                        }
>   +                    }
>                    } else {
>                        // the pool is exhausted
>                        switch(_whenExhaustedAction) {
>                            case WHEN_EXHAUSTED_GROW:
>   -                            Object obj = _factory.makeObject();
>   -                            pair = new ObjectTimestampPair(obj);
>   +                            try {
>   +                                _numActive++;
>   +                                Object obj = _factory.makeObject();
>   +                                pair = new ObjectTimestampPair(obj);
>   +                                newlyCreated = true;
>   +                            }
>   +                            finally {
>   +                                if(null == pair) {
>   +                                    synchronized(this) {
>   +                                        _numActive--;
>   +                                        notifyAll();
>   +                                    }
>   +                                }
>   +                            }
>                                break;
>                            case WHEN_EXHAUSTED_FAIL:
>                                throw new NoSuchElementException();
>                            case WHEN_EXHAUSTED_BLOCK:
>   -                            try {
>   -                                if(_maxWait <= 0) {
>   -                                    wait();
>   +                            synchronized(this) {
>   +                                // only sleep when the pool is really empty
>   +                                // between the isEmpty check at the beginning
>   +                                // and here, an object could have been added 
>   +                                // to the pool
>   +                                if (_pool.isEmpty()) {
>   +                                    try {
>   +                                        if(_maxWait <= 0) {
>   +                                            wait();
>   +                                        } else {
>   +                                            wait(_maxWait);
>   +                                        }
>   +                                    } catch(InterruptedException e) {
>   +                                        // ignored
>   +                                    }
>   +                                }
>   +                                if(_maxWait > 0 && ((System.currentTimeMillis()
- starttime) >= _maxWait)) {
>   +                                    throw new NoSuchElementException("Timeout waiting
for idle object");
>                                    } else {
>   -                                    wait(_maxWait);
>   +                                    continue; // keep looping
>                                    }
>   -                            } catch(InterruptedException e) {
>   -                                // ignored
>   -                            }
>   -                            if(_maxWait > 0 && ((System.currentTimeMillis()
- starttime) >= _maxWait)) {
>   -                                throw new NoSuchElementException("Timeout waiting
for idle object");
>   -                            } else {
>   -                                continue; // keep looping
>                                }
>                            default:
>                                throw new IllegalArgumentException("whenExhaustedAction
" + _whenExhaustedAction + " not recognized.");
>                        }
>                    }
>                }
>   -
>                try {
>                    _factory.activateObject(pair.value);
>                    if(_testOnBorrow && !_factory.validateObject(pair.value))
{
>                        throw new Exception("validateObject failed");
>                    }                
>   -                _numActive++;
>                    return pair.value;
>                } 
>                catch (Exception e) {
>   +                // object cannot be activated or is invalid
>   +                synchronized(this) {
>   +                    _numActive--;
>   +                    notifyAll();
>   +                }
>                    try {
>                        _factory.destroyObject(pair.value);
>                    } 
>   
>   
>   
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org



Mime
View raw message