commons-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lee Breisacher <LBreisac...@seagullsoftware.com>
Subject RE: [pool] addObject/makeObject synchronization again
Date Mon, 07 Jul 2008 18:05:33 GMT
Oops - never mind -- I had the wrong configuration on the pool. But all the discussion about
synchronization still applies. Sorry...

Lee

> -----Original Message-----
> From: Lee Breisacher [mailto:LBreisacher@seagullsoftware.com]
> Sent: Monday, July 07, 2008 10:58 AM
> To: Jakarta Commons Users List
> Subject: RE: [pool] addObject/makeObject synchronization again
>
> 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 not 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, then call borrowObject() once more. That waits.
>  - in thread B, return one of the borrowed objects. That
> should unblock thread 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
> > =12547740#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 = _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 == null) {
> >             throw new IllegalStateException("Cannot add objects
> > without a factory.");
> >         }
> >         Object obj = _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
>
>

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


Mime
View raw message