Skip to content

fix(client): cancel idle interval when pool empties - #292

Merged
seanmonstar merged 1 commit into
hyperium:masterfrom
lrowe:lrowe-cancel-idle-interval-when-empty
Aug 19, 2026
Merged

fix(client): cancel idle interval when pool empties#292
seanmonstar merged 1 commit into
hyperium:masterfrom
lrowe:lrowe-cancel-idle-interval-when-empty

Conversation

@lrowe

Copy link
Copy Markdown
Contributor

The IdleTask is started on first connection when pool_idle_timeout is set however it is not stopped when the pool is empty leaving the event loop spinning every pool_idle_timeout ms.

I noticed this in Deno (see: denoland/deno#29444) and the fix seems fairly small. Do you think such a change would be worth considering? Happy to work this up with test cases if so.

@Catwoman08

Copy link
Copy Markdown

This is tied to hyperium/hyper#4100

@seanmonstar

Copy link
Copy Markdown
Member

Yea, I think this is worth fixing. And it should "just work", a new connection will see None and start a new timer task. Some sort of unit test would be good to add to the legacy pool module.

@seanmonstarseanmonstar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just needs a test, I think.

@lrowe
lroweforce-pushed the lrowe-cancel-idle-interval-when-empty branch from e82b31f to 42d0315CompareAugust 17, 2026 23:30
@lrowe
lrowe marked this pull request as ready for review August 17, 2026 23:31
@lrowe

Copy link
Copy Markdown
ContributorAuthor

I've added an assert to test_pool_timer_removes_expired that idle_interval_ref.is_none().

@lrowe
lrowe requested a review from seanmonstarAugust 17, 2026 23:33
Comment threadsrc/client/legacy/pool.rs
@lrowe
lroweforce-pushed the lrowe-cancel-idle-interval-when-empty branch from 42d0315 to b6f8e3cCompareAugust 19, 2026 16:33
@lrowe
lrowe requested a review from seanmonstarAugust 19, 2026 16:33
@seanmonstar
seanmonstar merged commit 551e7bb into hyperium:masterAug 19, 2026
18 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lrowe@Catwoman08@seanmonstar