fix ChainedRateLimiter treating IdleDuration and chained ReplenishmentRateLimiters incorrectly#126158
fix ChainedRateLimiter treating IdleDuration and chained ReplenishmentRateLimiters incorrectly#126158DeagleGross wants to merge 3 commits intodotnet:mainfrom
ChainedRateLimiter treating IdleDuration and chained ReplenishmentRateLimiters incorrectly#126158Conversation
|
Tagging subscribers to this area: @agocke, @VSadov |
There was a problem hiding this comment.
Pull request overview
This PR fixes ChainedRateLimiter integration with PartitionedRateLimiter by correcting idle detection semantics and ensuring replenishment is invoked for replenishing limiters wrapped inside a ChainedRateLimiter.
Changes:
- Update
ChainedRateLimiter.IdleDurationto returnnullif any child limiter reportsIdleDuration == null. - Add internal
ChainedRateLimiter.TryReplenish()and call it fromDefaultPartitionedRateLimiter.Heartbeat()so inner replenishing limiters get replenished. - Add/adjust unit tests covering chained replenishment and idle-based eviction behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/ChainedRateLimiter.cs | Fixes IdleDuration semantics and adds TryReplenish() that replenishes inner replenishing limiters. |
| src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/DefaultPartitionedRateLimiter.cs | Special-cases ChainedRateLimiter during heartbeat to call its replenishment logic. |
| src/libraries/System.Threading.RateLimiting/tests/PartitionedRateLimiterTests.cs | Adds coverage ensuring heartbeat replenishes chained inner replenishing limiters and validates eviction behavior with null idle durations. |
| src/libraries/System.Threading.RateLimiting/tests/ChainedLimiterTests.cs | Updates/extends tests for the corrected IdleDuration behavior. |
| foreach (RateLimiter limiter in _limiters) | ||
| { | ||
| if (limiter is ReplenishingRateLimiter replenishingRateLimiter) | ||
| { | ||
| try | ||
| { | ||
| replenishingRateLimiter.TryReplenish(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| exceptions ??= []; | ||
| exceptions.Add(ex); | ||
| } | ||
| } |
There was a problem hiding this comment.
TryReplenish() only calls TryReplenish() on direct children that are ReplenishingRateLimiter. If a child limiter is itself a ChainedRateLimiter (nested chains are possible since RateLimiter.CreateChained doesn’t flatten), replenishing limiters inside that nested chain won’t be replenished. Consider handling nested ChainedRateLimiter instances recursively (or flattening chains at construction) so replenishment reaches all inner replenishing limiters.
Wonder if |
Fixed
ChainedRateLimiter.IdleDuration. In the current PR if it detects any of chained child rateLimiters havingnullIdleDuration, it will return null now.IdleDurationequallingnowmeans rateLimiter is in use or is not ready to be idle.DefaultPartitionedRateLimiterdoes not replenish the children rateLimiters ofChainedRateLimiter. I decided not to makeChainedRateLimiterimplementReplenishingRateLimiter, because it is unknown of how to implementReplenishmentPeriodandIsAutoReplenishingproperties. Instead it now has an internal methodTryReplenish()which is called on theDefaultPartitionedRateLimiter.Heartbeat()Related #68776
Fixes #125560