[#878] Bound the JDBC connection pool and expire its connections one by one

[vharseko]

Fixes #878

Rebased onto master. #876, #880 and #883 have all landed, so the branch no longer carries the commit of #872 it was stacked on: it is a single commit on top of master, and the whole diff belongs to this PR. What the rebase had to decide rather than merge is at the end, under Rebased onto master.

Problem

The pool held its connections in an unbounded queue behind a Caffeine entry keyed by the connection string:

static LoadingCache<String, BlockingQueue<CachedConnection>> cached = Caffeine.newBuilder()
    .expireAfterAccess(Duration.ofMillis(getCacheTtlMillis()))
    .removalListener(...)
    .build(conStr -> new LinkedBlockingQueue<>());

Nothing limited how many connections a backend opened. Not the queue itself — getConnection establishes a new connection whenever the queue comes back empty, and no count of live connections existed anywhere. The peak is therefore the number of threads borrowing at once, i.e. the worker threads: WorkQueue.computeNumWorkerThreads falls back to Platform.computeNumberOfThreads(16, 2.0f), so max(16, 2 x CPU) by default. The only ceiling left was the max_connections of the database — which the backend treats as a condition to wait out, so a burst turned into a burst of connect attempts against a server already at its limit.

The TTL sat on the entry, not on a connection. expireAfterAccess is reset by every read of the entry, and both the borrow (cached.get(...)) and the return (cached.get(connectionString).add(this)) read it. Under continuous traffic the entry never expired and neither did anything in it: a burst that opened 200 connections kept all 200 for as long as the backend saw any traffic at all. The documented meaning of org.openidentityplatform.opendj.jdbc.ttl — "the time after which an idle pooled connection is closed" — only held when the whole backend was idle.

Expiry was lazy, and the case it exists for is the one it missed. No scheduler() on the builder, so an entry was only ever expired by a later cache operation — and a backend that has gone idle, the only situation in which it could expire at all, performs none.

A closed backend released nothing. JDBCStorage.close() only flipped the storage status, so disabling or removing a JDBC backend left its connections open, possibly for good by the point above.

And the return path raced: cached.get(connectionString) and .add(this) are two operations, so a connection could land in a queue evicted between them — never handed out again, never closed.

Fix

The cache entry is replaced by a pool of its own, ConcurrentMap<String, Pool>.

• A bound. A semaphore sized from org.openidentityplatform.opendj.jdbc.pool.max, defaulting to max(16, 2 x CPU) — the shape of the server's worker thread pool, since an operation holds one connection for its duration. The bound is a ceiling on connections held, not on operations served: a borrow above it waits for a returned connection until the deadline of pool.timeout ([#872] Bound the connect of the JDBC pool and report a connect it cannot make #876) and only then fails, naming the bound and the property it comes from, in the error the client sees and in a throttled warning in the server log — this is the one failure the change introduces, and a deployment whose peak sits above the default has to be able to attribute it. 0 means no bound.
• The TTL is a property of a connection. Idle connections are kept most recently returned first, each carrying the time it was returned. The hot ones are reused and the ones a burst opened sink to the bottom, which is where they are found — by the borrow, and by a sweeper thread running every half TTL, so expiry no longer needs a borrow behind it. The TTL is read on every borrow and every sweep rather than once in a static initializer, so it can be changed on a running server the way the bounds of [#872] Bound the connect of the JDBC pool and report a connect it cannot make #876 can. The sweep hands each close to an executor rather than performing it: every pool is swept on one thread, and one close that did not return would stop the expiry of all of them. It also gives up on the one connection a borrow took from under it rather than on the whole cycle, and nothing — an Error included — escapes it into scheduleWithFixedDelay, which never runs a task that threw again.
• A borrow is bounded in whichever phase it spends its time. Not only in the connect ([#872] Bound the connect of the JDBC pool and report a connect it cannot make #876) and in the wait for a returned connection, but in the emptying of the pool as well: pollFirst(0, MS) is a poll of no duration that still hands out whatever the deque holds, and a connection whose socket is half-open — a moved VIP, a firewall that dropped the idle sockets — costs the validation timeout to discard. The pool holds as many of those as its bound allows, so draining it overran the deadline the operator set: 80s of validation against a 60s pool.timeout by default, 320s on 32 cores, before the connect that follows had even started. The validation of a pooled connection is bounded by what is left of the borrow too.
• The return needs no lookup. A connection holds the pool it came from, so close() hands it back directly and the race is gone with the intermediate get.
• A nested borrow does not wait for the bound. PersistentCompressedSchema.store() opens a storage.write of its own — the definition has to commit independently of the entry — and EntryContainer.modifyDN (EntryContainer.java:2165) reaches it from inside a transaction, having encoded the entry there. Making the second borrow wait for the first would wait for this very thread. The exemption is from the wait rather than from the pool: a nested borrow served out of the idle deque carries the permit that connection already holds and is pooled again on return like any other, and only one that had to establish a connection of its own, because the pool stood at its bound, holds no permit — that one is closed rather than pooled when it comes back, so the pool does not grow past its bound. (addEntry and modifyEntry encode before the write and do not nest.) The count of what a thread holds is kept per pool, not per thread: that deadlock exists only within one pool, and a shared count would judge a thread holding a connection to one database reentrant while it borrows from another.
• A closed backend releases its connections. JDBCStorage registers as a user of the pool of its connection string on open(), borrows from that same string for as long as it is open, and gives it up on close(); the idle ones are closed when the last user goes, and one still on loan is closed when it comes back rather than pooled for a borrower that is not going to come. Reference counted because a pool belongs to a database rather than to a backend: two backends may address one database, and closing one must not take the connections of the other. The string is pinned rather than re-read, because db-directory reaches the listener of a running backend — applyConfigurationChange takes it, isConfigurationChangeAcceptable refuses nothing, and <adm:component-restart/> renders a message rather than holding the change back — so a borrow that followed the change would draw from a pool no storage had registered with, while the pool this one did register with kept a user that never borrows, and the unregistered one is drained the moment another backend that did register with it closes.

Nothing is left behind by a borrow that failed. The reservation an attempt takes is given back on every way out of it rather than on catch (SQLException) alone: DriverManager catches SQLException alone too, so an unchecked failure of a driver — Connector/J hands a url with a % in it to URLDecoder, and this backend keeps its credentials in the url — used to leave a permit behind, and nothing gives such a permit back. connect() closes the session it established whatever is thrown, and destroy() releases the permit from a finally.

The same holds for the import: ImporterImpl.close() guards its commit() against everything rather than against SQLException, and hands the connection back — and closes the stamp session — on every way out. Only that return gives back the permit the borrow took, and a pool is never removed from the static map, so an Error out of a bulk import used to cost the bound one permit for the life of the server. The failure of the return rides along with what escaped, by addSuppressed, instead of replacing it: the commit is what went wrong, and the rollback of the return fails on exactly the connection whose commit just did. The constructor gives back the connection it borrowed — and closes the storage it opened — when a transaction below it throws, since close() belongs to an object that was built.

close() on a connection already returned is a no-op, as the JDBC contract says, rather than putting it into the pool a second time.

Not in this

The leak of a connection whose rollback() fails in close() is already fixed by #876 (testConnectionThatCannotBeRolledBackIsClosed), so nothing here repeats it — it is listed in the issue analysis against master, where #876 has not landed.

Closing the pool at server shutdown, rather than only at backend close, is left out: the map is static, so an in-process shutdown of the whole server still leaves the pools behind. Every path that closes a backend now releases them, which covers the deployments the issue describes; a shutdown hook is a change to DirectoryServer rather than to this backend. Nothing removes a Pool from the map either, and the sweeper is never stopped, so a connection string used once is a pool for the life of the JVM.

The stamp connections of #866 are outside the bound: newStampConnection() goes to DriverManager directly, so the peak is pool.max plus one per concurrent stamp, and they carry none of the connect bounds of #876. Pre-existing, and newly worth stating now that a bound exists at all. It does go through poolKey() now, so a db-directory changed under a running backend no longer stamps the trees of this backend in a database the rest of it has stopped using.

pool.max is read when a pool is built rather than at every use, unlike connect.timeout, pool.timeout and ttl: a pool is never removed from the map and the permits of one already built are not resized, so this property takes a restart of the server — the failure that recommends raising it says so now, and so does its javadoc. None of the four is documented anywhere outside the source. DEFAULT_POOL_MAX still counts the worker threads only, while the replay threads of replication — which default to that same computeNumberOfThreads(16, 2) — and the import, backup and admin paths borrow on top of them; openPool() now reports the one difference it can measure, and picking a different default remains a decision about capacity rather than a fix.

Verification

Nineteen cases added to CachedConnectionTestCase, none of which needs a database — the stub driver of #876 serves them, so a regression fails the build wherever it runs:

case	asserts
testThePoolDoesNotGrowPastItsBound	with pool.max=2, two borrows on threads of their own fill the pool, a third fails with the bound named, and a returned connection then serves the next borrow
testABorrowNestedInAnotherMayPassTheBound	with pool.max=1, a second borrow on the same thread succeeds, is closed rather than pooled on return, and the pool keeps exactly one idle connection afterwards
testABorrowStopsAtItsDeadlineRatherThanDrainingThePool	with six pooled connections that each cost a second to find broken and a pool.timeout of one, the borrow gives up on the deque instead of draining it, and is served in about a second rather than in six
testABorrowWithNoDeadlineWaitsForAReturnedConnection	pool.timeout=0 waits without limit rather than giving up at once
testTheBoundOfThePoolReadsItsBoundaryValues	pool.max=0 is no bound, and a negative or non-numeric value falls back to the default
testAnIdleConnectionIsClosedAfterItsTtl	a connection idle past the TTL is closed and a fresh one established, instead of being handed out
testAZeroTtlKeepsNoIdleConnection	ttl=0 keeps nothing
testTheSweepClosesAnIdleConnectionWithNoBorrowBehindIt	what the sweeper runs, with no borrow involved, closes the connection and gives its place in the pool back
testTheSweepDoesNotCloseOnTheSweeperThread	what the sweeper runs hands the close on rather than performing it
testTheScheduledSweepClosesOnAThreadOfItsOwn	and the sweep the sweeper actually runs, with no executor supplied to it, closes on the pool of closer threads
testClosingTheLastUserReleasesTheConnections	the idle connections are closed and the pool is empty
testConnectionsSurviveWhileAnotherBackendStillUsesTheDatabase	closing one of two users keeps them; closing the second releases them
testAConnectionReturnedAfterTheLastUserLeftIsClosed	one on loan at that moment is closed when it comes back
testABackendClosedAndOpenedAgainPoolsItsConnections	a pool that lost its last user and gained one again pools as before
testTheStorageBorrowsFromThePoolItRegisteredWith	a db-directory changed under a running storage does not move its borrows, and its close() releases the pool it registered with
testAnImportGivesItsConnectionBackWhenTheCommitFailsUnchecked	an Error out of commit() is reported to the caller and the connection is back in the pool, with no permit lost
testAConnectFailingUncheckedCostsThePoolNothing	twice the bound of unchecked connect failures leaves meteredCount() == 0, and the pool still serves
testHoldingAConnectionToOneDatabaseDoesNotExemptABorrowFromAnother	the borrow from the second pool is metered and comes back to it
testASecondCloseDoesNotPoolTheConnectionTwice	one connection, one place in the pool

The bound case borrows one connection per thread on purpose: two borrows on one thread are nested by definition, and a nested one is allowed past the bound.

Test runs, all green:

suite	result
CachedConnectionTestCase	32/32 (13 of #876 + 19) — superseded, see below
PgSqlTestCase	54/54
MySqlTestCase	54/54

Every case that covers a mechanism of this PR was also run against that mechanism reverted, to check that it fails on the old behaviour rather than passing either way.

MsSqlTestCase and OracleTestCase were not run locally — nothing dialect-specific was added, the path is shared with the two engines above — and are left to CI.

Rebased onto master

#876, #880 and #883 landed while this branch stood, and it is now one commit on top of master
rather than a branch stacked on issues/872-jdbc-connect-timeout. The three commits it carried are
squashed into that one: their base was the first commit of #872, so every one of them would have had
to be replayed onto a file two rounds of review had rewritten, and resolving the same hunks three
times over would have been three guesses rather than one.

master grew the alive window of #879 and the read bound of #872 in the very code this PR replaces,
so four points of contact are decisions rather than merges:

• pollIdle() is told whether the borrow trusts the alive window, and asks isUsable() as
  master wrote it. The pool of this PR and that window are one layer, not two: the deque this
  change hands out from the hot end is what gives the window anything to bypass, and the validation
  it skips is the one this change would otherwise pay per connection it discards.
• A connection carries three sets of state at once — the poolable flag of JDBC backend: the connection pool connects without any timeout and retries a failed connect forever #872, the proof of
  life of JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879, and the pool, the meter and the permit of this PR. close() sends one that may not
  be pooled through Pool.destroy() rather than closing it outright, or the bound would lose a place
  for every connection kept out of the pool, permanently.
• Every pool lookup of a storage goes through poolKey(). getValidatedConnection() and
  distrustPool() of JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879 read db-directory again, where this PR had already established that the
  string open() registered with is the one to use: a db-directory changed on a running backend
  sent the validated borrow to a pool this storage never registered with, and filed the distrust
  against a pool holding none of its connections.
• A nested borrow takes a permit when the pool has room, and goes on unmetered only when the pool
  stands at its bound. This is a fix to this PR rather than to the merge: Pool.held documents the
  exemption as being from the wait rather than from the pool, while the code exempted a nested
  borrow from both — so a connection borrowed inside another was never pooled again, and the pool
  stopped handing out the connection returned last. testTheConnectionReturnedLastIsBorrowedFirst
  of [#879] Skip the validation of a pooled JDBC connection returned a moment ago #883 is what caught it.

Not carried over: the validation of a pooled connection is no longer clamped to what is left of
the borrow (isUsable(con, deadline)). master bounds that validation at the socket instead, and
the pollIdle loop checks the deadline after every connection it discards, so a drain of a pool the
database no longer answers overruns by at most the single validation it is inside — which is what
master does today. Say the word and it comes back as a commit of its own.

suite	result
CachedConnectionTestCase	83/83

That run covers the cases of this PR and the ones #872 and #879 added to the same class. The engine
suites have not been re-run since the rebase and are left to CI.

Review of the rebase, answered

a1bdc2f follows the review of 2 September and the reading of the branch it prompted. Nothing here changes what the pool does; the shape of the fix, the bound and the TTL are as reviewed.

The blocking one. close() lost the permit of a connection whose rollback() threw unchecked. The CAS that makes one close() the only one runs first, so an escape past it left the connection closed by nothing at all and its permit released by nothing either — destroy() is the only caller of releasePermit(), and a pool is never removed from the static map, so that place in the bound was gone for the life of the server. The hand-off is decided in a finally now: whatever does not reach give() reaches destroy(). The flag is raised before give() rather than after, so a connection that already reached the idle deque is not closed a second time under the borrower it is about to be handed to.

ImporterImpl.releaseConnection() needed the same widening for the same reason: its close() is that return, and an unchecked failure of it left with the failure of the commit — or, in the Throwable branch above it, with the Error that branch exists to preserve — dropped on the floor.

The three non-blocking ones.

• The status of a failed open(). It was set inside the try-with-resources of the validating borrow, so a throw from the implicit close() left the storage reporting working() while open() failed and gave its registration of the pool back. write() and ImporterImpl both skip the re-open when the status says working, so the pool stayed with no user and destroyed every connection returned to it. Set after the block now, and the registration is taken inside it.
• catch (Exception) where the file's own standard is Throwable. Widened in the ImporterImpl constructor and in open(). An Error is rethrown as it is rather than wrapped.
• liveCount(). It counted permits while its javadoc promised connections, and the ones it misses are exactly those past the bound. Renamed meteredCount() and documented as the count the bound is about.

The bound against the worker threads. Correct, and the comment that claimed otherwise is fixed: Platform.computeNumberOfThreads(16, 2) is only what WorkQueue.computeNumWorkerThreads falls back to, and a configured ds-cfg-num-worker-threads replaces it outright. No default can follow that count — the bound belongs to a database two backends may share, while the count belongs to the server — so openPool() names both in the log when the borrowers outnumber the places, rather than leaving the wait to be found in a latency graph. It is one difference and not the whole demand, and says so: the replay threads of replication default to that same count again and borrow on top of the workers.

Read again, and what that turned up

Re-reading the branch against those findings turned up more of the same family. All of it is in a1bdc2f.

A cross-thread return left its borrower permanently reentrant	The depth that exempts a nested borrow came down only where the returning thread was the borrowing one, and the connection forgot its borrower either way — so a return made on another thread left that borrower at a depth nothing could lower. That thread was then taken for a nested borrow for the life of the server, exempt from the wait at the bound, opening an unmetered connection per operation that the return then closed: a physical connect apiece, past a bound the operator set. The connection carries the counter of its borrower rather than the borrower itself.
releasePool() answered for a registration that was never made	It went by the CAS rather than by openPool() having returned. A throw between the two would take a user off a pool this storage never added one to — and the pool of a database two backends share would lose the user of the other one, draining connections it is still borrowing.
newStampConnection() was the one connection left following db-directory	Through poolKey() now, like every other.
The pool-full failure was throttled JVM-wide	On a single AtomicLong, while the stall warning twenty lines above is a map per connection string with a comment saying why. Two backends standing full at once: the one that reported first silenced the one whose operations are the ones failing.
The sweep interval was fixed at the first pool	Although the TTL is read on every borrow and every sweep so that it can be changed on a running server. The sweep books its next run out of the one before it now, so lowering the TTL lowers when connections are actually reaped.
The sweep could close a connection a millisecond old	It decided expiry from a reading taken during peekLast() and removed with a separate removeLastOccurrence(). A borrow that took the connection and gave it back between the two left the decision standing on a reading it no longer carried. The reading is taken again after the removal; a fresh connection goes back to the deque.
raise pool.max was advice that could not be taken	The property is read once per pool and a pool is never removed from the map, so the remedy the failure names needs a restart. It says so.
Dead weight	CachedConnection.invalidate() had no caller that ships — the drain it wrapped is reachable through the pool, and the tests call that. The public wrapper constructor passes poolable=false, which is what the accounting made of it anyway. Two comments the removal of Caffeine left behind — a removalListener that no longer exists, and a TTL described as read once — say what the code does now.

Verification of this round

suite	result
CachedConnectionTestCase	86/86

Three cases added:

case	asserts
testAConnectionWhoseRollbackFailsUncheckedIsClosed	an unchecked rollback() is reported, the connection is closed, and the pool keeps neither it nor its permit
testAnOpenThatFailsOnTheReturnLeavesTheStorageClosed	a borrow that could not be returned fails the open, the storage does not report working(), and the open that follows is not skipped
testAReturnOnAnotherThreadLowersTheDepthOfTheBorrower	after a cross-thread return the borrower waits at the bound and gives up, rather than passing it as a nested borrow

The third was run against the mechanism reverted and fails there on its own assertion, so it is not passing either way.

The engine suites have not been re-run since the rebase and are left to CI.

Merged with master (#882)

#877 (#882) has landed and this branch is merged with it. One file conflicts,
JDBCStorage.java, and the whole of it is one question: both branches moved the
borrow of an import, in different directions.

• The importer keeps the borrow this branch moved into its constructor, and
  startImport() is collapsed to match. [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 borrows in startImport() and gives
  the connection back through a finally when the importer cannot be built; this
  branch moved the open and the borrow into ImporterImpl() so that one owner gives
  back what one owner took. git flagged only the constructor: it had auto-merged
  startImport() to master's version, which borrows before building the importer, so
  taking the conflicted side alone would have borrowed twice - once in
  startImport(), once in the constructor - and leaked one of the two on every
  import. That is the merge deciding something it did not look like it was deciding,
  and it is the reason this section is worth reading rather than taking on the
  summary's word.
• Both transactions of the importer keep StatementBound.BULK. That is the
  contract of JDBC backend: no statement is given a query timeout, and the read bound of an established connection is gone too #877 - every statement an import issues is bulk by construction - and
  this branch was written before it, so its constructor carried neither.
• The ReadOnlyStorageException of [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 stays, inside the try rather than in
  front of it. [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 put the refusal where the importer is built, which is right and
  is kept. With the open now inside the same constructor, a refusal in front of the
  try would leave a storage this constructor opened with nobody to close it.
• One seam, naming the pool this storage registered with. [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 routed both borrows
  through getConnection(boolean trusted) so that a stand-in for the pool intercepts
  every path; this branch routes every borrow through poolKey() so that a
  db-directory changed on a running backend cannot send them to different pools.
  The seam is one method and it goes through poolKey().

One behaviour changed, and it is not cosmetic. The refusal now stands in front of
the borrow, so an import of a read-only storage takes no connection at all rather than
taking one and returning it. The two tests of #882 that pinned the return -
testStartImportGivesTheConnectionBackWhenTheImporterCannotBeBuilt and
testStartImportClosesTheStorageItOpenedWhenTheImporterCannotBeBuilt - now pin that
nothing is borrowed (borrows == 0, verify(con, never()).close()). The leak they
cover is the same one; what changed is that this state cannot reach it. The second
still pins the storage: the constructor opened it, so the refusal gives it back.

testEveryStatementOfAnImportIsBulk builds its importer through the borrow seam
instead of handing a connection to the constructor, on the same physical connection the
entry read beside it runs on - the backstop is keyed on the connection, not on the
storage, which is what that test was always about.

mvn -pl opendj-server-legacy test-compile is green, and so are
JDBCStatementBoundTestCase (37/37), CachedConnectionTestCase (86/86),
JDBCStorageRetryTest (66/66), StampConnectionTestCase (5/5), BulkCursorTest
(12/12) and PersistentCompressedSchemaTest (8/8) - 214/214 together. The four engine
suites have not been re-run since this merge and are left to CI.

[maximthomas]

comment was posted by mistake

[maximthomas]

Reviewed 368cb104f2 only (aa64e76142 is shared with #876). The design is sound and a clear improvement over master — the pool is bounded, the TTL is per connection, expiry no longer needs a later borrow, and a closed backend releases its connections. One issue should be fixed before merge; the rest are nits.

Permit accounting is not exception-safe (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java has no finally anywhere in 950 lines, and every permit release is wired to the happy path or to catch (SQLException). Three sites, one root cause:

// getConnection — reservation taken at :513, released only on SQLException
try {
    return borrowed(connect(connectionString, dialect, connectTimeoutSeconds, pool, !reentrant));
} catch (SQLException e) {
    if (!reentrant) {
        pool.cancelReservation();
    }

// connect — closeQuietly is inside the SQLException-only catch,
// so an unchecked throw here leaks the physical DB session too
} catch (SQLException e) { // nothing holds this connection yet: it would leak
    closeQuietly(conNew);
    throw e;
}

// Pool.destroy — closeQuietly catches only SQLException, so releasePermit() is skipped
void destroy(CachedConnection con) {
    closeQuietly(con.parent);
    con.releasePermit();
}

The permit's owner (the CachedConnection) is not constructed until the last line of connect(), so anything unchecked thrown before that orphans it with no object left to return it.

Reachable and reproduced: a % in a MySQL URL throws IllegalArgumentException from URLDecoder inside ConnectionUrlParser — the driver only wraps CJException, and DriverManager catches only SQLException around driver.connect. This project puts credentials in the URL (opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/MySqlTestCase.java:48), so a generated password containing % hits it. OutOfMemoryError reaches the same path.

1: IllegalArgumentException: URLDecoder: Illegal hex characters ... | liveCount=1
2: ...                                                             | liveCount=2
3: ...                                                             | liveCount=3
4: SQLTimeoutException: ... all 3 connections of the pool are in use (raise ...pool.max to allow more)

pools is static and never pruned, and only a live CachedConnection can return a permit, so nothing recovers it — not the database coming back, not closePool, not disabling and re-enabling the backend. After pool.max occurrences every borrow blocks the full 60 s and then reports that all connections are in use while holding none, pointing the operator at a property that would only feed the leak.

Suggested fix: try { ... } finally { if (!reentrant && !handedOff) pool.cancelReservation(); } around the borrow; widen the connect() guard to catch (Throwable) with a rethrow so closeQuietly always runs; wrap the close in destroy() in try { ... } finally { con.releasePermit(); }.

StubDriver (opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java) only ever throws SQLException, so CI does not defend this invariant — worth a driver mode that throws unchecked, asserting liveCount() == 0 afterwards.

A blocking close can park the sweeper for every pool (minor)

Pool.destroy() (opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java) closes inline on the single sweeper thread, and scheduleWithFixedDelay never overlaps runs, so one close that does not return stops TTL expiry JVM-wide — silently, since sweep() logs only thrown exceptions. Oracle's logoff() is a real round trip and relaxReadBound has lifted its read bound (Postgres/MySQL/MSSQL close() never read, so they are safe). abort(Executor) is already delegated and unused.

The borrow path does not compensate: it polls the head (pollFirst, freshest) while the sweep reaps the tail, so the stale tail is only reached once demand drains everything fresher.

held is global rather than per-pool (minor)

held (opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java) is one ThreadLocal for all pools, so a thread holding a connection to backend A is judged reentrant when borrowing from backend B: B's connection takes no permit, bypasses B's bound, and is destroyed instead of pooled on return. The deadlock the exemption guards against only exists within one pool, so the counter wants to be keyed by pool.

Nits

• pool.max is read once: it is the only property in the class not re-read at use (connect.timeout, pool.timeout and ttl all are), so it is fixed for a connection string from its first use. Worth one clause in the javadoc, since the neighbouring TTL javadoc advertises the opposite of itself.
• The held javadoc names a path it cannot cover: EntryContainer.importEntry's only caller passes a chunk-backed transaction that holds no connection, so held == 0 there and the exemption never fires. modifyDN is correct and should stay.
• ImporterImpl constructor is not exception-safe (opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java): con is borrowed and then two transactions are constructed on bare lines; a throw between them loses the borrow. Unreachable today (both startImport() sites close the storage first, so the mode is always re-opened READ_WRITE), but one refactor from becoming live. catch (RuntimeException e) { closeQuietly(con); throw e; } settles it.
• close() unregisters with a connection string open() may not have used (opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java): applyConfigurationChange replaces config in between, so a live db-directory change makes closePool target a different URL than openPool did. Capture the string used to register.
• open()'s catch unregisters unconditionally: the compareAndSet(false, true) may have skipped registration, but the failure path still runs compareAndSet(true, false) + closePool. Latent — no caller issues two open()s on one instance — but the two flags are only safe by accident.
• ImporterImpl.close() masks the commit failure: the new finally { con.close(); } lets the rollback error replace the SQLException from commit(), which is exactly the case where the commit failed on a broken connection. addSuppressed keeps both.
• Sweep interval is computed once: derived from the TTL in force when the first pool is created and never revisited, so a TTL lowered at runtime is still swept on the old period.
• DEFAULT_POOL_MAX counts only worker threads: MultimasterReplication.getNumberOfReplayThreadsOrDefault defaults to the same computeNumberOfThreads(16, 2.0f), and import/backup/admin paths borrow too. Where borrowers outnumber the bound, each excess borrow also occupies its worker for the full pool.timeout before failing.
• close() is not idempotent: a second call would put the same connection into the idle deque twice. No caller reaches it today, but JDBC's contract says close() on a closed connection is a no-op.
• No test for close-then-reopen: addUser()'s closed = false is what keeps rebuild, removeStorageFiles() and import from leaving a pool that never pools again, and it is the least covered line in the change. Relatedly, clearProperties() is a no-op for pools already built — the suite is correct only because each test uses a unique URL.
• lib/extensions comment is inaccurate: opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CachedConnectionTestCase.java says a driver "needs one dropped into lib/extensions by hand", but all four drivers ship in lib/ and opendj-server-legacy/resource/bin/_script-util.sh sets CLASSPATH=${INSTALL_ROOT}/lib/*, so lib/extensions is not on the server classpath at all.

Three pre-existing issues surfaced while reviewing this; none are caused by the PR and all deserve their own issues: RootContainer.open() calls storage.write() unconditionally even for READ_ONLY, which JDBCStorage.write() rejects, so offline export-ldif/verify-index/backendstat appear broken for JDBC backends; CompressedSchema.getAttributeId publishes a token before persisting it and never withdraws it on failure, leaving entries undecodable after a restart; and ImporterImpl's single Connection is written through concurrently by the phase-one and phase-two pools.

[vharseko]

Rebased onto master (0b9c0f6), which had moved on under the JDBC backend since this branch was cut (#886 catalog lookup, #866 table stamping and statistics, #867 SQL Server upsert).

Conflicts and how they were resolved:

• CachedConnection.getConnection(): master still carries the old recursive "retry the connect with a doubling wait" tail, which this stack replaces wholesale with the poll/connect/backoff loop (isConnectionLimit(), warnStall(), the SQLTimeoutException at the pool bound). The new implementation is what stands; the TTL of the Caffeine pool entry, which master configures elsewhere, is untouched.
• JDBCStorage.close(): both sides kept - unstampableTrees.clear() from master and closing the pool of this backend from here.
• ImporterImpl.close(): master's version kept (commit, statistics, then the connection back to the pool in a finally that also closes the stamp session), with the comment of this PR on why the connection has to go back even when the commit failed.

mvn -pl opendj-server-legacy test-compile passes, CachedConnectionTestCase 20/20 (no database). The container suites are left to CI.

Note this branch still carries #876 ([#872] Bound the connect of the JDBC pool ...) beneath its own commit, so it will need another rebase once that one is merged.

[vharseko]

Thanks - the permit accounting was the real one, and it is fixed together with both minors and most of the nits in 1a3e034453.

Permit accounting is not exception-safe

Fixed at all three sites, and the reachability holds up: DriverManager.getConnection wraps only catch (SQLException ex) around driver.connect, and ConnectionUrlParser.decode (Connector/J 9.2.0, com/mysql/cj/conf/ConnectionUrlParser.java:556) catches only UnsupportedEncodingException around URLDecoder.decode, so a % in a password reaches the borrow as an IllegalArgumentException.

• the attempt gives back what it took from a finally, not from catch (SQLException). A connection that was established but never handed to the caller goes out through pool.destroy(), which returns the permit along with it, so a throw from between the connect and the handoff leaks neither.
• connect() guards with catch (Throwable t) and rethrows, so the physical session is closed whatever comes out of the driver.
• Pool.destroy() releases the permit from a finally, and closeQuietly swallows RuntimeException as well as SQLException.

The driver mode you asked for is there: StubDriver.failWith now takes a Throwable, and testAConnectFailingUncheckedCostsThePoolNothing fails twice the bound with an unchecked exception, asserting liveCount() == 0 after each one and then borrowing successfully - so the pool must not report connections it does not hold as in use.

I checked that the new cases actually defend the invariant by reverting the three mechanisms and running the suite against the old behaviour:

testAConnectFailingUncheckedCostsThePoolNothing:304  attempt 1 kept a permit of the pool expected [0] but found [1]
testASecondCloseDoesNotPoolTheConnectionTwice:346    one connection was pooled twice expected [1] but found [2]
testHoldingAConnectionToOneDatabase...:328           the borrow passed the bound of the other pool expected [1] but found [0]

A blocking close can park the sweeper for every pool

Fixed. Pool.sweep takes the executor to close on; the scheduled sweep passes a cached pool of daemon threads (JDBC backend connection pool closer), and the one-argument sweep(ttl) still closes inline for callers that own the wait. A close that does not return now costs one parked thread instead of the expiry of every pool in the JVM. testTheSweepDoesNotCloseOnTheSweeperThread asserts that what the sweeper runs removes the connection from the pool and hands the close on, rather than performing it.

held is global rather than per-pool

Fixed - the counter is a field of Pool now. testHoldingAConnectionToOneDatabaseDoesNotExemptABorrowFromAnother holds a connection to one url and borrows from another on the same thread, asserting that the second borrow is metered and comes back to its pool.

Nits

Taken:

• ImporterImpl constructor is not exception-safe - and it loses more than the borrow: close() belongs to an object that was built, so the if (!isOpen) close() compensation never ran either, leaving the storage working() with the pool registered. The whole constructor is guarded now: the connection goes back and the storage this constructor opened is closed.
• close() unregisters with a connection string open() may not have used - open() keeps the string it registered with in poolConnectionString, and releasePool() gives the pool back to that one.
• open()'s catch unregisters unconditionally - it unregisters only what that call registered.
• ImporterImpl.close() masks the commit failure - the commit failure stays the exception and the failure of the return is addSuppressed onto it.
• The held javadoc names a path it cannot cover - you are right, OnDiskMergeImporter passes a PhaseOneWriteableTransaction to importEntry and that thread holds no connection. The javadoc names modifyDN only.
• close() is not idempotent - a second call is a no-op, reset by the borrow.
• No test for close-then-reopen - testABackendClosedAndOpenedAgainPoolsItsConnections.

Left alone, and worth saying so rather than leaving them to be found again:

• pool.max is read once and the sweep interval is computed once - both still true, neither documented yet.
• DEFAULT_POOL_MAX counts only worker threads - agreed on the arithmetic; picking a different default is a decision about capacity rather than a fix, so it is not in this commit.
• clearProperties() is a no-op for pools already built - still so; the new cases use unique urls like the rest of the suite.
• lib/extensions comment - it belongs to [#872] Bound the connect of the JDBC pool and report a connect it cannot make #876 (aa64e76142), so the fix goes there. Your reading of the classpath is right: _script-util.sh:273 sets CLASSPATH=${INSTALL_ROOT}/lib/*, the assembly creates lib/extensions empty, and what ConfigurationFramework loads from it goes into a class loader of its own that DriverManager will not take a driver from.

One more that is not covered here: sweep() catches RuntimeException but not Error, and scheduleWithFixedDelay cancels a task that throws - so an Error would stop the expiry for good, and silently, which is the same failure mode as the blocking close. Say the word and it goes in.

The three pre-existing ones

Agreed, all three deserve issues of their own. I confirmed the first against the code: RootContainer.java:134-135 calls storage.write(...) unconditionally after open(accessMode), and JDBCStorage.write() builds a WriteableTransactionTransactionImpl, which throws ReadOnlyStorageException for a storage opened READ_ONLY (JDBCStorage.java:1078).

Test runs

suite	result
CachedConnectionTestCase	25/25 (20 + 5)
PgSqlTestCase	54/54
MySqlTestCase	54/54

MsSqlTestCase and OracleTestCase are left to CI - nothing dialect-specific was touched.

[vharseko]

Filed the pre-existing ones. A correction to what I wrote above: only two of the three needed an issue.

• Offline tools cannot open a JDBC backend - already tracked as Offline export-ldif, verify-index and backendstat cannot open a JDBC backend #874, with [#874] Grant the offline tools a read-only JDBC transaction instead of refusing it #880 open against it. Same diagnosis as yours, down to RootContainer.open() asking for a write transaction that JDBCStorage refuses.
• Compressed schema keeps an attribute token whose store failed, leaving entries that reference it undecodable #890 - the compressed schema keeps an attribute token whose store failed. It turns out worse than "never withdrawn on failure": the registration stays, so every later encode takes the lock-free fast path and writes entries with a token the tree does not carry. After a restart loadAttributeToMaps pads the gap with null, and those entries decode into ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN - or, when the lost token was the highest one, into an IndexOutOfBoundsException off CopyOnWriteArrayList.get, since the ad == null branch is the only one decodeAttribute guards. The first schema change afterwards then NPEs in reloadAttributeTypeMaps, which walks the same list and dereferences the padded slot.
• JDBC backend: the importer shares one JDBC connection across every import thread #891 - the importer shares one connection across every import thread. Importer is @ThreadSafe by contract ("implementations must be thread-safe"), phase two runs invokeParallel on a cached thread pool with one thread per chunk, and phase one clears a tree per entry container. Two consequences beyond the interleaving: clearTree, deleteTree and openTree each end in con.commit(), so one thread commits whatever else is in flight - ImporterImpl.close() is written as though its commit were the one that decides durability - and StampSession.connection() is a plain lazy if (con==null), so two threads open two connections and one is never closed. PDBStorage.ImporterImpl shows the shape that satisfies the contract, a ThreadLocal of the per-thread state.

The sweep() catching RuntimeException but not Error is not filed - it is in this PR's own code, so it belongs here if you want it in.

[maximthomas]

The design holds and does what it says — the bound, the per-connection TTL, the sweeper, the refcount and the permit finallys are all there, and the round-1 fixes land. Three majors below, no blockers. One of them is not new: it is the poll() defect from the #876 review, carried through the rewrite.

ImporterImpl.close() returns the connection outside a finally (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1581-1615

The PR body says the connection goes back "from a finally block". It goes back below a catch:

try { con.commit(); ... updateTableStatistics(con, writtenTrees); }
catch (SQLException e) { failure=e; }          // SQLException only
try { con.close(); } catch (SQLException e) { ... }
finally { txw.stampSession.close(); }

Any throwable that is not an SQLException out of con.commit() (CachedConnection:765 hands straight to parent.commit()) or out of updateTableStatistics() skips both con.close() and stampSession.close(). Only CachedConnection.close() → give()/destroy() releases the permit, so it is gone.

It does not heal on re-enable: nothing removes a Pool from the static pools map, so poolOf() hands back the same Semaphore, one permit short. ImportTask:684 catches Exception → STOPPED_BY_ERROR and the JVM lives on; repeats walk the bound to zero, after which every borrow blocks the full pool.timeout.

Reachable sources are Error (OOM in a bulk import) and a driver's commit() — not a RuntimeException from this file. getTableName is a pure SHA-224 hash and #886's catalog lookup is in openTree, so the obvious candidate does not fire.

try {
    con.commit();
    ...
} finally {
    try { con.close(); } catch (SQLException e) { /* addSuppressed */ }
    finally { txw.stampSession.close(); }
}

pollIdle() never consults the borrow deadline (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:290-308

Same defect the #876 review raised against the then-named poll(); the rewrite carried it over.

remainingWait -= System.currentTimeMillis() - polledAt;
if (remainingWait <= 0) { remainingWait = 0; }   // does not end the loop

pollFirst(0, MS) is a non-blocking poll, so the clamp only makes the rest of the polls non-blocking — the loop drains the whole deque, paying isValid(VALIDATION_TIMEOUT_SECONDS=5) per entry. The only exit is an empty deque. deadline is read at :569 (pool full) and :594 (connection limit) — never before pollIdle, never after it returns null.

A moved VIP or a firewall idle-timeout leaves the pooled sockets half-open, and the sweeper only reaps past the TTL (15s), so a burst's connections all pass the TTL check at :298 and each blocks 5s. Worst case in one pollIdle = idleCount × 5s, idleCount ≤ pool.max: 80s by default, 320s on 32 cores, against a 60s pool.timeout. Then tryReserve() succeeds (every destroy released its permit) and the borrow falls into connect() for another 30s — ~110s for a borrow the operator bounded at 60s, on every worker at once.

Worth noting alongside: relaxReadBound() (:659) sets networkTimeout 0 on pooled connections, so closeQuietly(parent) inside destroy() (:342) is itself unbounded on a driver that talks on close (Oracle logoff) — and it runs on the borrowing thread.

Pass the deadline into pollIdle and break on it; bound the validation with min(VALIDATION_TIMEOUT_SECONDS, remaining).

A live db-directory change borrows from a pool with no users (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:107-118, with :143-145

open() pins the string; the borrow re-reads it:

poolConnectionString = config.getDBDirectory();          // open(), pinned once
CachedConnection.openPool(poolConnectionString);
...
return CachedConnection.getConnection(config.getDBDirectory());   // getConnection(), live

applyConfigurationChange does only this.config = cfg;, and isConfigurationChangeAcceptable returns true unconditionally. Change X → Y on a running backend and pool X keeps users=1 with nothing borrowing from it, while every operation goes to poolOf(Y) with users=0. close() then drains X and never Y — the leak #878 exists to remove, back through the config path. Worse, if a second backend had registered Y and is later disabled, removeUser takes Y to zero, sets closed=true and drains it while this backend is still borrowing from it; give() destroys every returned connection instead of pooling it, so the live backend reconnects per operation, permanently.

The <adm:requires-admin-action><adm:component-restart/> on db-directory does not prevent this — AdministratorAction only renders a dsconfig/doc message, and BackendConfigManager.applyConfigurationChange acts on enabled and java-class alone; everything else reaches the backend's own listener.

Simplest fix: borrow from poolConnectionString instead of re-reading config. Otherwise handle the swap in applyConfigurationChange (openPool(new), closePool(old)), or reject the change in isConfigurationChangeAcceptable if a live change is not meant to be supported.

Nits

• Nested-borrow javadoc is false for the idle-hit case (CachedConnection.java:201): pollIdle (:560) runs before the reentrancy branch (:564), so a nested borrow served from the deque is metered and is pooled on return. The bound still holds — give() destroys unmetered connections — but "never pooled on return" and the PR's "holds no permit" are wrong as written. liveCount() also cannot see in-flight unmetered borrows.
• The sweeper hand-off test cannot fail (CachedConnectionTestCase.java:365): pool.sweep(1000, handedOff::add) supplies its own executor, so the production closer field (CachedConnection:92/121/143) is never read. Reverting closer to DIRECT_EXECUTOR leaves it green. The scheduleWithFixedDelay wiring (:132) has no test at all.
• The JDBCStorage half is untested: every new case drives CachedConnection.openPool/closePool directly. poolRegistered, poolConnectionString and releasePool() — four of the round-1 fixes — are asserted by nothing; deleting both calls leaves the suite green.
• Two sweep tests race the live sweeper (CachedConnectionTestCase.java:199, :361): both back-date returnedAtMillis to now-60000, and the scheduled sweeper runs every max(1000, 15000/2)=7500ms over pools.values() with a 15s TTL. A sweep landing before the assertion empties the deque. The class runs well past 7.5s, so this is a real CI flake.
• The pool-full failure logs nothing (CachedConnection.java:564): the one new failure mode of this PR throws SQLTimeoutException after 60s with no server-log line; warnStall() is only on the connection-limit path (:602). On upgrade, a deployment over the new default bound sees LDAP errors with nothing attributing them to it.
• sweep() abandons the whole cycle on one lost race (:367): !idle.removeLastOccurrence(con) is treated like "nothing expired" and returns. One concurrent borrow of the tail leaves every other expired connection open until the next sweep. continue rather than return.
• close() never removes the config change listener (JDBCStorage.java:199): the constructor calls addJDBCChangeListener(this); PDBStorage.close() removes its own. A disabled backend keeps mutating this.config — the input to major 3.
• isClosed() delegates to the parent (CachedConnection.java:800): a connection already returned to the pool answers false. No in-tree caller since the Caffeine removalListener went, so this is SPI surface only.
• Nothing is ever torn down (:86, :209): no pools.remove, no ThreadLocal.remove, no counterpart to startSweeper(). One Pool per connection string ever used, retained for the JVM's life, and the sweeper keeps waking for pools that will never hold a connection again.
• Boundary contracts untested: no case sets pool.max=0 (→ Integer.MAX_VALUE), pool.timeout=0 (→ Long.MAX_VALUE), ttl=0, or a negative/non-numeric value. "0 means unbounded" is operator-facing; inverting either mapping would pass.
• Wall-clock time for TTL and deadlines (:336, :364): System.currentTimeMillis(), while JDBCStorage.write() (:892) uses nanoTime. An NTP step back stops the TTL firing; a step forward expires the pool at once.
• The properties are documented nowhere: pool.max, pool.timeout, connect.timeout and ttl appear only in source. The one place an operator learns the bound exists is the exception text.
• Stamp connections are outside the bound (JDBCStorage.java:404): newStampConnection() calls DriverManager.getConnection directly, so the peak is pool.max plus one per concurrent stamp, and they carry none of #876's connect bounds. Pre-existing (#866), newly relevant now that a bound exists — worth a line in the PR text either way.

[vharseko]

Thanks — all three majors hold up against the code, and they are fixed together with most of the nits in 642729ffc3. The PR text is updated too: both places you caught it describing something other than what the code does are rewritten, and the things left alone are now stated in "Not in this" rather than left to be found again.

ImporterImpl.close() returns the connection outside a finally

Fixed. The commit is guarded against everything rather than against SQLException, and releaseConnection() hands the connection back and closes the stamp session on every way out; the failure of the return is addSuppressed onto what escaped instead of replacing it.

It costs more than the permit, which is worth recording: skipping con.close() also skips pool.leave() (CachedConnection:781-783), so the held counter of that thread stays above zero for good. Every later borrow on it is then taken for a nested one — unmetered, past the bound, and destroyed rather than pooled on return. Import threads are reused, so one such failure degrades a whole thread to a physical connect per operation, which is worse than the one lost permit.

testAnImportGivesItsConnectionBackWhenTheCommitFailsUnchecked covers it: commit() throws an Error, the Error reaches the caller, the connection is back in the pool and the permit with it.

Your reading of the reachability holds up. updateTableStatistics wraps its loop body in catch (Exception) (JDBCStorage.java:800) and getTableName is a pure SHA-224 hash, so commit() and an Error are what is left.

pollIdle() never consults the borrow deadline

Fixed. pollIdle takes the deadline and returns null once it has passed, and isUsable bounds the validation with min(VALIDATION_TIMEOUT_SECONDS, remaining) — never 0, which the JDBC contract reads as "no timeout".

testABorrowStopsAtItsDeadlineRatherThanDrainingThePool puts six connections that each cost a second to validate into a pool with pool.timeout=1, and asserts the borrow is served in about a second. Against the old code it measures 6051 ms, so the arithmetic you gave reproduces.

The unbounded closeQuietly(parent) inside destroy() on the borrowing thread is left as it is. It is the same trade the sweeper hand-off makes, but on the borrow path there is nobody to hand it to without returning to the caller a connection it does not hold. Say the word and it goes onto the closer pool as well.

A live db-directory change borrows from a pool with no users

Fixed the simple way: the borrow uses poolConnectionString — the string open() registered with — and falls back to the configuration only when there is no registration. testTheStorageBorrowsFromThePoolItRegisteredWith changes db-directory under a running storage, asserts the borrow does not follow it, and asserts close() releases the pool it did register with.

The configuration path is as you describe: BackendConfigManager.applyConfigurationChange (:820-900) acts on enabled and java-class and returns, so everything else reaches the listener of the storage, and <adm:component-restart/> on db-directory (JDBCBackendConfiguration.xml:57-59) only renders a message.

Nits

Taken:

• Nested-borrow javadoc is false for the idle-hit case — right, pollIdle runs before the reentrancy branch. The javadoc says now that the exemption is from the wait rather than from the pool: a nested borrow served out of the deque carries the permit that connection already holds and is pooled like any other, and only one that had to establish a connection of its own holds none. Same correction in the PR text.
• The sweeper hand-off test cannot fail — testTheScheduledSweepClosesOnAThreadOfItsOwn calls the sweep with nothing supplied to it and asserts the close ran on the closer pool. Reverting closer to DIRECT_EXECUTOR fails it.
• The JDBCStorage half is untested — two cases drive it now, through open(), getConnection(), startImport() and close().
• Two sweep tests race the live sweeper — both set the TTL to 600000 before back-dating, so the sweeper running beside them cannot reach the connection while the explicit sweep(1000, ...) still can. The window was microseconds wide rather than a likely flake, but it costs one line to close.
• The pool-full failure logs nothing — warnPoolFull, throttled on the same interval as the stall warning, since every worker thread arrives at it at once.
• sweep() abandons the whole cycle on one lost race — continue.
• Boundary contracts untested — pool.max at 0, negative and non-numeric; pool.timeout=0; ttl=0.
• And the one from round 1 that never got an answer: sweep() catches Throwable now. scheduleWithFixedDelay never runs a task that threw again, so an Error stopped the expiry of every pool in the JVM — the same failure mode as the blocking close.

Left alone, with the reason:

• close() never removes the config change listener — it does not, but PDBStorage is not the model to copy: it removes the listener in close() (:964) and adds it in the constructor only (:894), while PDBStorage$ImporterImpl.close() calls PDBStorage.this.close() (:285) — so PDB loses its listener after every import, and JDBCStorage.close() carries the same double duty. With the borrow no longer reading config, the listener staying is not the input to anything any more; doing it properly means finding the path that finalizes the backend, which is an issue of its own.
• Nothing is ever torn down, stamp connections outside the bound, pool.max read once, the sweep interval computed once, DEFAULT_POOL_MAX counts only worker threads, the properties are documented nowhere — all still true, all now stated in "Not in this" rather than implied. On the last one: grep finds none of the four outside the source, ttl included, so the gap predates the bound; a doc change belongs to its own issue.
• isClosed() delegates to the parent — SPI surface only, as you say, and no in-tree caller. Left.
• Wall-clock time for TTL and deadlines — left; it is the clock the rest of this class already uses, and moving it is a change of its own.

Test runs

suite	result
CachedConnectionTestCase	32/32 (13 of #876 + 19)
PgSqlTestCase	54/54
MySqlTestCase	54/54

MsSqlTestCase and OracleTestCase are left to CI — nothing dialect-specific was touched.

Each of the four mechanisms was reverted and the suite run against the old behaviour, to check the new cases fail on it rather than passing either way:

testABorrowStopsAtItsDeadlineRatherThanDrainingThePool         the borrow drained the pool past its deadline: 6051 ms
testAnImportGivesItsConnectionBackWhenTheCommitFailsUnchecked  the import kept the connection of the pool expected [1] but found [0]
testTheStorageBorrowsFromThePoolItRegisteredWith               expected [...storage-registered] but found [...storage-changed]
testTheScheduledSweepClosesOnAThreadOfItsOwn                   the close ran on the test thread, not on the closer pool

[vharseko]

Rebased onto master — re-requesting review, since the approval predates the rebase

#876, #880 and #883 have landed, so this branch no longer stands on issues/872-jdbc-connect-timeout:
it is one commit on top of master, and the whole diff belongs to this PR. The stacked-on note at
the top of the description is replaced accordingly, so "review everything above a8fcec7886" no
longer applies.

The three commits are squashed into one. Their base was the first commit of #872, and master
carries the last — two rounds of review apart. Replaying them one by one would have meant resolving
the same hunks in CachedConnection three times, each a separate guess at the intent; one pass onto
the final baseline is one decision per hunk, and this PR merges squashed anyway. The force-push is
that rebase.

master grew the alive window of #879 and the read bound of #872 inside the very code this PR
replaces, so four things had to be decided rather than merged. The description spells them out; the
short form:

• pollIdle() is told whether the borrow trusts the alive window and asks isUsable() as master
  wrote it — the pool here and that window are one layer, not two.
• A connection now carries poolable (JDBC backend: the connection pool connects without any timeout and retries a failed connect forever #872), the proof of life (JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879) and the pool, meter and permit
  of this PR together. close() routes a connection that may not be pooled through Pool.destroy()
  rather than closing it, or the bound loses a place for good on every one of them.
• getValidatedConnection() and distrustPool() of JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879 re-read db-directory, where this PR had
  already established that the string open() registered with is the one to use. All three pool
  lookups go through poolKey() now — otherwise a db-directory changed on a running backend sent
  the validated borrow to a pool this storage never registered with.

One of the four is a fix to this PR, not to the merge. A nested borrow now takes a permit when the
pool has room, and goes on unmetered only when the pool is at its bound. Pool.held documents the
exemption as from the wait rather than from the pool, but the code exempted a nested borrow from
both — so a connection borrowed inside another was never pooled again, and the pool stopped handing
out the connection returned last. testTheConnectionReturnedLastIsBorrowedFirst of #883 failed on it;
that is how it surfaced.

One behaviour is deliberately not carried over: the validation of a pooled connection is no longer
clamped to what is left of the borrow. master bounds that validation at the socket instead, and the
pollIdle loop checks the deadline after every connection it discards, so a drain overruns by at most
the single validation it is inside — which is what master does today. If you want it back, it is a
commit of its own and I will add it.

CachedConnectionTestCase: 83/83, covering the cases of this PR together with the ones #872 and
#879 added to the same class. The engine suites have not been re-run since the rebase and are left to
CI.

Your approval of 24 Aug was given on 642729ffc3, which this rebase replaces, so I am asking for it
again rather than carrying it over a commit you have not seen.

[maximthomas]

Blocking

issue (blocking): close() loses a permit for good when rollback() throws unchecked.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:1806-1813

if (!returned.compareAndSet(false, true)) { return; }
if (owner == Thread.currentThread()) { pool.leave(); }
try { rollback(); } catch (SQLException e) { pool.destroy(this); throw e; }

There is no finally, and SQLException is the only catch. An unchecked throw out of parent.rollback() escapes after the CAS and after leave(), so the connection reaches neither give() nor destroy(). releasePermit() has exactly one caller — Pool.destroy() — and pools are never removed from the static map, so the bound drops by one for the life of the JVM; enough of them and every borrow fails with SQLTimeoutException. Give close() a finally with a handed-off flag so every exit that is not a successful give() goes through destroy().

Checked and clean: the retried close() returns at the CAS; pool and releasePermit are package-private and callers hold only java.sql.Connection, so no caller can compensate; #876's testConnectionThatCannotBeRolledBackIsClosed uses doThrow(new SQLException(...)), which exercises the catch, not the escape.

Non-blocking

issue (non-blocking): The bound is sized from a formula that is only a fallback.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:126

static final int DEFAULT_POOL_MAX = Math.max(16, Runtime.getRuntime().availableProcessors() * 2);

The comment justifies this as "sized like the worker thread pool ... Platform.computeNumberOfThreads(16, 2)". But that is the else branch of opendj-server-legacy/src/main/java/org/opends/server/api/WorkQueue.java:150-159:

if (configuredNumWorkerThreads != null) { return configuredNumWorkerThreads; }
int value = Platform.computeNumberOfThreads(16, 2.0f);

An 8-core server with ds-cfg-num-worker-threads tuned to 64 gets 64 borrowers and 16 permits; under sustained load the surplus waits out pool.timeout and fails, on a config the operator never touched. Derive the default from the configured value, or warn at startup when it exceeds the bound.

Checked and clean: the configured value reaches computeNumWorkerThreads from all four call sites (TraditionalWorkQueue:159/:623, ParallelWorkQueue:114/:431); nothing outside CachedConnection.java:122/:126 and the test class references POOL_MAX_PROPERTY or DEFAULT_POOL_MAX.

issue (non-blocking): A failed open() reports working() with its registration already released.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:222-234

try (final Connection con = getValidatedConnection()) {
    this.accessMode = accessMode;
    storageStatus = StorageStatus.working();
} catch (Exception e) { if (registeredHere) { releasePool(); } throw e; }

The status is set inside the resource block, so a throw from the implicit con.close() runs releasePool() (users 0, closed true) and rethrows with the status still working(). RootContainer.open wraps and rethrows without storage.close(), and both re-open guards (:853, :1922) test isWorking() — so no user is registered again, and Pool.give() destroys every returned connection. Pooling is off for that connection string, silently. Hoist the assignment out of the try-with-resources.

Checked and clean: no permit is leaked here — destroy() releases them; the loss is the pooling.

issue (non-blocking): catch (Exception) where the PR's own standard needs Throwable.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1940-1950 and :229

} catch (Exception e) {
    if (borrowed != null) { try { borrowed.close(); } catch (SQLException e2) {} }

In the ImporterImpl constructor the borrow is already handed off, and new WriteableTransactionTransactionImpl(borrowed) runs new StampSession() in a field initializer. An Error there — OOM in a bulk import — escapes, and nothing releases the permit. In open() a NoClassDefFoundError from driver static init escapes with the pool holding a user that never leaves. ImporterImpl.close() sixty lines below catches Throwable for exactly this reason. Widen both to match.

Checked and clean: the constructor's comment "nothing holds what this constructor takes until it returns" is why this is worse, not better — after construction close() compensates; inside it nothing does.

issue (non-blocking): liveCount() cannot see the connections that exceed the bound.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:409-412

/** The connections this pool holds, borrowed and idle together. */
int liveCount() { return max - permits.availablePermits(); }

It counts permits. A nested borrow at the bound goes unmetered — final boolean metered = pool.tryReserve() fails and, with reentrant true, the borrow falls through holding nothing. So the connections it misses are exactly the ones over the bound, and the eight assertions that rely on it (several == 0) pass with unmetered connections outstanding. Either correct the javadoc or count unmetered connections separately. This is the one deviation the description does not mention.

[vharseko]

Thanks — the blocking one holds up exactly as you traced it, and so do the three non-blocking ones. All four are fixed in a1bdc2f, together with what re-reading the branch against them turned up. The PR text is updated: the paragraphs in "Not in this" that your findings overtook are rewritten rather than left standing.

close() loses a permit for good when rollback() throws unchecked

Fixed. The hand-off is decided in a finally: whatever does not reach give() reaches destroy(), so the SQLException a rollback is supposed to throw, a connection that may not be pooled, and the unchecked failure a driver throws instead all leave through one place.

Your reachability holds up at every link — releasePermit() has the single caller, pool is private rather than package-private, so not even a same-package caller could compensate, and testConnectionThatCannotBeRolledBackIsClosed does exercise the catch and not the escape.

One thing to add to the accounting: the permit was not the whole of it. Nothing closed parent on that path either, so the escape leaked the socket as well — and with returned already true, a caller closing defensively a second time got the no-op, so there was no recovery from outside.

The flag is raised before give() rather than after, which is worth a word since it reads backwards. After give() the pool owns the connection: if something threw once it had reached the idle deque, a compensating destroy() would close a connection sitting there waiting to be handed out — and pollIdle may hand it out inside the alive window without asking the driver. Better to leak in that impossible corner than to hand out a closed connection.

ImporterImpl.releaseConnection() needed the same widening for the same reason, and this one is not a corner: its con.close() is that return, guarded by catch (SQLException) alone. An unchecked failure of it walked out with the commit's SQLException dropped — and in the catch (Throwable t) branch above, with the Error that branch exists to preserve replaced.

testAConnectionWhoseRollbackFailsUncheckedIsClosed covers it: rollback() throws IllegalStateException, the failure reaches the caller, parent.close() is verified, and the pool keeps neither the connection nor its permit.

The bound is sized from a formula that is only a fallback

Correct, and the comment that claimed otherwise is fixed: Platform.computeNumberOfThreads(16, 2) is the else branch, and ds-cfg-num-worker-threads replaces that count outright.

Deriving the default from the configured value is the one part I did not take. The bound belongs to a database that two backends may share; the worker count belongs to the server. A default that follows one of them would make the pool of a shared database a function of a setting that says nothing about that database, and CachedConnection would have to reach into the work queue to build a pool. So openPool() reports instead: when the worker threads outnumber the places, one line per connection string names both counts, the effective pool.timeout and both knobs.

Where I would push back a little is on the consequence. The surplus does not fail on the difference alone — it serializes, and an operation fails only if no connection comes back within pool.timeout. It is a latency cost that becomes a failure at the tail, not a broken configuration.

And the report is one difference rather than the whole demand, which it now says: the replay threads of replication default to that same computeNumberOfThreads(16, 2.0f) (MultimasterReplication:315) and borrow on top of the workers, so a replicated install at defaults has roughly twice the borrowers the warning can see. Counting them means reaching into the replication plugin for a private static with no accessor, which is a worse coupling than the one it would document.

A failed open() reports working() with its registration already released

Fixed, and the chain you traced is the chain: write() (:829) and the ImporterImpl constructor (:1917) both open only when the status does not already say working, so the pool kept no user and give() destroyed everything returned to it.

The assignment is hoisted out. The registration moved the other way — into the try — so that an open failing anywhere gives it back, not only one that failed in the borrow.

That move then needed a second flag, which is worth flagging as a thing you found by implication: releasePool() cannot answer for the CAS, only for openPool() having returned. A throw between the two (startSweeper() failing to make a thread) would take a user off a pool this storage never added one to — and on a shared database, the user of the other backend, draining connections it is still borrowing. That hazard predates this PR; it just used to fire later, out of close().

testAnOpenThatFailsOnTheReturnLeavesTheStorageClosed drives it: the return's rollback fails, open() reports it, the storage does not say working, and the next open() is not skipped.

catch (Exception) where the PR's own standard needs Throwable

Taken, both places. The constructor is the one that matters — you are right that the comment about nothing holding the borrow is why it is worse rather than better — and WriteableTransactionTransactionImpl does run new StampSession() in a field initializer (:1410), so the Error never reaches the cleanup. An Error is now rethrown as it is rather than wrapped in a StorageRuntimeException, and a close that fails while unwinding is addSuppressed rather than swallowed.

liveCount() cannot see the connections that exceed the bound

Right on all counts, including that it is the one deviation the description did not mention. Renamed meteredCount(), with a javadoc that says what it counts and why the connections it misses are exactly the ones over the bound. No separate counter: the assertions that read "kept a permit" want permits, and the name was the part that lied.

What re-reading turned up

Same family, all in the same commit.

• A cross-thread return left its borrower permanently reentrant. The depth came down only where the returning thread was the borrowing one, and owner was nulled either way — so a return on another thread left the borrower at a depth nothing could lower. That thread was then reentrant for the life of the server: exempt from the wait, opening an unmetered connection per operation that the return then closed. A physical connect apiece, past the bound. The connection carries the counter of its borrower now rather than the borrower itself, so the return lowers it wherever it happens. testAReturnOnAnotherThreadLowersTheDepthOfTheBorrower, and it fails on the old behaviour with the borrower thread was taken for a nested borrow and passed the bound of the pool.
• newStampConnection() was the last connection still following db-directory. Through poolKey() now. After a live change the data stayed with the registered database while the table comments went to the new one.
• The pool-full failure was throttled on one JVM-wide AtomicLong, twenty lines below a stall warning that is a map per connection string with a comment explaining why. Two backends full at once: the first to report silenced the one whose operations were failing. Same map now.
• The sweep interval was fixed at the first pool, although the TTL is read per borrow and per sweep so it can be changed on a running server. The sweep books its next run out of the one before it; the booking is in a finally, so a Throwable cannot stop expiry for the JVM the way a task thrown out of scheduleWithFixedDelay does.
• The sweep could close a connection a millisecond old. It decided from a reading taken during peekLast() and removed with a separate removeLastOccurrence(); a borrow that took the connection and returned it between the two left the decision standing on a stale reading. The reading is taken again after the removal.
• raise pool.max was advice that could not be taken. Read once per pool, and a pool is never removed from the map — the message says "and restart the server" now, and the property's javadoc says why.
• Dead weight. CachedConnection.invalidate() had no caller that ships; the drain is reachable through the pool and the tests call that. The public wrapper constructor passes poolable=false, which is what the accounting made of it anyway. Two comments the removal of Caffeine left behind — a removalListener that no longer exists, and a TTL described as read once — now describe the code as it is.

Left alone, with the reason

• returned is reset by borrowed(), so a close() arriving after the connection has been re-borrowed is not a no-op. True, but it needs a caller that closes a connection it has already handed back — one that is broken independently. Every pool has this exposure and none guards it.
• The pool-full SQLTimeoutException carries no SQLState. The inconsistency with the connect-timeout beside it is real, but 08001 would be a false statement: no connect was attempted. Leaving it null beats naming the wrong thing.
• Executors.newCachedThreadPool() for the closes is unbounded. That is the trade this PR argued for and I would keep it: one shared thread only moves the head of the line, which is the failure the hand-off exists to avoid.

Test runs

suite	result
CachedConnectionTestCase	86/86

Three cases added. The depth one was run against the mechanism reverted and fails there on its own assertion, so it is not passing either way.

The engine suites have not been re-run since the rebase and are left to CI.

[vharseko]

@maximthomas #877 (#882) has landed and this branch is merged with it, 43968b7. One file conflicts, JDBCStorage.java, and the whole of it is one question: both branches moved the borrow of an import, in different directions. The description carries the full account; here is what is worth attacking.

git flagged the wrong half

#882 borrows in startImport() and gives the connection back through a finally when the importer cannot be built. This branch moved the open and the borrow into ImporterImpl(), so that one owner gives back what one owner took.

The merge marked only the constructor. It had auto-merged startImport() to master's version — the one that borrows before building the importer — so taking the conflicted side alone would have borrowed twice, once in startImport() and once in the constructor, and leaked one of the two on every import. No marker, no compile error, and the unit suite would not have caught it: both borrows go through the same stand-in.

Resolved by keeping this branch's design and collapsing startImport() to return new ImporterImpl();.

What of #882 comes with it

• Both transactions keep StatementBound.BULK — the contract that every statement of an import is bulk by construction. This branch predates it and carried neither.
• The ReadOnlyStorageException stays, and stays where you put it — where the importer is built. It moved inside the try rather than in front of it: with the open now in the same constructor, a refusal in front would leave a storage that constructor had just opened with nobody to close it.
• One seam — getConnection(boolean trusted) — now going through poolKey(). Your seam so a stand-in intercepts every path, this branch's key so a db-directory changed on a running backend cannot send the paths to different pools. They compose; neither side's line carried both.

One behaviour changed, and it is not cosmetic

The refusal now stands in front of the borrow, so an import of a read-only storage takes no connection at all rather than taking one and returning it.

That makes two of your tests describe something that no longer happens. testStartImportGivesTheConnectionBackWhenTheImporterCannotBeBuilt and testStartImportClosesTheStorageItOpenedWhenTheImporterCannotBeBuilt both pinned verify(con).close(). They pin borrows == 0 and verify(con, never()).close() now — the same leak, covered at the state that cannot reach it. The second still pins the storage: the constructor opened it, so the refusal gives it back, and closes == 1 is unchanged.

I am flagging this rather than presenting it as a straight merge: it walks back the assertion of a test you asked for, and if you would rather the refusal sat behind the borrow so that the original assertions stand as written, say so.

testEveryStatementOfAnImportIsBulk builds its importer through the borrow seam instead of handing a connection to the constructor, on the same physical connection the entry read beside it runs on — the backstop is keyed on the connection rather than on the storage, which is what that case was always about.

State

mvn -pl opendj-server-legacy test-compile is green. JDBCStatementBoundTestCase 37/37, CachedConnectionTestCase 86/86, JDBCStorageRetryTest 66/66, StampConnectionTestCase 5/5, BulkCursorTest 12/12, PersistentCompressedSchemaTest 8/8 — 214/214. The four engine suites have not been re-run since this merge; CI is running them now.

The five points of your last round are answered in a1bdc2f, which is behind this merge and unchanged by it. Re-requesting review.