JDBC connection pool is unbounded and its TTL never closes an idle connection under traffic

[vharseko]

Describe the bug

The pool of the JDBC backend has no upper bound on the number of connections it holds, and the TTL that is supposed to close the idle ones does not do what its name says.

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:38-51
static LoadingCache<String, BlockingQueue<CachedConnection>> cached = Caffeine.newBuilder()
    .expireAfterAccess(Duration.ofMillis(getCacheTtlMillis()))
    .removalListener((String key, BlockingQueue<CachedConnection> value, RemovalCause cause) -> {
        for (CachedConnection con : value) {
            try {
                if (!con.isClosed()) {
                    con.parent.close();
                }
            } catch (SQLException e) {
                // ignore
            }
        }
    })
    .build(conStr -> new LinkedBlockingQueue<>());

Four things follow from that shape.

1. No upper bound. The queue is an unbounded LinkedBlockingQueue and the cache has no maximumSize, so nothing in OpenDJ limits how many connections a backend opens: as many as there are threads asking at once. The only ceiling is the max_connections of the database itself — which the backend then treats as a retry condition, so a burst of concurrent operations turns into a burst of connect attempts against a server already at its limit.

2. The TTL sits on the URL, not on the connection. expireAfterAccess is keyed by the connection string, and the key is touched by every borrow (getConnection) and every return (close()). Under continuous traffic the entry never expires, so the connections in the queue never expire either: a burst that opened 200 connections keeps all 200 open for as long as the backend sees any traffic at all. The documented behaviour of org.openidentityplatform.opendj.jdbc.ttl — "the time after which an idle pooled connection is closed" — only holds when the whole backend is idle, not when a connection is.

3. Expiry is lazy, and the case it is for is the one it misses. The cache is built without a scheduler(), so Caffeine expires an entry during a later cache operation. On a backend that has gone completely idle — the only situation in which the entry can expire at all, per (2) — there is no later operation to trigger it, so the connections stay open on the database side until its own idle reaper takes them.

4. A closed backend does not release its connections. JDBCStorage.close() only flips the status:

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:112-114
public void close() {
    storageStatus = StorageStatus.lockedDown(LocalizableMessage.raw("closed"));
}

The pool entry for its URL is left alone, so disabling or removing a JDBC backend leaves its connections open, and by (3) possibly indefinitely.

There is also a narrow race in the return path: close() does cached.get(connectionString).add(this), and if the entry is evicted between the get and the add, the connection lands in a queue nothing refers to any more — never handed out again, never closed.

Impact

A deployment sized for its steady-state connection count can hold its peak count open indefinitely, and a database shared with other applications sees a backend that never gives connections back. Restarting or disabling a backend does not release them either. On a database with a modest max_connections, the unbounded pool is also what makes the "too many connections" path reachable in the first place.

Expected behavior

• an upper bound on the pooled connections of one backend, configurable, with a borrow above it waiting for a returned connection rather than opening another (the deadline of org.openidentityplatform.opendj.jdbc.pool.timeout added in [#872] Bound the connect of the JDBC pool and report a connect it cannot make #876 already covers the wait);
• the TTL applied per connection — closing one that has been idle for it — rather than to the pool as a whole;
• expiry that happens without a subsequent borrow, so an idle backend really does release its connections;
• JDBCStorage.close() releasing the pool of its connection string;
• the return path leaving no connection in an evicted queue.

Environment

master (5.2.x), all four JDBC dialects. The line numbers above are of master as it stands; #876 rewrites getConnection but touches none of the cache shape.

Noticed while fixing #872 (PR #876).

[vharseko]

Went through the pool on master; all four points and the race hold. Adding the evidence, one constraint that has to shape any maximum-size design, and three leaks the report does not cover.

Confirmed

No upper bound. Worth stating more precisely: the unbounded queue is not itself what opens the connections. getConnection establishes a new one whenever the queue comes back empty, and no counter of live connections exists anywhere — the queue's unboundedness only governs how many idle ones are retained. The peak is therefore the number of threads borrowing at once, which is the worker threads: WorkQueue.computeNumWorkerThreads falls back to Platform.computeNumberOfThreads(16, 2.0f), so max(16, 2 x CPU) by default — 64 on a 32-core box — plus the import and administrative paths.

The TTL sits on the URL. expireAfterAccess is reset by any read of the entry, and cached.get(connectionString) is called both on the borrow and on the return (close() does cached.get(connectionString).add(this)).

Expiry is lazy — no scheduler() on the builder — and a closed backend does not release its pool: JDBCStorage.close() only flips storageStatus.

The return-path race is real: cached.get(...) and .add(this) are two separate operations.

The constraint a maximum has to respect: a borrow can nest

PersistentCompressedSchema.store() opens a storage.write(...) of its own rather than writing through the surrounding transaction — the definition has to commit independently of the entry. On two paths that happens while a transaction is already open, so one thread holds two connections at once:

• EntryContainer.importEntry (EntryContainer.java:1623) calls id2entry.encode inside the transaction. This is exactly the chain spelled out by the comment in JEStorage.java:353-364. On top of that ImporterImpl holds a connection of its own for the whole import (JDBCStorage.java:708).
• EntryContainer.modifyDN (EntryContainer.java:2165) uses the three-argument id2entry.put(txn, id, entry), which also encodes inside the transaction.

addEntry (EntryContainer.java:1521) and modifyEntry encode before storage.write, so an ordinary add or modify does not nest — but a subtree rename and an import do.

So a hard maximum with no reentrancy in its accounting deadlocks. Either the accounting is per thread — a thread already holding a connection may exceed the bound — or the bound has to be at least twice the worker thread count. Mitigating: with the borrow deadline of #876 in place a deadlock degrades to a pool.timeout stall and a failed operation rather than a permanent hang.

Three leaks not in the report

1. A connection whose rollback() fails is neither pooled nor closed.

public void close() throws SQLException {
    rollback();                              // throws -> the line below never runs
    cached.get(connectionString).add(this);
}

The CachedConnection is dropped with parent still open. This catches precisely the situation where connections matter: a database restart, a session killed by an administrator, a server-side idle reaper firing — every live connection fails its rollback at once.

2. The race is wider than the return path. Any cached.get() may hand back a fresh queue instance, and getConnection calls it more than once per borrow (in #876, inside the poll() loop), so the pool has no stable identity to synchronize on. From the other side: the removal listener walks the queue with LinkedBlockingQueue's weakly consistent iterator, so a connection added while the entry is being evicted is not seen by the listener either — the same leak from the opposite end.

3. Releasing on backend close is not enough. cached is static, so an in-process shutdown of the whole server — EmbeddedDirectoryServer, and the test suites — leaves the connections open as well. And whatever releases the pool has to decide what happens to the connections borrowed at that moment: they are not in the queue, and they will be returned into a queue created after the release.

Two smaller notes

• The TTL is read once. cached is a static field and getCacheTtlMillis() runs in its initializer, so org.openidentityplatform.opendj.jdbc.ttl has to be set before the class loads and cannot change at runtime — unlike the timeouts of [#872] Bound the connect of the JDBC pool and report a connect it cannot make #876, which are read on every borrow.
• The removal listener closes connections on ForkJoinPool.commonPool, Caffeine's default executor: a burst of blocking network closes lands on a pool the rest of the JVM shares.

Ordering against #876

#876 already covers the neighbouring holes — isValid(0) bounded to 5s, per-dialect connect and login bounds, a deadline over the whole borrow, and a retry restricted to "the server accepts no further connection" instead of blind recursion — and touches none of the cache shape, as the report says.

The dependency runs both ways, though: the bound asked for here is what makes the retry path of #876 nearly unreachable, since threads would wait on the queue instead of hammering a database already at max_connections. So #876 first, this on top of it — otherwise the two collide in getConnection.

Shape of a fix

A semaphore over the connections of one connection string, with reentrant per-thread accounting; a queue that records when each connection was returned, so the borrow can discard one that has been idle longer than the TTL; a background sweep (or Caffeine.scheduler(Scheduler.systemScheduler())) so that an idle backend really does give its connections back; JDBCStorage.close() invalidating the entry for its URL; a return that goes through one stable queue reference rather than get + add; and a close() that closes the connection when its rollback fails instead of dropping it.

Pulling in HikariCP for this looks like the wrong trade — it is on the order of a hundred lines here, against a dependency that would have to go through the build and a licence review.