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

[vharseko]

Fixes #878

Stacked on #876. The branch sits on issues/872-jdbc-connect-timeout, which is not a branch of this repository, so the base here is master and the diff carries the commit of #876 until that one merges — GitHub narrows it to the commits of this PR by itself once it does. Review everything above a8fcec7886. The two touch the same method: #876 rewrites getConnection to bound the connect, this one gives it a pool to borrow from, and landing them apart would mean merging two rewrites of it by hand. The branch of #876 has moved on since (it now carries #879), so this one is rebased onto its head once that PR settles.

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.

pool.max is read when a pool is built rather than at every use, unlike connect.timeout, pool.timeout and ttl, and the sweep interval is derived from the TTL in force when the first pool is created. Neither of the four properties is documented anywhere outside the source. DEFAULT_POOL_MAX counts the worker threads only, while the replay threads of replication and the import, backup and admin paths borrow too; picking a different default is 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 liveCount() == 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)
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.

[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