Re: Regression in Pool 2.13.0

RC vote is out: https://lists.apache.org/thread/fcjo1yt47m69yp1r1fmx858c31q18rp0

Gary

On Mon, Dec 29, 2025 at 3:49 PM Gary Gregory <[email protected]> wrote:
>
> On Mon, Dec 29, 2025 at 2:55 PM Phil Steitz <[email protected]> wrote:
> >
> > Sorry, that test should have been disabled.  The root cause of it is the
> > race condition in POOL-413, which is still open.
>
> OK, I've disabled that test and will restart teh RC...
>
> Gary
>
> >
> > On Mon, Dec 29, 2025 at 6:19 AM Gary Gregory <[email protected]> wrote:
> >
> > > On Mon, Dec 29, 2025 at 8:06 AM Gary Gregory <[email protected]>
> > > wrote:
> > > >
> > > > On Sun, Dec 28, 2025 at 12:44 PM Phil Steitz <[email protected]>
> > > wrote:
> > > > >
> > > > > I would say yes, get the regression fix out now and work on POOL-413
> > > et al
> > > > > afterwards.
> > > >
> > > > Starting RC...
> > >
> > > Hm, I just got this random error on my 3rd local build:
> > >
> > > [INFO] Running org.apache.commons.pool2.impl.TestGenericObjectPool
> > > [ERROR] Tests run: 110, Failures: 1, Errors: 0, Skipped: 1, Time
> > > elapsed: 114.3 s <<< FAILURE! -- in
> > > org.apache.commons.pool2.impl.TestGenericObjectPool
> > > [ERROR]
> > > org.apache.commons.pool2.impl.TestGenericObjectPool.testAddObjectConcurrentCallsRespectsMaxIdle
> > > -- Time elapsed: 0.013 s <<< FAILURE!
> > > org.opentest4j.AssertionFailedError: Concurrent addObject() calls
> > > should not exceed maxIdle limit of 5, but found 7 idle objects ==>
> > > expected: <true> but was: <false>
> > > at
> > > org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
> > > at
> > > org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
> > > at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
> > > at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
> > > at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
> > > at
> > > org.apache.commons.pool2.impl.TestGenericObjectPool.testAddObjectConcurrentCallsRespectsMaxIdle(TestGenericObjectPool.java:1033)
> > > at java.base/java.lang.reflect.Method.invoke(Method.java:569)
> > > at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
> > > at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
> > >
> > > Any thoughts?
> > >
> > > Gary
> > >
> > > >
> > > > Gary
> > > >
> > > > >
> > > > > Phil
> > > > >
> > > > > On Sun, Dec 28, 2025 at 6:04 AM Gary Gregory <[email protected]>
> > > wrote:
> > > > >
> > > > > > Hi Phil and all:
> > > > > >
> > > > > > If I apply https://github.com/apache/commons-pool/pull/453.diff
> > > > > > locally, running the test from Eclipse fails, and a Maven (default
> > > > > > goal) build fails on random repetition items, for example:
> > > > > >
> > > https://gist.github.com/garydgregory/fdfbb61360813326998c0e253b10ed52
> > > > > >
> > > > > > I assume that we want to proceed with an RC to address the
> > > regression,
> > > > > > and we are pushing this out to future work.
> > > > > >
> > > > > > Gary
> > > > > >
> > > > > > On Sun, Dec 28, 2025 at 7:37 AM Gary Gregory <[email protected]
> > > >
> > > > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Sat, Dec 27, 2025, 18:09 Phil Steitz <[email protected]>
> > > wrote:
> > > > > > >>
> > > > > > >> I updated the changelog and I think we are ready for a patch
> > > release.
> > > > > > >
> > > > > > >
> > > > > > > OK sounds good. I'll get to that today.
> > > > > > >
> > > > > > > Gary
> > > > > > >
> > > > > > >>
> > > > > > >> Phil
> > > > > > >>
> > > > > > >> On Sat, Dec 27, 2025 at 12:24 PM Gary Gregory <
> > > [email protected]>
> > > > > > >> wrote:
> > > > > > >>
> > > > > > >> > That all sounds good. I can create a release candidate anytime
> > > if you
> > > > > > want.
> > > > > > >> >
> > > > > > >> > Gary
> > > > > > >> >
> > > > > > >> > On Sat, Dec 27, 2025 at 1:43 PM Phil Steitz <
> > > [email protected]>
> > > > > > wrote:
> > > > > > >> > >
> > > > > > >> > > Given that the regression reported in POOL-427 is
> > > significant, I
> > > > > > think we
> > > > > > >> > > should move quickly to validate the fix for the regression (or
> > > > > > revert
> > > > > > >> > back
> > > > > > >> > > to the previous version of the method) and create a patch
> > > release
> > > > > > as soon
> > > > > > >> > > as possible.  The investigations around POOL-413 are great and
> > > > > > should
> > > > > > >> > > continue in parallel.  It would be great if we could discuss
> > > ideas
> > > > > > for
> > > > > > >> > how
> > > > > > >> > > to address the core issue there here instead of spread across
> > > PRs.
> > > > > > >> > >
> > > > > > >> > > Phil
> > > > > > >> > >
> > > > > > >> > > On Sat, Dec 27, 2025 at 11:10 AM Phil Steitz <
> > > [email protected]
> > > > > > >
> > > > > > >> > wrote:
> > > > > > >> > >
> > > > > > >> > > > I just reverted the added sync in PR #452, which violates
> > > the "no
> > > > > > >> > factory
> > > > > > >> > > > methods while holding locks" invariant.  Strangely, the
> > > added
> > > > > > tests for
> > > > > > >> > > > POOL-426 still pass.  I think the race condition is still
> > > present
> > > > > > and
> > > > > > >> > the
> > > > > > >> > > > general problem in POOL-413 remains unresolved.
> > > > > > >> > > >
> > > > > > >> > > > On Thu, Dec 25, 2025 at 3:55 PM Phil Steitz <
> > > > > > [email protected]>
> > > > > > >> > wrote:
> > > > > > >> > > >
> > > > > > >> > > >> The fix for POOL-425 included in the 2.13.0 release
> > > introduced a
> > > > > > >> > > >> regression that makes addObject no-op when maxIdle is set
> > > to a
> > > > > > >> > negative
> > > > > > >> > > >> value (no limit).  The POOL-425 fix also failed to account
> > > for a
> > > > > > race
> > > > > > >> > > >> condition reported in POOL-426.
> > > > > > >> > > >>
> > > > > > >> > > >> I have created a PR
> > > > > > https://github.com/apache/commons-pool/pull/452
> > > > > > >> > that addresses
> > > > > > >> > > >> both issues.  To avoid the race condition, I had to add
> > > > > > >> > synchronization to
> > > > > > >> > > >> addObject.  I tried several ways to avoid the race by
> > > modifying
> > > > > > >> > create (as
> > > > > > >> > > >> suggested by Raju Gupta, the OP for POOL-426) but I could
> > > not
> > > > > > find a
> > > > > > >> > way to
> > > > > > >> > > >> do that safely without introducing other issues.  I don't
> > > see the
> > > > > > >> > added
> > > > > > >> > > >> sync in addObject as critical as this method is not used
> > > in hot
> > > > > > code
> > > > > > >> > paths
> > > > > > >> > > >> internally and the lock that it acquires is the same lock
> > > that
> > > > > > create
> > > > > > >> > will
> > > > > > >> > > >> subsequently acquire if it proceeds to add an object.
> > > > > > >> > > >>
> > > > > > >> > > >> The regression could be addressed in a simpler way by just
> > > > > > fixing the
> > > > > > >> > > >> error in the code (failure to check for negative
> > > maxIdle).   If
> > > > > > there
> > > > > > >> > are
> > > > > > >> > > >> any doubts about the PR above, I am happy to make that
> > > simple
> > > > > > >> > change.  In
> > > > > > >> > > >> any case, we should patch this quickly as it will likely
> > > break
> > > > > > some
> > > > > > >> > apps
> > > > > > >> > > >> that use addObject with maxIdle unilimited.
> > > > > > >> > > >>
> > > > > > >> > > >> Thanks, all, and sorry for my mistake in the POOL-425 fix.
> > > > > > >> > > >>
> > > > > > >> > > >> Phil
> > > > > > >> > > >>
> > > > > > >> > > >
> > > > > > >> >
> > > > > > >> >
> > > ---------------------------------------------------------------------
> > > > > > >> > 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]
> > > > > >
> > > > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [email protected]
> > > For additional commands, e-mail: [email protected]
> > >
> > >