dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.99k stars 4.66k forks source link

RateLimiter seems to hang with 1024 Permit Limit #105446

Open TonyValenti opened 2 months ago

TonyValenti commented 2 months ago

Description

When using a Permit limit of 1024, the SlidingWindowRateLimiter seems to hang.

Reproduction Steps

Run the code below. It should complete in about 2 seconds. Instead, it hangs forever.

using System.Threading.RateLimiting;

internal static class Program {
    public static async Task Main(string[] args) {

        var Values = await TestEnumerable()
            .ToListAsync()
            .ConfigureAwait(false)
            ;

    }

    static async IAsyncEnumerable<int> TestEnumerable() {
        var Max = 1024; //Change this.

        var Options = new System.Threading.RateLimiting.SlidingWindowRateLimiterOptions() {
            AutoReplenishment = true,
            QueueLimit = int.MaxValue,
            QueueProcessingOrder = QueueProcessingOrder.OldestFirst,
            PermitLimit = Max,
            SegmentsPerWindow = Max,
            Window = TimeSpan.FromSeconds(1),
        };

        var RateLimiter = new System.Threading.RateLimiting.SlidingWindowRateLimiter(Options);

        foreach (var Value in Enumerable.Range(0, 2000)) {
            using var Lease = await RateLimiter.AcquireAsync()
                .ConfigureAwait(false)
                ;

            yield return Value;
        }
    }

}

Expected behavior

It completes after 2 secords.

Actual behavior

It hangs forever.

Regression?

Not sure.

Known Workarounds

It seems that setting the Max value to a lower number, like 100, works fine.

Configuration

.NET 8.0.7 Win11 x64

Other information

It is possible that I'm misconfiguring my RateLimitOptions but I dont think so.

dotnet-policy-service[bot] commented 1 month ago

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

TonyValenti commented 1 month ago

I've been digging into this more and it seems like the issue has something to do with the Timer not firing.

This will also demonstrate the issue. You'll notice that the output should appear multiple times but it only shows once.

namespace TimerTests {
    internal class Program {
        static void Main(string[] args) {
            var Value = TimeSpan.FromSeconds(1) / 1024;

            var T = new Timer(TimerAction, default, Value, Value);

            Console.ReadLine();

        }

        private static void TimerAction(object? state) {
            Console.WriteLine(DateTimeOffset.UtcNow);
        }

    }
}
TonyValenti commented 1 month ago

So I think I've found the issue with the Timer class.

In this code:

        public Timer(TimerCallback callback,
                     object? state,
                     TimeSpan dueTime,
                     TimeSpan period)
        {
            long dueTm = (long)dueTime.TotalMilliseconds;
            ArgumentOutOfRangeException.ThrowIfLessThan(dueTm, -1, nameof(dueTime));
            ArgumentOutOfRangeException.ThrowIfGreaterThan(dueTm, MaxSupportedTimeout, nameof(dueTime));

            long periodTm = (long)period.TotalMilliseconds;
            ArgumentOutOfRangeException.ThrowIfLessThan(periodTm, -1, nameof(period));
            ArgumentOutOfRangeException.ThrowIfGreaterThan(periodTm, MaxSupportedTimeout, nameof(period));

            TimerSetup(callback, state, (uint)dueTm, (uint)periodTm);
        }

You'll notice this line:

            long periodTm = (long)period.TotalMilliseconds;

Timers run only once if their period is set to 0. In the situation above, I clearly want the timer to repeat because I've provided a non-zero TimeSpan, but the timespan gets rounded down to Zero when the cast happens.

I think there are two potential ways to solve this. Either:

if(period is > TimeSpan.Zero && period < TimeSpan.FromMilliseconds(1)){
   //Pick One:
  period = TimeSpan.FromMilliseconds(1);
  //OR
   throw new ArgumentOutOfRangeException();

}

Personally, I think that setting the period to 1ms is the better option because that captures the intent of "I want this timer to run more than once" and does not push the range check down into user code.

TonyValenti commented 1 month ago

@vcsjones - Any thought about this?