Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Disallow TimeSpan.Zero in rate limiters
  • Loading branch information
BrennanConroy committed Sep 13, 2022
commit dc6dedc6ad77b677dd9e41c73bbfdb354644789c
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ public FixedWindowRateLimiter(FixedWindowRateLimiterOptions options)
{
throw new ArgumentException($"{nameof(options.QueueLimit)} must be set to a value greater than or equal to 0.", nameof(options));
}
if (options.Window < TimeSpan.Zero)
if (options.Window <= TimeSpan.Zero)
{
throw new ArgumentException($"{nameof(options.Window)} must be set to a value greater than or equal to TimeSpan.Zero.", nameof(options));
throw new ArgumentException($"{nameof(options.Window)} must be set to a value greater than TimeSpan.Zero.", nameof(options));
}

_options = new FixedWindowRateLimiterOptions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ public sealed class FixedWindowRateLimiterOptions
{
/// <summary>
/// Specifies the time window that takes in the requests.
/// Must be set to a value >= <see cref="TimeSpan.Zero" /> by the time these options are passed to the constructor of <see cref="FixedWindowRateLimiter"/>.
/// Must be set to a value greater than <see cref="TimeSpan.Zero" /> by the time these options are passed to the constructor of <see cref="FixedWindowRateLimiter"/>.
/// </summary>
/// <remarks><see cref="TimeSpan.Zero"/> means the limiter will never replenish.</remarks>
public TimeSpan Window { get; set; } = TimeSpan.Zero;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ public SlidingWindowRateLimiter(SlidingWindowRateLimiterOptions options)
{
throw new ArgumentException($"{nameof(options.QueueLimit)} must be set to a value greater than or equal to 0.", nameof(options));
}
if (options.Window < TimeSpan.Zero)
if (options.Window <= TimeSpan.Zero)
{
throw new ArgumentException($"{nameof(options.Window)} must be set to a value greater than or equal to TimeSpan.Zero.", nameof(options));
throw new ArgumentException($"{nameof(options.Window)} must be set to a value greater than TimeSpan.Zero.", nameof(options));
}

_options = new SlidingWindowRateLimiterOptions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ public sealed class SlidingWindowRateLimiterOptions
{
/// <summary>
/// Specifies the minimum period between replenishments.
/// Must be set to a value >= <see cref="TimeSpan.Zero" /> by the time these options are passed to the constructor of <see cref="SlidingWindowRateLimiter"/>.
/// Must be set to a value greater than <see cref="TimeSpan.Zero" /> by the time these options are passed to the constructor of <see cref="SlidingWindowRateLimiter"/>.
/// </summary>
/// <remarks><see cref="TimeSpan.Zero"/> means the limiter will never replenish.</remarks>
public TimeSpan Window { get; set; } = TimeSpan.Zero;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ public TokenBucketRateLimiter(TokenBucketRateLimiterOptions options)
{
throw new ArgumentException($"{nameof(options.QueueLimit)} must be set to a value greater than or equal to 0.", nameof(options));
}
if (options.ReplenishmentPeriod < TimeSpan.Zero)
if (options.ReplenishmentPeriod <= TimeSpan.Zero)
{
throw new ArgumentException($"{nameof(options.ReplenishmentPeriod)} must be set to a value greater than or equal to TimeSpan.Zero.", nameof(options));
throw new ArgumentException($"{nameof(options.ReplenishmentPeriod)} must be set to a value greater than TimeSpan.Zero.", nameof(options));
}

_options = new TokenBucketRateLimiterOptions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ public sealed class TokenBucketRateLimiterOptions
{
/// <summary>
/// Specifies the minimum period between replenishments.
/// Must be set to a value >= <see cref="TimeSpan.Zero" /> by the time these options are passed to the constructor of <see cref="TokenBucketRateLimiter"/>.
/// Must be set to a value greater than <see cref="TimeSpan.Zero" /> by the time these options are passed to the constructor of <see cref="TokenBucketRateLimiter"/>.
/// </summary>
/// <remarks><see cref="TimeSpan.Zero"/> means the limiter will never replenish.</remarks>
public TimeSpan ReplenishmentPeriod { get; set; } = TimeSpan.Zero;

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,15 @@ public override void InvalidOptionsThrows()
Window = TimeSpan.FromMinutes(-2),
AutoReplenishment = false,
}));
Assert.Throws<ArgumentException>(
Copy link
Member

Choose a reason for hiding this comment

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

Ideally all of these Assert.Throws would instead be AssertExtensions.Throws and have a first argument that's the name of the argument we expect is being flagged as invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't aware of those extensions 👍

() => new FixedWindowRateLimiter(new FixedWindowRateLimiterOptions
{
PermitLimit = 1,
QueueProcessingOrder = QueueProcessingOrder.NewestFirst,
QueueLimit = 1,
Window = TimeSpan.Zero,
AutoReplenishment = false,
}));
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,16 @@ public override void InvalidOptionsThrows()
SegmentsPerWindow = 1,
AutoReplenishment = false
}));
Assert.Throws<ArgumentException>(
() => new SlidingWindowRateLimiter(new SlidingWindowRateLimiterOptions
{
PermitLimit = 1,
QueueProcessingOrder = QueueProcessingOrder.NewestFirst,
QueueLimit = 1,
Window = TimeSpan.Zero,
SegmentsPerWindow = 1,
AutoReplenishment = false
}));
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,16 @@ public override void InvalidOptionsThrows()
TokensPerPeriod = 1,
AutoReplenishment = false
}));
Assert.Throws<ArgumentException>(
() => new TokenBucketRateLimiter(new TokenBucketRateLimiterOptions
{
TokenLimit = 1,
QueueProcessingOrder = QueueProcessingOrder.NewestFirst,
QueueLimit = 1,
ReplenishmentPeriod = TimeSpan.Zero,
TokensPerPeriod = 1,
AutoReplenishment = false
}));
}

[Fact]
Expand Down