Fix regression in fix for POOL-425, fix race in POOL-426.

[psteitz]

Fixes the regression in the fix for POOL-425, which made addObject no-op when maxIdle is negative (no limit). Also fixes the race condition identified in POOL-426.

[rajucomp]

@psteitz Could you also include the additional tests at https://github.com/apache/commons-pool/pull/451/changes as well ? There is an additional test for POOL-413 as well and I think this fixes POOL-413 as well. Thanks!

[garydgregory]

Hello @rajucomp and @rajucomp

If I apply #451 to this PR locally (and edit to resolve the conflict), the tests pass and I get 2 checkstyle errors, so the test is OK. It might be cleaner for #451 to change and not use the same test method name as in this PR.

I can bring both PRs in if you are both OK with the changes.

[rajucomp]

@garydgregory

I don't think #451 fixes POOL-413 as per my investigation. If you run all the tests, the tests seems to pass but if you run the test only in isolation, it will fail. Not sure what's going on there. @psteitz could you confirm this ?
I think you can skip #451 as the current PR in the state at #452 contains the fix for POOL-426. Let's merge with the existing changes.
It would be better if we close #451 and try to fix POOL-413 in a separate PR #453.
This would keep the history clean and short and easy to review. Let me know what you both think.

Happy Holidays to you awesome folks!

[psteitz]

When I add just the tests, they fail, as I expected they would. I did not make changes to create, which it looks like one of the PRs against master was trying to do. I don't think that change is correct. The patch in this PR does not make changes to create.

[psteitz]

But thanks for the tests. Please add a link or just paste them to POOL-413 JIRA as they do illustrate that issue.

[rajucomp]

Yeah. That change is incorrect where we were taking the min value. I think the fix in this PR is correct for POOL-426. The fix is not correct for POOL-413.

[rajucomp]

@psteitz I have added the details in POOL-413 to illustrate the issue. There is a PR for the failing test at #453. A fix has been proposed in the JIRA itself. Let me know your thoughts on this.

[rajucomp]

This also fixes #450. Closing #450.

[rajucomp]

Closing POOL-425 and POOL-426.

[psteitz]

I am really sorry, guys, but I am going to have to revert this change. The added sync violates the "no factory methods while holding a lock" invariant, since it effectively extends the lock to all of create. I will revert the change and implement the simple fix for the regression.

[elrodro83]

Addendum mentioned in previous comment for reference: 8f46cde