System.Threading.RateLimiting.DefaultPartitionedRateLimiter cache invalidation bug

[PaulKlein]

Description

In the System.Threading.RateLimiting nuget package, PartitionedRateLimiter.Create() returns DefaultPartionedRateLimiter instances.

The DefaultPartionedRateLimiter internally caches the created partioned RateLimiter instances. It tracks when this cache is invalid using _cacheInvalid which is checked from the Heartbeat method.

However the Heartbeat() method never clears this flag, so once set, the cache invalidation runs every time the heartbeat triggers, which by default is every 100 milliseconds. This means it is constantly clearing + re-initialising the cache list.

See: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/DefaultPartitionedRateLimiter.cs#L216

Reproduction Steps

Create a partitioned rate limiter, and request a rate limit lease to trigger the initial cache invalidation.

var partitionedRateLimiter = PartitionedRateLimiter.Create<string, string>(
  resource => RateLimitPartition.GetNoLimiter(resource)
);
var rateLimiter = partitionedRateLimiter.AttemptAcquire("myResource");

From then on, every time DefaultPartionedRateLimiter.Heartbeat() fires, the _cacheInvalid logic triggers and clears + re-initialises the _cachedLimiters list.

Expected behavior

After the _cachedLimiters list is rebuilt, _cacheInvalid should be reset to false so that it is only rebuilt when required.

Actual behavior

The cached list is rebuilt every 100 milliseconds unnecessarily.

Regression?

No response

Known Workarounds

No response

Configuration

System.Threading.RateLimiting Nuget package 8.0.0

Other information

No response

[ghost]

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

Issue Details

---

Description

In the System.Threading.RateLimiting nuget package, PartitionedRateLimiter.Create() returns DefaultPartionedRateLimiter instances.

The DefaultPartionedRateLimiter internally caches the created partioned RateLimiter instances. It tracks when this cache is invalid using _cacheInvalid which is checked from the Heartbeat method.

However the Heartbeat() method never clears this flag, so once set, the cache invalidation runs every time the heartbeat triggers, which by default is every 100 milliseconds. This means it is constantly clearing + re-initialising the cache list.

See: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/DefaultPartitionedRateLimiter.cs#L216

Reproduction Steps

Create a partitioned rate limiter, and request a rate limit lease to trigger the initial cache invalidation.

var partitionedRateLimiter = PartitionedRateLimiter.Create<string, string>(
  resource => RateLimitPartition.GetNoLimiter(resource)
);
var rateLimiter = partitionedRateLimiter.AttemptAcquire("myResource");

From then on, every time DefaultPartionedRateLimiter.Heartbeat() fires, the _cacheInvalid logic triggers and clears + re-initialises the _cachedLimiters list.

Expected behavior

After the _cachedLimiters list is rebuilt, _cacheInvalid should be reset to false so that it is only rebuilt when required.

Actual behavior

The cached list is rebuilt every 100 milliseconds unnecessarily.

Regression?

No response

Known Workarounds

No response

Configuration

System.Threading.RateLimiting Nuget package 8.0.0

Other information

No response

Author:	PaulKlein
Assignees:	-
Labels:	area-System.Threading
Milestone:	-

[BrennanConroy]

Thanks for catching that, would you be interested in contributing a PR to fix it?

[PaulKlein]

Sure, I think it's just a case of resetting this flag after rebuilding the cache? I've added a simple PR for this, hopefully this is OK.

I looked at the tests as well but I wasn't sure if there's any easy way to add a test for this.

Cheers