Skip to content

fix ChainedRateLimiter treating IdleDuration and chained ReplenishmentRateLimiters incorrectly#126158

Merged
DeagleGross merged 8 commits intodotnet:mainfrom
DeagleGross:dmkorolev/ratelimiting-idleduration
Apr 10, 2026
Merged

fix ChainedRateLimiter treating IdleDuration and chained ReplenishmentRateLimiters incorrectly#126158
DeagleGross merged 8 commits intodotnet:mainfrom
DeagleGross:dmkorolev/ratelimiting-idleduration

Conversation

@DeagleGross
Copy link
Copy Markdown
Member

@DeagleGross DeagleGross commented Mar 26, 2026 •
edited
Loading

  1. Fixed ChainedRateLimiter.IdleDuration. In the current PR if it detects any of chained child rateLimiters having null IdleDuration, it will return null now. IdleDuration equalling now means rateLimiter is in use or is not ready to be idle.

  2. DefaultPartitionedRateLimiter does not replenish the children rateLimiters of ChainedRateLimiter. Now there are 2 implementations of chained limiter depending on the passed limiters to RateLimiter.CreateChained(...)

Related #68776
Fixes #125560

@DeagleGross DeagleGross self-assigned this Mar 26, 2026
@DeagleGross DeagleGross requested a review from halter73 March 26, 2026 16:52
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

dotnet-policy-service bot commented Mar 26, 2026

Tagging subscribers to this area: @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Copilot AI reviewed Mar 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.IdleDuration to return null if any child limiter reports IdleDuration == null.
  • Add internal ChainedRateLimiter.TryReplenish() and call it from DefaultPartitionedRateLimiter.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.

@BrennanConroy
Copy link
Copy Markdown
Member

BrennanConroy commented Mar 26, 2026

2. because it is unknown of how to implement ReplenishmentPeriod and IsAutoReplenishing properties

IsAutoReplenishing would be false if any of the limiters were of type ReplenishingRateLimiter and had IsAutoReplenshing set as false.

ReplenishmentPeriod would have to be the lowest non-zero (or non-negative) value from the sub-limiters.

Wonder if RateLimiter.CreateChained(...) should return 2 different ChainedRateLimiter implementations, one that is just RateLimiter and one that is ReplenishingRateLimiter depending on the passed in limiters.

Copilot AI review requested due to automatic review settings March 27, 2026 09:54
@DeagleGross
Copy link
Copy Markdown
Member Author

DeagleGross commented Mar 27, 2026

Wonder if RateLimiter.CreateChained(...) should return 2 different ChainedRateLimiter implementations, one that is just RateLimiter and one that is ReplenishingRateLimiter depending on the passed in limiters.

Reworked like that: introduced shared static methods on ChainedRateLimiter to not duplicate code. Now there is no special case on DefaultPartitionedRateLimiter for ChainedRateLimiter

Copilot AI reviewed Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

This was referenced Mar 27, 2026
Open
Open
Open
Open
Open
Open
BrennanConroy
BrennanConroy reviewed Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

nit: It's a bit odd to have the static shared methods inside a concrete class (ChainedRateLimiter).

Could we move the shared methods to a separate static class, similar to

runtime/src/libraries/System.IO.Hashing/src/System/IO/Hashing/XxHashShared.cs

Line 21 in 3270e92

internal static unsafe class XxHashShared

@build-analysis build-analysis bot mentioned this pull request Apr 8, 2026
Closed
Copilot AI review requested due to automatic review settings April 9, 2026 13:09
Copilot AI reviewed Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

This was referenced Apr 9, 2026
Open
Open
Open
Open
@DeagleGross
Copy link
Copy Markdown
Member Author

DeagleGross commented Apr 10, 2026

/ba-g CI keeps failing on WASM tests, not related to the actual code change. Merging

@DeagleGross DeagleGross merged commit c6d701a into dotnet:main Apr 10, 2026
88 of 98 checks passed
@DeagleGross DeagleGross deleted the dmkorolev/ratelimiting-idleduration branch April 10, 2026 16:18
@dotnet-maestro dotnet-maestro bot mentioned this pull request Apr 11, 2026
Open
Copilot AI mentioned this pull request Apr 11, 2026
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@halter73 halter73 Awaiting requested review from halter73

Assignees

@DeagleGross DeagleGross

Labels

area-System.Threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

System.Threading.RateLimiting.ChainedRateLimiter

3 participants

@DeagleGross @BrennanConroy