Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Jakarta Commons Developers List" <dev@commons.apache.org>
Received-SPF: pass (nike.apache.org: domain of kutzi@gmx.de designates
 213.165.64.20 as permitted sender)
Message-ID: <477F9916.8040409@gmx.de>
Date: Sat, 05 Jan 2008 15:49:58 +0100
From: kutzi@gmx.de
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
 rv:1.8.1.9) Gecko/20071031 Thunderbird/2.0.0.9 Mnenhy/0.7.5.0
MIME-Version: 1.0
To: Jakarta Commons Developers List <dev@commons.apache.org>
Subject: Re: [POOL] Offer of help for a 1.4 release
References: <4752F4C5.6020403@apache.org>
	 <8a81b4af0712021144mff335e6of6aa465de6ca5e62@mail.gmail.com>
	 <47545EA7.7020201@apache.org>
 <475C7E6F.6040401@apache.org>	 <8a81b4af0712091935j13a44e71q651d76c74ba8e077@mail.gmail.com>
	 <20080103092057.60590@gmx.net>
 <477D3A3B.3020601@apache.org>	 <8a81b4af0801031809s7969a0fey9e903ae99aefc527@mail.gmail.com>
	 <477E1401.7080908@gmx.de>
 <8a81b4af0801041728i84eebc5l6620b5560e724187@mail.gmail.com>
In-Reply-To: <8a81b4af0801041728i84eebc5l6620b5560e724187@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Phil,

thanks for taking my issue serious.

Phil Steitz wrote:

> Thanks again for the feedback.   Any other opinions / suggestions on
> this are appreciated.  I suggest the following compromise, which would
> also fix the maxActive exceeded by one issue I discovered with
> 1.2/1.4-RC1 yesterday:
> 
> (*) Move makeObject back inside synchronized scope in borrowObject.
> (patch below)
> 
> This addresses Christoph's production use case (assuming I have
> understood it correctly) and also closes the maxActive exceeded by one
> problem that my testing uncovered yesterday.  It does not have a huge
> performance impact in the tests that I have run and still leaves
> activation and validation (the per-request operations) outside of
> synchronized scope.  More elegant solutions to both problems are
> certainly possible and we can consider them in subsequent releases -
> including configurable serialization of just the makes.

This is not what I was trying to achieve :-(
While this fixes my issue with the parallel creation of objects, it 
reopens another issue which I have with the implementation in pool 1.3:

The pool blocks completely while an object creation is in progress.
Just consider the following use case:

- Thread A tries to borrow an object. The pool contains no idle objects, 
hence a new one is created
- While object is still in creation, Thread B tries to return his 
borrowed object to the pool. Since returning the object depends on the 
same synchronization lock as makeObject, B will block as long as the new 
object isn't created (which can maybe take several seconds, as seen in 
my use case). I think this is unacceptable.
- Still while the creation is in progress, Thread C wants to borrow an 
object. It cannot continue, either, though an idle object would be 
available, if B could have returned its object

I propose, as in my previous post, a distinct lock for object creation.
I have attached a patch which should fix this behavior, but probably 
will not fix the "maxActive exceeded by 1". Do you already have a 
testcase for this one? The existing unit tests run without errors when 
my patch is applied.


Greetings
Christoph


Index: src/java/org/apache/commons/pool/impl/GenericObjectPool.java
===================================================================
--- src/java/org/apache/commons/pool/impl/GenericObjectPool.java 
(revision 609143)
+++ src/java/org/apache/commons/pool/impl/GenericObjectPool.java 
(working copy)
@@ -322,6 +322,8 @@
       */
      public static final long 
DEFAULT_SOFT_MIN_EVICTABLE_IDLE_TIME_MILLIS = -1;

+    private final Object makeObjectLock = new Object();
+
      //--- constructors -----------------------------------------------

      /**
@@ -920,7 +922,7 @@
                  } catch(NoSuchElementException e) {
                      ; /* ignored */
                  }
-
+
                  // otherwise
                  if(null == pair) {
                      // check if we can create one
@@ -969,19 +971,21 @@
              // create new object when needed
              boolean newlyCreated = false;
              if(null == pair) {
-                try {
-                    Object obj = _factory.makeObject();
-                    pair = new ObjectTimestampPair(obj);
-                    newlyCreated = true;
-                } finally {
-                    if (!newlyCreated) {
-                        // object cannot be created
-                        synchronized (this) {
-                            _numActive--;
-                            notifyAll();
-                        }
-                    }
-                }
+              synchronized( makeObjectLock ) {
+                  try {
+                      Object obj = _factory.makeObject();
+                      pair = new ObjectTimestampPair(obj);
+                      newlyCreated = true;
+                  } finally {
+                      if (!newlyCreated) {
+                          // object cannot be created
+                          synchronized (this) {
+                              _numActive--;
+                              notifyAll();
+                          }
+                      }
+                  }
+              }
              }

              // activate & validate the object


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