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, [EMAIL PROTECTED] 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: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
