Fix idle pool cleaner generation race by pavel-ptashyts · Pull Request #2289 · AsyncHttpClient/async-http-client

pavel-ptashyts · 2026-07-25T13:32:49Z

Summary

claim the exact idle generation evaluated by the channel pool cleaner
keep generation and ownership in one atomic state without restoring per-offer allocations
cover stale close, fresh entry loss, and equal-timestamp generations with regression tests

Testing

./mvnw -pl client -Dtest=DefaultChannelPoolTest test (19 tests passed)
full JDK 11 ./mvnw clean verify attempt: 1,326 tests passed and one unrelated timing assertion failed in SemaphoreTest.combinedCheckAcquireTime
immediate isolated SemaphoreTest rerun passed (10/10)

Codex on behalf of Pavel Ptashyts

Claim the exact idle generation evaluated by the cleaner so a lease and re-offer cannot turn a stale expiry decision into a close of the fresh entry. Keep generation and ownership in one CAS-able state to preserve the per-channel allocation model and avoid hiding a newer entry from poll. Fixes AsyncHttpClient#2284 Codex on behalf of Pavel Ptashyts Co-Authored-By: Codex <codex@openai.com>

hyperxpro · 2026-07-25T17:39:08Z

-        /** Stamp the idle start and mark the channel leasable again. Called on every offer. */
+        /** Stamp a new idle generation and mark the channel leasable again. Called on every offer. */
        void reset(long now) {
+            long nextGeneration = (state & ~OWNED_MASK) + 2;


reset() needs its precondition written down (DefaultChannelPool.java:337). The non-atomic RMW of state is safe only while reset() never runs on an unowned state. If it does: reset() reads G unowned → cleaner CAS(G, G|1) succeeds → reset()'s plain stores clobber the claim → cleaner closes the channel while offerFirst publishes it as leasable. Unreachable in production today (every path goes through

ChannelManager.tryToOfferChannelToPool, which setDiscards first, and poll() only returns after a successful claim), but ChannelPool is a public SPI that documents no such rule, and the PR removed the paragraph that documented the old design's equivalent invariant. Either restore an explicit invariant paragraph, or make reset an ownership-transfer CAS (compareAndSet(s, (s & ~OWNED_MASK) + 2) on an isOwned(s) snapshot) so
IdleState is safe under any caller. Worth noting concurrentOfferPollRemoveAllIsConsistent already offers without lease discipline — it only survives because it runs with the cleaner effectively disabled.

Agreed. I made reset an ownership transfer instead of a non-atomic update. IdleState now starts owned, reset rejects a leasable generation, publishes the timestamp, and CASes the owned snapshot to the next generation. offer also installs the per-channel state with setIfAbsent and returns false when the transfer fails. I added a regression test for duplicate offers.

hyperxpro · 2026-07-25T17:59:50Z

-     * the channel and {@code poll()} unlinks a channel from the deque before claiming it. {@code start}
-     * doubles as a generation token: it changes on every offer, letting the cleaner detect a channel
-     * that was leased and re-offered between its expiry check and its claim.
+     * <p>The low bit of {@code state} is the ownership flag; the remaining bits are incremented on


The remaining bits are incremented by two on every offer, not by one, since bit zero is the ownership flag. Worth saying explicitly so the plus two in reset is not read as a typo.

Updated the description to say that the generation bits advance by two on every offer because bit zero is reserved for ownership.

hyperxpro · 2026-07-25T17:59:51Z

+        }
+
+        /** Atomically claim the current generation. */
        boolean takeOwnership() {


This overload pair is easy to misread at a call site, because the no argument form silently re reads the field while the one argument form validates a caller supplied snapshot, and both are named takeOwnership. The tryTakeOwnership name suggested in the issue for the snapshot form reads better and makes the failure mode obvious.

Renamed the snapshot form to tryTakeOwnership. The no-argument takeOwnership method now clearly delegates to the conditional snapshot operation.

hyperxpro · 2026-07-25T17:59:52Z

+        /** Stamp a new idle generation and mark the channel leasable again. Called on every offer. */
        void reset(long now) {
+            long nextGeneration = (state & ~OWNED_MASK) + 2;
+            // Keep the new generation unavailable until its timestamp is published.


This comment does not describe what the store actually does. The JMM already keeps the new generation from being observed with a stale timestamp without this store. Volatile accesses are totally ordered in the synchronization order, per JLS 17.4.4 and 17.4.7, so a reader that observes state equal to nextGeneration must also observe start as now or newer, because the store to start precedes the store to state in program order.

What this extra store really does is narrow, but not close, the window described in the comment above on the generation read, and it costs an extra StoreLoad fence per pooled request on x86 64. Either drop it, or keep it and relabel it honestly as defence in depth.

Removed the extra volatile state store. reset now writes the timestamp and then CASes the owned snapshot directly to the next leasable generation; synchronized reset calls prevent concurrent publishers from corrupting the winning timestamp.

hyperxpro · 2026-07-25T17:59:53Z

+    }
+
+    @Test
+    public void staleCleanerClaimDoesNotHideReofferedChannelFromPoll() throws Exception {


This test never runs the cleaner. It calls takeOwnership with a stale snapshot directly and then asserts that a failed CAS had no side effect, which restates the contract of compareAndSet rather than exercising the pool. Race b from the issue therefore has no end to end coverage.

I would fold it into the BlockingActiveChannel test and assert that the partition still holds one entry after the cleaner pass, since that is the assertion that would actually catch the cleaner hiding the entry.

Related gap: the soak test concurrentOfferPollRemoveAllIsConsistent runs with idle timeout off and a one hour TTL, so the cleaner only ever takes the isOwned then remove branch and never claims or closes anything.

Removed the standalone CAS-contract test and folded the observable guarantee into the BlockingActiveChannel test. It now asserts that the partition still contains exactly one fresh entry after the cleaner pass, before polling it. I also enabled idle expiry in the soak test and added a dedicated expiring channel with a close latch, so the test proves that the cleaner ownership and close path actually runs.

hyperxpro · 2026-07-25T17:59:54Z

+
+        @Override
+        public boolean isActive() {
+            AtomicBoolean block = blockNextActiveCheck;


This null guard is load bearing and deserves a one line comment saying why. EmbeddedChannel's constructor calls the subclass isActive once while these fields are still null, so without the guard you get an NPE. The trap is that the NPE is swallowed by a DefaultPromise listener, so removing this check would fail silently rather than loudly, and an IDE will flag the check as always true on a final field and invite someone to delete it.

Added the one-line constructor-dispatch explanation above the null guard so the EmbeddedChannel initialization trap is explicit.

Make pool offers transfer an owned idle generation with CAS so duplicate or concurrent offers cannot reset a leasable generation with a non-atomic update. Clarify generation encoding and snapshot claim naming, and exercise the cleaner retention and close paths end to end. Refs AsyncHttpClient#2284 Codex on behalf of Pavel Ptashyts Co-Authored-By: Codex <codex@openai.com>

hyperxpro · 2026-07-25T21:55:59Z

@@ -132,16 +132,18 @@ private boolean offer0(Channel channel, Object partitionKey, long now) {
        if (partition == null) {
            partition = partitions.computeIfAbsent(partitionKey, pk -> new ConcurrentLinkedDeque<>());


Minor one. This runs before the reset guard now, so a rejected offer leaves an empty deque sitting in partitions and the cleaner walks it every tick. Was not possible before, since offer0 could not return false. Probably just move the lookup below the reset call.

hyperxpro · 2026-07-25T21:55:59Z

+            IdleState existingIdleState = idleStateAttribute.setIfAbsent(newIdleState);
+            idleState = existingIdleState == null ? newIdleState : existingIdleState;
+        }
+        if (!idleState.reset(now)) {


Heads up that false here means the caller closes the channel. ChannelManager line 426 calls closeChannel on a false return, and that is really what false has always meant here, NoopChannelPool returns false every time exactly so nothing gets pooled.

So a duplicate offer now kills a live pooled channel. I tried it: second offer returns false, the close leaves it inactive but still in the deque, and the next poll on that partition comes back null. Nothing in tree can actually hit this since setDiscard runs first, but it does turn a harmless no op into quietly dropping a good keep alive.

Might be simpler to treat a duplicate as a no op, debug log and return true.

hyperxpro · 2026-07-25T21:55:59Z

+     * generation whose expiry it evaluated, without temporarily claiming a newer entry while checking
+     * whether {@code start} changed.
+     *
+     * <p>A generation can be reset only while owned. New state starts owned, and {@code poll()} claims


Tiny thing, this says two producers of owned state but there are four. The removeAll tombstone and the cleaner's pre close claim leave it owned too, and offer will reset from either. That part is pre existing and harmless since poll drops inactive channels, it just reads like a complete list.

Separately, the guard returns false with nothing logged, so if the invariant ever does break, all you would see is a connection closing for no apparent reason.

hyperxpro · 2026-07-25T21:55:59Z

-        /** Stamp the idle start and mark the channel leasable again. Called on every offer. */
-        void reset(long now) {
+        /** Transfer an owned generation to a new leasable generation. Called on every offer. */
+        synchronized boolean reset(long now) {


I do not think the synchronized is buying what it looks like it is buying. If a duplicate offer loses the race it parks here, and when it wakes up it re reads state fresh. So if a poll grabbed ownership in the meantime, the loser sails straight through the isOwned check and stamps a timestamp that is now well out of date, onto a generation that is not its own.

Walking through it. reset(1000) publishes gen 2 with start 1000, poll takes gen 2, a second thread calls reset(1000) and parks. First thread offers again, so reset(5000) publishes gen 4 with start 5000, and poll takes gen 4. Now the parked thread wakes, sees gen 4 is owned, passes the check, writes start back to 1000, and moves gen 4 to gen 6 leasable. Idle clock jumps back four seconds, and the entry goes leasable via a thread that never owned it. I did reproduce this.

With a plain CAS and no monitor the loser just fails and nothing happens. The lock is what turns that into a quiet wrong answer.

hyperxpro · 2026-07-25T21:55:59Z

+            long nextGeneration = (stateSnapshot & ~OWNED_MASK) + 2;
            start = now;
-            owned = 0;
+            return STATE_UPDATER.compareAndSet(this, stateSnapshot, nextGeneration);


Small thing, this CAS cannot fail. Only reset takes state out of owned and the monitor serializes those, and tryTakeOwnership needs an unowned snapshot so it can never match. I ran four threads at it for eight seconds and got 58 million resets with zero CAS failures, so the boolean is always true unless the guard above catches it.

The lock is not free though. On JDK 25 a take plus reset cycle went from 20.7 ns to 52.9 ns, roughly 22 ns of that being the monitor, which more than eats the fence you saved by dropping the intermediate store.

Guard, then start = now, then a plain volatile store of the next generation gets you the same thing for two stores.

hyperxpro · 2026-07-25T21:55:59Z

+        cleanerRun.get(5, TimeUnit.SECONDS);
+
+        assertTrue(channel.isActive(), "cleaner must not close a freshly re-offered channel");
+        assertEquals(1, partitionSize(pool, KEY), "cleaner must not unlink the fresh generation");


I do not think this one can fail. There is only one node in the deque, poll unlinks it, and the re offer makes a fresh node the cleaner's cursor never reaches, so the iterator remove is a no op on something already unlinked. I checked by making the tryTakeOwnership failure branch call it.remove as well, and the test still passed.

If you want it to actually bite, put a second untouched channel ahead of the blocked one so the iterator is still on a live node when it resumes.

hyperxpro requested changes Jul 25, 2026

View reviewed changes

pavel-ptashyts requested a review from hyperxpro July 25, 2026 18:34

hyperxpro requested changes Jul 25, 2026

View reviewed changes

		@@ -132,16 +132,18 @@ private boolean offer0(Channel channel, Object partitionKey, long now) {
		if (partition == null) {
		partition = partitions.computeIfAbsent(partitionKey, pk -> new ConcurrentLinkedDeque<>());

Uh oh!

Conversation

pavel-ptashyts commented Jul 25, 2026

Summary

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants