.NET 8 Timer Implementation Has Silent Period Minimum and TokenBucketRateLimiter Is Not Aware of This

[rorozcov]

Description

We were using TokenBucketRateLimiter with a ReplenishmentPeriod in the microseconds (so less than 1ms).

This value is just passed into the Timer constructor by the limiter.

The timer constructor however, has a line of code where it gets the period in total milliseconds from the timestamp, and then casts it to a long.

This meant that any value < 1ms was being casted to 0. We ran into an issue where the rate limiter was never replenishing, because the timer would not run, so our application would just pause and no errors were present.

In other applications our replenishment periods were > 1ms, so this wasn't caught in those instances.

I don't know if this qualifies as a bug, but at least the documentation should warn you about this.
TokenBucketRateLimiterOptions only warns about > TimeSpan.Zero..

Timer's reference docs implicitly mention it here, which is the constructor used by the rate limiter..

These other docs do explicitly mention time being in milliseconds, but it is not the same constructor.

The TokenBucketRateLimiter could maybe do something to prevent the timer from being initialized with a low enough TimeSpan, but then that would couple the limiter implementation much closer to the Timer implementation.

I am not sure how or if this is something the .NET team would look to fix, but for us at least a documentation update or summary comment update would go a long way to make others aware of the limitation.

Reproduction Steps

Create a TokenBucketRateLimiter with a ReplenishmentPeriod of less than 1ms.

        var options = new TokenBucketRateLimiterOptions
        {
            TokenLimit = 1000,
            QueueLimit = int.MaxValue,
            TokensPerPeriod = 1,
            ReplenishmentPeriod = TimeSpan.FromMicroseconds(500),
            AutoReplenishment = true
        };

        var rateLimiter = new TokenBucketRateLimiter(options);

Then in the VS debugger:

Expected behavior

To allow for lower than 1ms timespans or make it explicit that we cannot do a replenishment period (or timer period, depending on where the "fix") should be applied, lower than 1ms unless it's the infinite time span which is allowed.

Actual behavior

Added it in repro steps

Regression?

No response

Known Workarounds

We have our own logic in our applications to ensure

Configuration

.NET 8.0.403
Windows (issue is found in our Linux containers too, running the aspnet image from the Microsoft MCR).
We run in x64 but target AnyCPU.

Other information

I am happy to help in any way with fixing this issue, including creating pull requests since I know the team is quite busy. I wanted to consult with you all first before applying a "fix" I would like but that might not be aligned with your goals/thinking.

[dotnet-policy-service]

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

[BrennanConroy]

Similar issue #105446

We'll need to decide what behavior to have here. Throw for "invalid" values or set a minimum timer value.

[rorozcov]

I'm happy to issue a PR to address this. The question is, how do we decide what the "correct" implementation is.

I'd argue that the rate limiter should just have a check for ms > 1 and throw, with updates to docs and comments.

I don't know if you want to "fix" the timer to allow smaller timescales than 1ms. That might be a more disruptive change given it's probably used in many more places

[rorozcov]

@BrennanConroy

So I'd say we have two choices in the timer:

1. We throw if you pass in a value < 1ms but greater than 0ms. This would be backwards compatible with anyone using insanely large Timespans (like int64.max for total ms or ns), and throwing could break users, their timers are already not working if they're passing in < 1ms Timespans.

2. We change the math to use total nanoseconds instead, considering the idea of anyone using int64.max as their Timespan is extremely unlikely. This could break someone passing in int64.max for the total milliseconds, but I'd argue that is an unlikely edge case (int64.max in nanoseconds is around 292 years, with int64.max ms being 292.5 million years) exceeding human and hardware lifespans that we are unlikely to break anyone. This is my preferred option and it would enable much smaller periods for Timers.

Either fix will require documentation changes to both the Timer docs and the TokenBucketRateLimiter docs.

I am happy to write a PR if you and the .NET team agree in concept so we can iterate and solve it.

Let me know!

[rorozcov]

I've checked the underlying APIs and it appears we do need to keep using milliseconds. I dug deeper and found that the other timers in .NET use different approaches.

System.Threading.Timer (this timer) and System.Threading.PeriodicTimer both just cast the milliseconds to a long.

However, System.Timers.Timer uses Math.Ceiling to avoid the problems I've mentioned in this thread.

Now, while any number < 1ms is going to be way less than the minimum timer resolution, the intent of a user passing in < 1ms should still be that the timer will run. Assuming less than 1ms is 0 is confusing from a user's point of view, because only by explicitly setting it to zero would one assume it'll only run once.

I've consulted with others and have reached the following two options:

1. Updated System.Threading.Timer to use Match.Ceiling. This brings backcompat risks since other users could've written code around this problem already.

2. Updating the docs to make it extremely clear that any value below 1ms will be treated as 0 and that users should mitigate this themselves.

[BrennanConroy]

It's very unlikely that we would make a change in the timer implementation for this.

Really we have 3 options IMO:

1. Do nothing except update docs
2. Set a lower bound of 1ms in the rate limiter implementations
3. Throw if we get an interval of less than 1ms

I think I'm leaning towards option 2 (with some doc updates). Limiters will keep working, and if someone uses settings that would have made the interval less than 1ms the limiter will worst case be more restrictive than the user expects, which seems better than less restrictive.

[TonyValenti]

I believe that option 2 is the right option as it captures user intent.

[rorozcov]

@BrennanConroy is someone actively looking into fixing this?

I might have some time in the next month or so to address the issue. It should only be a couple of lines of code to set the limit to 1ms.

Thoughts?

[su-senka]

@BrennanConroy @rorozcov — checking in on this one. It looks like the team's direction settled on option 2 (clamp ReplenishmentPeriod to a 1ms floor inside the rate limiter implementations, with accompanying doc updates), which is also what @TonyValenti landed on.
@rorozcov, you mentioned last September you might have time to pick this up — has that happened, or is there a PR in flight I'm missing? If not, I'd like to take a crack at it: validate in TokenBucketRateLimiter's constructor and throw/clamp (matching the agreed approach) for periods below 1ms, plus a test covering the repro above and a doc/comment update on TokenBucketRateLimiterOptions.ReplenishmentPeriod. Let me know if that scope sounds right before I start.