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

rorozcov commented 5 days ago

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[bot] commented 5 days ago

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

BrennanConroy commented 5 days ago

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

rorozcov commented 5 days ago

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

dotnet / runtime