feat(http): improve HTTPClient rate limiting behavior #1084
Conversation
Signed-off-by: Alex Schoenhofen <[email protected]>
Signed-off-by: Alex Schoenhofen <[email protected]>
Signed-off-by: Alex Schoenhofen <[email protected]>
alentoghostflame
changed the title
| if exc.status == 401: | ||
| raise LoginFailure("Improper token has been passed.") from exc | ||
| raise | ||
|
|
||
| return data | ||
|
|
||
| async def exchange_access_code( |
There was a problem hiding this comment.
This new method and all the other OAuth2 related changes should be dropped if this PR just focuses on rate limiting.
|
|
||
| loop = asyncio.get_running_loop() | ||
| _log.debug("Bucket %s: Resetting after %s seconds.", self.bucket, self.reset_after) | ||
| self._reset_remaining_task = loop.create_task(self.reset_remaining(self.reset_after)) |
There was a problem hiding this comment.
What are the differences between this and using loop.call_later?
shiftinv
added
s: in progress
Co-authored-by: Emre Terzioglu <[email protected]> Signed-off-by: Alex Schoenhofen <[email protected]>
…h commit. Signed-off-by: Alex Schoenhofen <[email protected]>
…, add retry_request kwarg to HTTPClient.request() Signed-off-by: Alex Schoenhofen <[email protected]>
…ed on something that it was cool with earlier? Signed-off-by: Alex Schoenhofen <[email protected]>
| if self.bucket == x_bucket: | ||
| pass # Don't need to set it again. | ||
| elif self.bucket is None: | ||
| self.bucket = x_bucket | ||
| else: | ||
| raise IncorrectBucket( | ||
| f"Update given for bucket {x_bucket}, but this RateLimit is for bucket {self.bucket}!" | ||
| ) |
There was a problem hiding this comment.
That pass is a smell, couldn't this be written as
if self.bucket is None:
self.bucket = x_bucket
elif self.bucket != x_bucket:
raise ...?
| ... | ||
|
|
||
|
|
||
| class RateLimit: |
There was a problem hiding this comment.
This class seems to reimplement what asyncio provides via primitives other than Event (Semaphore/Condition). See if you can utilize those in the implementation?
There was a problem hiding this comment.
Didn't realize that you commented this, whoops. This is a rather old PR at this point, I would like NC to merge the PR on their end before updating it here.
While I need to look into Condition to see how useful it is here, Semaphores AFAIK are not meant/able to have their limit be dynamically changed. Ideally the limit wouldn't need to be changed after the rate limit info is discovered, but there seem to be some cases where Discord will change the rate limit on you. IIRC for example, the rate limit for deleting new messages is typically 5/5s, but deleting older ones can swap it to a 3/5s.
| @@ -432,6 +432,7 @@ def __init__( | |||
| self._first_connect: asyncio.Event = asyncio.Event() | |||
| self._connection._get_websocket = self._get_websocket | |||
| self._connection._get_client = lambda: self | |||
| self._token: str | None = None | |||
There was a problem hiding this comment.
Since we still support python <= 3.10, we can't use this hint or it will raise an error
self._token: Optional[str] would be more appropriate
There was a problem hiding this comment.
We actually can as long as there is a from __future__ import annotations at the top, but i agree, albeit for a different reason: it would look inconsistent with the rest of the codebase
Summary
A port of Nextcord's HTTP-Recore PR
DUE TO THIS BEING A PORT, I PROBABLY SCREWED SOMETHING UP!
The code in this PR was modeled after NC's
HTTPClient, but I can see that there are some obvious differences between DS's and NC'sHTTPClientcode. While I have tried to keep the changes intact or otherwise adapt them, I'm sure there are more subtle (and major) differences that I missed. If you think that I may have clobbered something, I probably didn't notice the change and did clobber it. Please say something!Massively reworks the core of HTTPClient, adding support for:
asyncio.gather()!Due to the major changes there are breaking changes, notably:
MaybeUnlockhas been removed and replaced with a completely different implementation ofRateLimit,HTTPClient.request()will no longer clobber some given headers, such as auth and user-agent ones,HTTPClient.tokenhas been removed due to auth changes andClient._tokenhas been added,HTTPClient._default_authexists.HTTPClient's attributes are gone with new and old ones being_'d,HTTPClient.static_login()no longer takes a token ("<token>") and instead takes an auth string. ("Bot <token>")I'll finish fleshing out this PR summary well before I undraft it.
Checklist
pdm lintpdm pyright