remove unnecessary RateLimiterHelper#128247
Open
WeihanLi wants to merge 7 commits into
Open
Conversation
dotnet-policy-service
Bot
added
the
community-contribution
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Removes the internal RateLimiterHelper wrapper and replaces calls with direct Stopwatch.GetElapsedTime invocations across the rate limiter implementations.
Changes:
- Delete
RateLimiterHelper.csand remove its entry from the project file. - Replace
RateLimiterHelper.GetElapsedTime(...)calls withStopwatch.GetElapsedTime(...)in the four rate limiter classes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| RateLimiterHelper.cs | Removed helper class. |
| System.Threading.RateLimiting.csproj | Dropped compile reference to deleted helper. |
| ConcurrencyLimiter.cs | Switched IdleDuration to call Stopwatch.GetElapsedTime directly. |
| FixedWindowRateLimiter.cs | Replaced helper usages, including the _getElapsedTime delegate default and IdleDuration. |
| SlidingWindowRateLimiter.cs | Replaced helper usages in IdleDuration and ReplenishInternal. |
| TokenBucketRateLimiter.cs | Replaced helper usages in IdleDuration and ReplenishInternal. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @VSadov |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+10
to
+12
| extension(Stopwatch) | ||
| { | ||
| if (startTimestamp is null) | ||
| public static TimeSpan? GetElapsedTime(long? startTimestamp) |
| /// In tests, this field can be reassigned via reflection to inject custom time behavior without modifying the public API. | ||
| /// </summary> | ||
| private readonly Func<long?, TimeSpan?> _getElapsedTime = RateLimiterHelper.GetElapsedTime; | ||
| private readonly Func<long?, TimeSpan?> _getElapsedTime = Stopwatch.GetElapsedTime; |
| /// <summary> | ||
| /// Function to calculate elapsed time from a given tick value. | ||
| /// Defaults to <see cref="RateLimiterHelper.GetElapsedTime(long?)"/>. | ||
| /// Defaults to the <c>Stopwatch.GetElapsedTime(long?)</c> extension which returns <see langword="null"/> when the timestamp is <see langword="null"/>. |
| extension(Stopwatch) | ||
| { | ||
| if (startTimestamp is null) | ||
| public static TimeSpan? GetElapsedTime(long? startTimestamp) |
| /// <summary> | ||
| /// Function to calculate elapsed time from a given tick value. | ||
| /// Defaults to <see cref="RateLimiterHelper.GetElapsedTime(long?)"/>. | ||
| /// Defaults to the <see cref="RateLimiterHelper.GetElapsedTime(long?)"/> which returns <see langword="null"/> when the timestamp is <see langword="null"/>, and returns <c>Stopwatch.GetElapsedTime(long)</c> when not null. |
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.
This pull request refactors how elapsed time is calculated throughout the rate limiter implementations. The main change is to use
Stopwatch.GetElapsedTimedirectly instead of routing these calls through theRateLimiterHelperclass, which is simplified as a result. This reduces indirection and makes the code more straightforward.Refactoring elapsed time calculation:
RateLimiterHelper.GetElapsedTimeare replaced with direct calls toStopwatch.GetElapsedTimein theConcurrencyLimiter,FixedWindowRateLimiter,SlidingWindowRateLimiter, andTokenBucketRateLimiterclasses, including theirIdleDurationproperties and internal replenishment logic. [1] [2] [3] [4] [5] [6] [7] [8]Code cleanup and simplification:
GetElapsedTime(long, long)is removed fromRateLimiterHelper, and the class is simplified to only provide a helper for nullable timestamps. [1] [2]