Skip to content

Conversation

@sp1ff
Copy link

@sp1ff sp1ff commented Jul 12, 2025

This (small) patch changes RateLimit to wrap an inner implementation in a Mutex allowing us to clone the type as much as we want without allowing callers to circumvent rate-limiting.

This is the fix to which I alluded in #794.

This (small) patch changes `RateLimit` to wrap an inner implementation
in a `Mutex` allowing us to clone the type as much as we want without
allowing callers to circumvent rate-limiting.
@guilload
Copy link
Contributor

guilload commented Aug 4, 2025

I don't think users should pay for a mutex if it's not necessary. Maybe we could introduce an AtomicRate that we can clone, then make RateLimit generic over a RateImpl trait and then RateLimit be Clone if S and RateImpl are Clone.

@sp1ff
Copy link
Author

sp1ff commented Aug 27, 2025

Well, I just realized that my solution has a nasty bug: my newly Clonable RateLimit instances will share a reference to one instance of RateLimitInner, which when polled will park the caller's Waker with it's Sleep future (when it's decided to rate-limit). So if more than one RateLimit instance calls ready() when rate-limited, the last one "wins", overwriting the other's Waker, leaving the task stranded.

I don't think users should pay for a mutex if it's not necessary. Maybe we could introduce an AtomicRate that we can clone, then make RateLimit generic over a RateImpl trait and then RateLimit be Clone if S and RateImpl are Clone.

That's interesting: would your RateImpl trait be implemented by various rate limiting strategies (fixed window, sliding window log, sliding window counter, token bucket, &c, &c)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants