Drop empty pool partitions and string-free composite keys - #2390

Summary

Two costs of the HTTP/1 connection pool that grow with how many hosts a client has ever talked to, or that every request pays when it goes through a proxy or uses a virtual host.

1. The pool now drops partitions that stay empty. DefaultChannelPool added a partition (a ConcurrentLinkedDeque) for every key it was offered and only removed one in flushPartitions. A client that talked to many hosts once kept all their empty deques for good, and the idle cleaner walked every one of them on each tick (connectionPoolCleanerPeriod, 100 ms by default).

• The cleaner now drops a partition once it has found it empty at every tick for as long as an idle channel may stay pooled: pooledConnectionIdleTimeout, or connectionTtl when the idle timeout is off. A partition is a subclass of the deque with a retired flag and the key, so offer/poll pay no extra indirection.
• An offer may still hold the partition it read before the removal. The remover unmaps the partition, then raises the retired flag, then drains (the cleaner) or scans (a flush). offer reads the flag after linking its channel. Either the offer sees the flag, or the remover sees the channel. Nothing that lands in an unmapped partition is moved. It is closed by the cleaner's drain, by the flush, or by the offer that takes its node back, after the usual ownership claim. Moving channels between partitions was tried first. It reopened races: a channel was briefly in no deque, and a channel pooled before a flush could outlive the flush. Closing costs a reconnect. That happens only when an offer lands in the moment the cleaner drops a partition that has been empty for the whole idle timeout.
• Whoever unmaps a partition owns what is left in it, so the cleaner and a flush never both drain one. flushPartition now uses remove(key, partition). The old remove(key) could unmap a newer partition for the same key than the one it was closing.
• This also fixes a leak that predates this change. An offer racing flushPartitions could link its channel into the deque the flush had just unmapped. The channel then sat there alive, and nothing ever leased, reaped or closed it again.

The default connectionPoolCleanerPeriod is unchanged. PT0.1S has been the default since #1205 (2016), which added the setting so that idle connections are not all closed at once.

2. CompositePartitionKey no longer builds a string per lookup. With a proxy or a virtual host, PerHostChannelPoolPartitioning.getPartitionKey built Uri.getBaseUrl(), a fresh scheme://host:port string, only to hash and compare it. It then hashed the key through Objects.hash, which allocates a varargs array and boxes the proxy port. A request computes the key at least twice: once for its future, and once from the live request to poll the pool. The key now holds the scheme, host and port, as the plain PartitionKey already does, and hashes them inline. Keys are equal exactly when the base URL strings were equal, and toString prints the same text as before.

No public API change. The new constructor, Partition and retire are package-private.

Benchmarks

JMH 1.37, JDK 17.0.2, -f 2 -wi 3 -i 5 -prof gc, upstream main (d722291) against this branch (3817fa6). Both benchmarks are in client/src/jmh.

ChannelPoolCleanerBenchmark: one cleaner pass with N dormant partitions (empty for longer than the idle timeout) and 16 live ones, each leased and returned per invocation. cycle is that lease and return alone, so cleanerPass - cycle is the pass.

A pass allocates next to nothing on either side. What remains at 10,000 is the map's table: ConcurrentHashMap keeps the capacity it grew to, and the pass still walks the empty bins.

PartitionKeyLookupBenchmark: build the key, then hash and compare it against an equal key in a ConcurrentHashMap.

Throughput numbers come from a shared, loaded machine. The B/op figures are the stable ones.

Tests

• DefaultChannelPoolTest:
  • Deterministic tests: dropping an empty partition; keeping a non-empty one; the default lifetime, and how it is chosen from the idle timeout and the TTL; both sides of the offer race (a stale read is simulated by mapping the retired partition back); a claimed channel is left to its owner; the cleaner leaves alone a partition a flush unmapped first; an offer racing a flush has its channel closed rather than stranded.
  • Two soak tests (no Thread.sleep, bounded by a deadline): offers and polls against a cleaner that drops every empty partition, and against back-to-back flushes. At quiescence every channel must be pooled or closed.
  • Mutation runs confirmed that the tests fail without each part of the protocol: the flag read in link, the drain in retire, the close in link, the ownership check in retire, and retirement itself.
• ChannelPoolPartitioningTest: key equality across equivalent URIs, every component separates keys, the secured proxy port, explicit default ports, and the toString format.

mvnw.cmd -B -ntp clean verify -Dgpg.skip=true on JDK 17.0.2 at 3817fa6: BUILD SUCCESS, 2050 tests, 0 failures, 35 skipped, Revapi clean. JDK 11 is not installed on this machine; CI covers it.

Second-agent review

Each round ran read-only in fresh sessions on the full diff against upstream main.

• Guided reviewer: Codex gpt-6-astra, reasoning effort medium, with AGENTS.md as the checklist.
• Independent reviewer: Claude claude-opus-5-5, effort medium, with Read/Grep/Glob only.

Every finding was fixed except these, which were declined on purpose:

• Splitting the key change into its own PR (round 4). The two changes were scoped together on purpose.
• Wall-clock time for emptySince (rounds 3 and 6). It is the clock the idle-timeout check already uses, and a clock step only changes when a partition is dropped.
• destroy() during a cleaner pass (round 2). The same race already exists for offer, and ChannelManager closes all channels before destroy().
• Closing, rather than moving, the channel of an offer that races a partition drop (round 6). See above.
• The soak tests not proving that the race window was hit (rounds 4 and 8). A deterministic test covers each branch. In every mutation run, the soaks failed without the flag check in link and without the drain in retire. An assertion that the window was hit would depend on scheduling, which round 2 asked to avoid.

Claude Code on behalf of Pavel Ptashyts

🤖 Generated with Claude Code