Skip to content

Add rate limiting #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Add rate limiting #16

merged 2 commits into from
Jan 8, 2025

Conversation

bdach
Copy link
Contributor

@bdach bdach commented Jan 7, 2025

See https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit?view=aspnetcore-9.0 for more information on this.

I've never really tried this in production anywhere, but given it's in ASP.NET Core built-in, I'd hope it should be solid enough?

Limits mostly taken out of rear, and likely are much too lenient anyway. The hard limit is 30 requests/min, with 5 requests being replenished back to the tally every 10 seconds. The limit is enforced per user.

See
https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit?view=aspnetcore-9.0
for more information on this.

I've never really tried this in production anywhere, but given it's in
ASP.NET Core built-in, I'd hope it should be solid enough?

Limits mostly taken out of rear, and likely are much too lenient anyway.
@bdach bdach self-assigned this Jan 7, 2025
@bdach bdach mentioned this pull request Jan 7, 2025
@peppy peppy self-requested a review January 8, 2025 05:05

return RateLimitPartition.GetSlidingWindowLimiter(userId, _ => new SlidingWindowRateLimiterOptions
{
PermitLimit = 30,
Copy link
Member

Choose a reason for hiding this comment

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

If anything, I'd agree at this being too lenient, and it could be dropped as low as 5 per minute. Maybe we start stringent and wait for issues to occur (because let's be honest: the most likely case is going to be a runaway client stuck in an infinite retry death spiral as opposed to someone trying to abuse the service).

Suggested change
PermitLimit = 30,
PermitLimit = 5,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, although I've done a bit of a different change, see 592b035. The rate limit is now 6 req/min with 2 requests being replenished to the pool every 20sec.

The reason why I want that specific sort of thing is that a full upload flow takes 2 requests (the initial one to set up the beatmaps and get the file listing, and the second one to either do a full .osz upload or a differential update). So I want these rate limits to not be odd numbers, to prevent half-failing submissions because the client ran out of ratelimit between one request and the other.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine, yeah.

@peppy peppy merged commit 82190f3 into ppy:master Jan 8, 2025
4 checks passed
@bdach bdach deleted the rate-limiting branch January 8, 2025 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants