DefaultChannelPool cleaner can close a freshly re-offered channel

[rampreeth]

Summary

In 3.0.11, DefaultChannelPool can close a channel that was leased and then freshly returned to the pool while the idle-channel cleaner was inspecting it.

The cleaner calculates that the channel is idle-expired using the old idle timestamp. If the channel is leased and re-offered before the cleaner claims it, offer() resets the shared IdleState with a fresh timestamp.

However, the cleaner captures startSnapshot only after calculating the expiry result. It can therefore capture the new timestamp, successfully claim the new idle generation, and close it using the expiry result calculated from the old generation.

Version

Reproduced against the official async-http-client-project-3.0.11 tag.

The same deterministic reproduction also fails on current main as of f01188629 (3.0.12-SNAPSHOT).

The relevant shared IdleState behavior was introduced by #2183. The same reproduction does not occur with the per-offer idle holder used in 3.0.10.

Problematic ordering

The current cleaner flow is effectively:

boolean isIdleTimeoutExpired = isIdleTimeoutExpired(idleState, now);

// Other checks can allow the channel to be leased and re-offered here.

long startSnapshot = idleState.start();

if (!idleState.takeOwnership()) {
    continue;
}

if (idleState.start() != startSnapshot) {
    idleState.releaseOwnership();
    continue;
}

close(channel);

The expiry decision and the ownership validation can refer to different idle generations.

Deterministic reproduction

I created a unit test that forces this interleaving:

1. Offer a channel and allow its idle timeout to expire.
2. Start the cleaner.
3. Pause the cleaner after it has calculated that the old idle generation is expired.
4. Poll the channel and offer it back, resetting its IdleState.
5. Resume the cleaner.
6. Verify that the freshly re-offered channel remains active and leasable.

On unmodified 3.0.11, the assertion fails. The cleaner closes the channel and logs:

isIdleTimeoutExpired=true isRemotelyClosed=false isTtlExpired=false

Why reordering the timestamp read is not sufficient

Moving startSnapshot before the expiry calculation prevents the stale close in the reproduction above, but it still leaves a smaller race:

1. The cleaner evaluates old generation A.
2. The channel is leased and re-offered as fresh generation B.
3. The cleaner claims generation B.
4. Before the cleaner notices the generation changed and releases the claim, another poll() removes B from the deque but cannot take ownership.
5. The cleaner releases ownership, leaving the active channel absent from the pool.

I reproduced this second interleaving with a deterministic test that pauses the cleaner immediately after the claim. This is a fix-safety concern: the observed 3.0.11 failure is the stale close described above, while this interleaving shows why a timestamp-only change would not be a complete fix.

Fix requirement

The cleaner needs to atomically claim the exact idle generation that it evaluated. If the channel has been leased and re-offered since the snapshot was taken, the claim must fail without temporarily owning the fresh generation.

Conceptually (pseudocode, not a proposed public API), the cleaner flow should be:

IdleSnapshot snapshot = idleState.snapshot();

if (!isExpired(snapshot, channel, now)) {
    continue;
}

if (!idleState.tryTakeOwnership(snapshot)) {
    continue;
}

close(channel);

tryTakeOwnership(snapshot) must use a compare-and-set operation that succeeds only when the state is still the same unowned generation represented by snapshot. One possible implementation is a stamped atomic state containing both the generation identity and ownership flag.

Another option is to restore a distinct per-offer idle holder, where the timestamp and ownership flag belong to one deque entry. That is simpler to reason about and avoids sharing ownership across idle generations, but it restores the per-offer allocation that #2183 was intended to remove.

The exact representation is open for discussion; the correctness requirement is that generation validation and ownership acquisition form one atomic transition.

Regression coverage

A fix should include deterministic tests for both cases:

1. A stale idle-expiry decision must not close a channel that was leased and re-offered.
2. A poll racing with the cleaner's generation validation must not leave a live, freshly re-offered channel absent from the pool.

I’m happy to contribute the implementation and regression tests once the preferred state representation is clear.