Use cache for beatmap lookups on spectate screen #29968
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@peppy noticed recently that attempting to spectate just a few users was very likely to end up in requests very quickly being rejected with code 429 ("Too Many Requests"). I'm somewhat certain that the reason for that is that a significant number of players is wont to retry a lot in quick succession. That means that spectator server is going to note a lot of gameplay start and end messages in quick succession, too. And as it turns out, every gameplay start would end up triggering a new beatmap set fetch request: https://github.com/ppy/osu/blob/ccf1acce56798497edfaf92d3ece933469edcf0a/osu.Game/Screens/Spectate/SpectatorScreen.cs#L131-L134 https://github.com/ppy/osu/blob/ccf1acce56798497edfaf92d3ece933469edcf0a/osu.Game/Screens/Play/SoloSpectatorScreen.cs#L168-L172 https://github.com/ppy/osu/blob/ccf1acce56798497edfaf92d3ece933469edcf0a/osu.Game/Screens/Play/SoloSpectatorScreen.cs#L243-L256 To attempt to curtail that, use the beatmap cache instead, which should prevent these unnecessary requests from firing in the first place, therefore reducing the chance of the client getting throttled. This technically means that a different endpoint is used to fetch the data (`GET /beatmaps/?ids[]=` rather than `GET /beatmapsets/lookup?beatmap_id={id}`), but docs claim that both should return the same data, and it looks to work fine in practice.
|
I think this is fine to fix and merge, but it doesn't seem to solve the larger issue. Here's a log on this PR showing it still happening. I'm a bit short on time at this moment and haven't investigated further, but I repro'd this by opening the online users screen the spectating two people in succession: |
peppy
approved these changes
Sep 23, 2024
Well that was a bit of misdirection wasn't it, since the original snippet you posted showed the beatmap set request failing, but I'd be willing to bet money on the fact that it's all of these that cause the ratelimit to get exceeded: To resolve this we'd probably want pagination on that display there, except for the fact that there's a search box in there that allows searching by username, and we can't paginate with username search before these requests, because we only have user IDs to work with at that point.... Would probably need spectator server changes to at least send usernames across the wire. If not more. |
|
Bancho allowed lookups this fast in order for this screen to exist. We probably need the osu-web end rate limit adjusted. Paginating would be a bit of a step backwards from people's expectations, I think. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@peppy noticed recently that attempting to spectate just a few users was very likely to end up in requests very quickly being rejected with code 429 ("Too Many Requests").
I'm somewhat certain that the reason for that is that a significant number of players is wont to retry a lot in quick succession. That means that spectator server is going to note a lot of gameplay start and end messages in quick succession, too. And as it turns out, every gameplay start would end up triggering a new beatmap set fetch request:
osu/osu.Game/Screens/Spectate/SpectatorScreen.cs
Lines 131 to 134 in ccf1acc
osu/osu.Game/Screens/Play/SoloSpectatorScreen.cs
Lines 168 to 172 in ccf1acc
osu/osu.Game/Screens/Play/SoloSpectatorScreen.cs
Lines 243 to 256 in ccf1acc
To attempt to curtail that, use the beatmap cache instead, which should prevent these unnecessary requests from firing in the first place, therefore reducing the chance of the client getting throttled.
This technically means that a different endpoint is used to fetch the data (
GET /beatmaps/?ids[]=rather thanGET /beatmapsets/lookup?beatmap_id={id}), but docs claim that both should return the same data, and it looks to work fine in practice.