[HTTP] RemoveStalePools should never run in parallel

[ManickaP]

In certain cases, when there is a lot of different hosts and thus a lot of pools in pool manager (around 1 million from the customer report). And if scavenge timer (_cleaningTimer, _cleanPoolTimeout) gets triggered very often. The previous callback might still be running (even multiple of them) and thus causing rising number of handles.

The timer itself should never in parallel, we should start the cleanup logic while a previous callback is still processing.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In certain cases, when there is a lot of different hosts and thus a lot of pools in pool manager (around 1 million from the customer report). And if scavenge timer (_cleaningTimer, _cleanPoolTimeout) gets triggered very often. The previous callback might still be running (even multiple of them) and thus causing rising number of handles.

The timer itself should never in parallel, we should start the cleanup logic while a previous callback is still processing.

Author:	ManickaP
Assignees:	-
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[stephentoub]

The previous callback might still be running (even multiple of them) and thus causing rising number of handles.

Is that really the cause? Seems the rising number of handles is more an inability to clean up as fast as is needed... the extra callbacks aren't helping but they also shouldn't be hurting. If the cleanup is blocking, as in #61506, then the extra work items here will block more thread pool threads, but if anything I'd expect that ironically to actually help by putting backpressure on other work that's creating more handles.

I think it's fine to revisit the scavenging mechanism, e.g. ensuring only one scavenger runs at a time per pool. But I suspect changing that won't actually fix any underlying issue here.

[ManickaP]

What I understood from the email and graphs shared, the rise of handles was happening only with 10s interval for scavenging, when multiple instances of timer were running at the same time. When they used longer interval (30s), the number of handles was stable. If the problem lies in slow clean up, I'd expect to see the same problem with longer interval as well.

I did not repro it nor went deep into the code to understand the exact root cause. I only made conclusions based on what I saw. I might be wrong.

Either way, it won't hurt to fix this as well as the other issue (#61506).

[geoffkizer]

I think it's fine to revisit the scavenging mechanism, e.g. ensuring only one scavenger runs at a time per pool. But I suspect changing that won't actually fix any underlying issue here.

Yeah, I tend to agree. It's kind of weird that we run more than one scavenger at a time, but I'm not sure how it would actually make things worse. Possibly the multiple scavengers end up contending on the same connection pool locks? E.g. one starts, slowly enumerates the pools, and then a second starts, catches up to the same point in the enumeration as the first one, and starts trying to take the same connection locks as the first, and things go wonky from there?

What I understood from the email and graphs shared, the rise of handles was happening only with 10s interval for scavenging, when multiple instances of timer were running at the same time. When they used longer interval (30s), the number of handles was stable. If the problem lies in slow clean up, I'd expect to see the same problem with longer interval as well.

I agree with your assessment of the data, but I'm not sure how to explain it.

Either way, it won't hurt to fix this as well as the other issue (#61506).

I agree, though I would prioritize #61506 above this. It seems like a pretty straightforward fix that should just make things better.