Skip to content
This repository has been archived by the owner on Mar 13, 2023. It is now read-only.

fix: resolve global ratelimit being exceeded #679

Merged
merged 1 commit into from
Oct 18, 2022
Merged

fix: resolve global ratelimit being exceeded #679

merged 1 commit into from
Oct 18, 2022

Conversation

LordOfPolls
Copy link
Member

@LordOfPolls LordOfPolls commented Oct 17, 2022

What type of pull request is this?

  • Non-breaking code change
  • Breaking code change
  • Documentation change/addition
  • Tests change

Description

Resolves #677

Prevents the HTTP client from exceeding the global rate limit. The previous implementation could be exceeded if you fired a lot of requests in quick succession... defeating the point of the limit.

For the base implementation of this, credit should be given to @Bluenix2 in this thread discord/discord-api-docs#5144 (comment). Their implementation mostly works, but as per their own comment; they can still hit global rate limits. The most likely cause of this would be the request landing in different global buckets due to exec time. This can be resolved simply by changing the maximum allowed requests to a number below 50. 49 works, however, this implementation uses 45 to be more conservative.

Through testing, I was unable to hit a 429 Global rate limit. However, as a further safeguard, this also overrides the default ClientSession.connector with a TCPConnector with a hard limit of global_lock.max_requests (45) active connections.

Test Code:

@listen()
    async def on_ready(self):
        log.info("Bot is ready")

        await asyncio.gather(*[self.http.get_current_bot_information() for i in range(75)])

Changes

  • Utilise new rate limit system to global limits
  • Overrides connector with one with a 45 active connections limit

Checklist

  • I've formatted my code with Black
  • I've ensured my code works on Python 3.10.x
  • I've tested my code

@LordOfPolls LordOfPolls added the Bug::Library A bug in the library label Oct 17, 2022
@LordOfPolls LordOfPolls merged commit 155325e into dev Oct 18, 2022
@switchupcb
Copy link

switchupcb commented Nov 3, 2022

As the creator of that issue, I will state that the disgo implementation works without limiting the number of requests per second. You will also find that limiting the amount of requests per second doesn't do anything to solve the issue, because requests can land at Discord at any time. Such that this implementation will minimize issues but not eliminate them entirely.

The following Disgo implementation resolves this issue entirely.

This test consistently passes at 50 requests per second while attempting to send 101: https://github.com/switchupcb/disgo/blob/v10/wrapper/tests/integration/ratelimit_test.go

@LordOfPolls LordOfPolls deleted the global-rl branch November 14, 2022 09:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug::Library A bug in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants