Mailing-List: contact user-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Users List" <user@commons.apache.org>
Received-SPF: pass (athena.apache.org: local policy)
From: Lee Breisacher <LBreisacher@seagullsoftware.com>
To: Jakarta Commons Users List <commons-user@jakarta.apache.org>
Date: Mon, 7 Jul 2008 13:58:07 -0400
Subject: RE: [pool] addObject/makeObject synchronization again
Thread-Topic: [pool] addObject/makeObject synchronization again
Thread-Index: Acjb21K07B+3U40vTtafhsgv1W4CRAEfnyWg
Message-ID: 
 <94C476C03BFF5E42AC3518FDAC9643C498CFF0CC90@HQMAIL.rocketsoftware.com>
References: 
 <94C476C03BFF5E42AC3518FDAC9643C498CFF0CC1D@HQMAIL.rocketsoftware.com>
In-Reply-To: 
 <94C476C03BFF5E42AC3518FDAC9643C498CFF0CC1D@HQMAIL.rocketsoftware.com>
Accept-Language: en-US
Content-Language: en-US
acceptlanguage: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Hm, now I've just into something else that's quite puzzling. When I set the=
 WhenExhaustedAction to WHEN_EXHAUSTED_BLOCK, doing a returnObject() does n=
ot seem to unblock the borrower. Am I completely misunderstanding how WHEN_=
EXHAUSTED_BLOCK is supposed to work? Here's the test:

 - in thread A, exhaust the pool by borrowing all the available objects, th=
en call borrowObject() once more. That waits.
 - in thread B, return one of the borrowed objects. That should unblock thr=
ead A, right? It does not.

Is this a gaping bug - hard to believe it's never been encountered before.

Thanks,

Lee

> -----Original Message-----
> From: Lee Breisacher [mailto:LBreisacher@seagullsoftware.com]
> Sent: Monday, July 07, 2008 9:46 AM
> To: Jakarta Commons Users List
> Subject: [pool] addObject/makeObject synchronization again
>
> Hi all. Referring to POOL-108, recall late last year there
> was a problem with GenericObjectPool being overly
> synchronized. In particular, I'm concerned with addObject()
> and the fact that the call to factory.makeObject() was
> synchronized and causing performance problems.
>
> At that time, Phil Steitz made some excellent suggestions for
> fixing it - see this JIRA entry:
> http://issues.apache.org/jira/browse/POOL-108?focusedCommentId
> =3D12547740#action_12547740, and the patches attached to
> POOL-108 indeed fix the problem. Related to this is POOL-93
> which has some similar patches attached to it. However, after
> the various patches were applied, version 1.4 went out like this:
>
>     public synchronized void addObject() throws Exception {
>         . . . .
>         Object obj =3D _factory.makeObject();
>         . . . .
>
> Which means POOL-108 is not actually fixed! The performance
> problem is still there.
>
> I believe the call to makeObject should be outside of the
> lock (as seen in the patch attached to POOL-108), so  it
> should look like this:
>
>     public void addObject() throws Exception {
>         assertOpen();
>         if (_factory =3D=3D null) {
>             throw new IllegalStateException("Cannot add
> objects without a factory.");
>         }
>         Object obj =3D _factory.makeObject();
>         synchronized(this) {
>         try {
>             assertOpen();
>             addObjectToPool(obj, false);
>         } catch (IllegalStateException ex) { // Pool closed
>             try {
>                 _factory.destroyObject(obj);
>             } catch (Exception ex2) {
>                 // swallow
>             }
>             throw ex;
>         }
>       }
>     }
>
> I pulled over the sources and made this change in my local
> copy. One of the unit tests failed, but closer examination
> revealed that the test was not quite right, so I fixed that
> and now all the unit tests pass (both with the synchronized
> change and without it).  I'm working on a new unit test right
> now that will demonstrate the problem with the current 1.4
> (overly synchronized) version.
>
> So, my primary question is: Does everyone agree with this
> fix? Should I re-open POOL-108, or create a new JIRA, or is
> commons-pool 1.x at end-of-life and I should just use my
> locally edited copy and be done with it?
>
> Note that I discussed this briefly off-list with Phil and his
> response (with further comments and questions from me) are below...
>
> > 1) The fixes in 1.4 solve the bulk of the real practical
> problem.  The
> > hot-and-heavy access to a production pool should not involve
> > addObject.
> > This method is only used to pre-fill a pool or by the
> > (dreaded) Evictor when trying to maintain minIdle (a bad
> practice in
> > most cases).  The basic borrow-return sequence that most
> apps hammer
> > in production has the factory methods outside of synxhronized scope
> > now.
>
> I don't understand why the Evictor is "dreaded" and why
> minIdle is a "bad practice". Can someone elaborate on that?
> Maintaining a minIdle works fabulously for our application.
> The objects in our pool are rather heavyweight and can take
> several seconds to create. Also, our usage pattern is such
> that a small number of objects in the pool is usually
> sufficient, but occasional spikes require extra objects, so
> having the background Evictor (obviously not a great name for
> it now that it also maintains the minIdle level) keeping the
> pool filled is a good thing.
>
> > 2) addObject is broken in another way that I did not want to make
> > worse in 1.4 - it does no accounting, so it is possible to add more
> > than maxActive objects to the pool using this method.
> Forcing it to
> > lock the pool while it does its work makes it less likely that the
> > pool will grow way beyond maxActive when invoking this
> method.  This
> > issue raises a larger problem that is hard to address with backward
> > compatibility in mind, which is what exactly does maxActive
> mean?  The
> > documentation is not chrystal clear and any way to make it clear
> > requires either a convoluted explanation of all of the
> strange corner
> > cases that people may be depending on in their applications or
> > cleaning things up in a way that may break some people.
>
> I'm not sure this problem really exists (I found the JIRA for
> it - POOL-120). Like I said, when I tweaked the unit tests
> slightly, I could no longer reproduce this problem (with the
> "weaker" synchronization like I'm suggesting).  And yeah, we
> don't use maxActive (does anyone?) and it does seem like a
> bit of an odd feature. We do have a notion of maxTotal (idle
> + active), but that's outside the pool implementation.
>
> Anyway, thanks for your time...
>
> Lee
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
> For additional commands, e-mail: user-help@commons.apache.org
>
>

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