Make idle-channel pool cleaner reap in a single O(n) pass - #2225
Merged
hyperxpro merged 3 commits intoJul 18, 2026
Merged
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
DefaultChannelPool.IdleChannelDetector.reapPartition collected drop-worthy channels into a list and then called ConcurrentLinkedDeque.removeAll(list). removeAll re-walks all n nodes doing an O(m) list contains() per node — O(n*m), degenerating toward O(n^2) when a whole partition is dropped in one tick (a load spike's connections idling out as a wave, or a peer dropping many keep-alives at once). This runs on the shared HashedWheelTimer thread, so a slow tick also delays request-timeout firing for every pool sharing that timer. Unlink drop-worthy channels in place through the iterator (it.remove()) during the single scan instead, keeping the whole pass O(n). Behavior is unchanged: the same channels are closed/unlinked and healthy ones kept; only the removal mechanism differs. Drop the now-unused lazyAdd helper and the List/ArrayList imports. Add white-box tests covering many-channels-in-one-tick reaping (idle-expired and tombstones) and a mixed expired-plus-healthy pass that must remove some nodes while keeping others leasable.
hyperxpro
approved these changes
Jul 18, 2026
hyperxpro
added a commit
that referenced
this pull request
Jul 18, 2026
…coverage Follow-up to #2225. Two test-quality fixes for the O(n) iterator-remove reap: cleanerReapsExpiredButKeepsHealthyInSameTick used a 200ms idle window with the fresh channels offered immediately before firing the cleaner, so a GC/scheduling pause on a loaded CI box could age them past the timeout and get them wrongly reaped. Widen the window to 1s (mirrors channelReofferedAfterExpiryIsNotReaped) so only a pause longer than the window can misclassify a fresh channel. Add cleanerContinuesPastRemovedNodesToReachKeptNodes. Because offer() is offerFirst and that mixed test's fresh channels cluster at the front, every reaped node sat at the tail, so the scan never had to unlink a node and then advance to a KEPT node after it. The new test builds an alternating closed/open deque through the remote-close path (channels are closed, not aged, so it is fully deterministic) so each removed node is followed by a kept one, pinning the iterator-remove-then-continue guarantee.
hyperxpro
added a commit
that referenced
this pull request
Jul 18, 2026
…-removed coverage (#2249) Motivation: #2225 replaced `DefaultChannelPool`'s collect-then-`removeAll` reap with an in-place O(n) iterator removal, but two test gaps remained. First, `cleanerReapsExpiredButKeepsHealthyInSameTick` used a 200 ms idle timeout, making it susceptible to GC or scheduling delays that could incorrectly classify fresh channels as expired. Second, because `offer()` inserts at the front of the deque, the test never exercised the iterator-removal case where iteration must continue correctly after removing a node. Modification: Increase the idle timeout in the mixed test to 1 second, matching `channelReofferedAfterExpiryIsNotReaped`, to eliminate the tight timing dependency. Add `cleanerContinuesPastRemovedNodesToReachKeptNodes`, which deterministically constructs an alternating closed/open deque so every removed node is immediately followed by a retained one, directly validating the iterator-remove-then-continue behavior. Test-only; no production changes. Result: The test suite is no longer sensitive to timing jitter and now deterministically verifies the iterator removal logic introduced in #2225.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DefaultChannelPool.IdleChannelDetector.reapPartition collected drop-worthy channels into a list and then called ConcurrentLinkedDeque.removeAll(list). removeAll re-walks all n nodes doing an O(m) list contains() per node — O(n*m), degenerating toward O(n^2) when a whole partition is dropped in one tick (a load spike's connections idling out as a wave, or a peer dropping many keep-alives at once). This runs on the shared HashedWheelTimer thread, so a slow tick also delays request-timeout firing for every pool sharing that timer.
Unlink drop-worthy channels in place through the iterator (it.remove()) during the single scan instead, keeping the whole pass O(n). Behavior is unchanged: the same channels are closed/unlinked and healthy ones kept; only the removal mechanism differs. Drop the now-unused lazyAdd helper and the List/ArrayList imports.
Add white-box tests covering many-channels-in-one-tick reaping (idle-expired and tombstones) and a mixed expired-plus-healthy pass that must remove some nodes while keeping others leasable.