discord / discord-api-docs

Official Discord API Documentation
https://discord.com/developers/docs/intro
Other
5.9k stars 1.25k forks source link

[Libraries] Rate limit compliant #108

Closed jhgg closed 7 years ago

jhgg commented 8 years ago

We're going to start delisting API libraries that are not properly implementing rate limits. The whole "get 429ed and then retry" method doesn't work that well, as it seems this is incredibly prone to getting the bot in a cascading failure mode that just spams the API server. We don't want to keep having to ban bots every day who are getting stuck in these loops (it's becoming a bit of a problem) - so we are going to start discouraging the use of libraries (either by delisting them - or putting a disclaimer/warning until the author implements this feature properly) that do not properly throttle requests. This means that the client should be aware that it is being rate limited on a given method, and not attempt to send any requests during a period that it should know would be immediately rate limited. Please refer to our new docs on Rate Limiting.

Below is a list of libraries that are known to do queuing/throttling properly:

Library authors, please comment here letting me know if your lib does implement these properly, and perhaps with a link to the source code so we can take a quick look and make sure it looks right.

meew0 commented 8 years ago

discordrb, whose REST API calls are sync but usually called from multithreaded code, uses mutexes to lock API calls after a 429 happens. The most relevant piece of code is this:

https://github.com/meew0/discordrb/blob/master/lib/discordrb/api.rb#L67

This means that after a call fails once, any further calls during retry_after will be prevented and instead sent after the period ends. (If too many calls are bunched up and later ones end up rate limited again, it will prevent those as well, starting the cycle anew.)

discordrb relies on knowing what API calls are rate limited in what way, but it doesn't rely on knowing the exact bucket sizes. By default it assumes every request is rate limited - it doesn't matter if a 429 never happens. More specific things like message sending where the rate limiting happens per-server are cared for in the code for the specific endpoint (although the global rate limit in this specific case isn't implemented; however this should not be a problem as the lib will implicitly treat occurring global rate limits like all individual rate limits happening at once).

night commented 8 years ago

knowing the exact bucket sizes.

we are discussing this already, and will be exposing headers soon

Auralytical commented 8 years ago

Discord.Net 0.9 throttles individual requests on receiving 429s 1.0 (in dev) has pre-emptive limits with throttling buckets on 429 as a backup, and request timeouts.

1.0 relies on knowing bucket info (both sizes and intervals) though for pre-emptive to work, otherwise it can trigger multiple 429s at once due to async calls and latency.

EDIT: https://github.com/RogueException/Discord.Net/blob/dev/src/Discord.Net/Net/Queue/RequestQueueBucket.cs#L43-L109

SinisterRectus commented 8 years ago

The current version of Discordia backs off after receiving a 429. Due to an oversight, users can still make API calls from a coroutine that is different from the one that experienced the 429.

Discordia 1.0 (unfinished) features a RateLimiter class that keeps track of previous X calls and how much time had elapsed between them, and holds back calls if there were too many in the previous Y milliseconds, regardless of where the calls were made. The library will also handle 429s, although they are not expected to occur if the rate limiters do their job. I hope to release 1.0 before the end of July.

This is where the request is made.

abalabahaha commented 8 years ago

Eris currently tracks ratelimits (hard-coded bucket sizes and intervals) and blocks them client-side, so there are very little (if any) 429s unless Discord changes something.

discord.js currently holds requests when it sees a 429, and waits for retry_after. However, it makes async requests, so multiple requests can go through at once before one returns a 429. There are plans to make this better in the future.

EDIT: Eris: around https://github.com/abalabahaha/eris/blob/dev/lib/Client.js#L1276 discord.js: around https://github.com/hydrabolt/discord.js/blob/master/src/Client/InternalClient.js#L94

bwmarrin commented 8 years ago

discordgo develop branch... keeps a "bucket" per endpoint url and anytime a given endpoint gets a 429 it locks that "bucket" so no requests can be sent to it until after retry_after time. This allows it to handle any rate limit changes Discord makes but it probably doesn't match perfectly with the discord buckets.

The master branch doesn't ratelimit - should be updating dev to master soon though.

https://github.com/bwmarrin/discordgo/blob/develop/restapi.go#L52 https://github.com/bwmarrin/discordgo/blob/master/restapi.go#L52

EDIT: Also, as a side note. Because of how this works it will only send requests to a endpoint one at a time.

DatBlindArcher commented 8 years ago

DiscordUnity stops requests directly after the first 429 exception. It holds the socket for the amount of ms given in the retry after variable. (probably even a for a few ms longer because of how unity works) I haven't had a case yet where a request got through after a 429, even when I was spamming lists of requests, like it's supposed to work. Maybe in the future I will change it to work seperatly for each bucket, now it just blocks all.

DiscordUnity: https://github.com/robinhood128/DiscordUnity/blob/master/src/DiscordClient.cs#L1023

davidcole1340 commented 8 years ago

DiscordPHP Version 3 is synchronous so 429 blocks stop all other code from executing: https://github.com/teamreflex/DiscordPHP/blob/master/src/Discord/Helpers/Guzzle.php#L126-L130

DiscordPHP Version 4 is now asynchronous, however once receiving a 429 it stops all other requests until the 429 duration has been slept. Currently working on implementing buckets to pre-rate-limit.

austinv11 commented 8 years ago

Discord4J throws a RateLimitException upon hitting a 429 response code from any REST endpoint (discord or not) via my Requests class. This means that bot devs have to account for RateLimitExceptions otherwise their bot will not compile. Additionally, Discord4J provides a utility class (RequestBuffer) which handles these rate limits automatically for the user by queueing requests to retry only after the retry_after interval provided by discord.

satom99 commented 8 years ago

litcord sleeps given Retry-After interval and sets a variable to true to know whether requests done during this period should also sleep. Once it is over, variable is set to false and requests on hold are executed, including the errored one.

https://github.com/satom99/litcord/blob/master/litcord/client/rest.lua#L82 https://github.com/satom99/litcord/blob/master/litcord/client/rest.lua#L21

SinisterRectus commented 8 years ago

This means that the client should be aware that it is being rate limited on a given bucket, and not attempt to send any requests during that period that it should know would be immediately rate limited.

I have a question about what the protocol is for hitting not one, but multiple 429s. My understanding is that it is okay to receive a 429 as long as no requests specific to the rate-limited endpoint are made that will surely receive the same 429 during the provided retry-after time.

What if a client tries to send 100 messages in 5 seconds? Even if the client is sure to not send a request during the known retry-after time, it will still receive a 429 approximately every 5 seconds while it continues to release all of its queued messages.

Is it okay to continuously hit a 429 in this manner?

night commented 8 years ago

Rate limits have been improved. Please see the docs: https://github.com/hammerandchisel/discord-api-docs/blob/master/docs/topics/RATE_LIMITS.md

When your lib supports the new rate limits headers comment and we will validate your lib.

austinv11 commented 8 years ago

Discord4J as of version 2.5.3 supports the new rate limit headers and will pre-emptively ratelimit when possible and it will track endpoints to prevent successive 429s from firing. Relevant class: https://github.com/austinv11/Discord4J/blob/2.5.3/src/main/java/sx/blah/discord/api/internal/Requests.java#L79

b1naryth1ef commented 8 years ago

dscord latest is compliant as of this commit

b1naryth1ef commented 8 years ago

As an FYI, we plan on adjusting the recommended library list on September 20th. Libs that do not have compliant implementations by that time will be removed.

amishshah commented 8 years ago

Is it possible to change the X-RateLimit-Remaining header to a Retry-After form? Bots that are a second or two out of sync will keep hitting 429s otherwise.

night commented 8 years ago

X-RateLimit-Reset is an epoch time of when the rate limit resets on the method. Additionally, if your time is out of date you should consider installing ntpd.

Edit: I should also note that there is a Date header returned from the server if you want to depend on that too.

amishshah commented 8 years ago

@night but wouldn't it be more consistent to keep Retry-After instead of X-RateLimit-Reset? Since 429 responses include a Retry-After, it would make sense if all requests did too, instead of a timestamp...

austinv11 commented 8 years ago

I understand the X-RateLimit-Reset and its purpose, but I don't understand why ratelimit headers are inconsistent given that when you receive a 429 there's a retry after period. It would also make it more user friendly since I doubt most bot makers will know that the reason their library's rate limit measures are failing are due to their system configuration.

night commented 8 years ago

Our Rate Limit headers are pretty standard to most APIs, so I think they are pretty reasonable to expect from us too.

austinv11 commented 8 years ago

@night I feel like your comparison is a fallacy. Being content with something just because others do it doesn't necessarily make it the best option.

amishshah commented 8 years ago

discord.js (development) supports the new Rate Limit headers, see here

davidcole1340 commented 8 years ago

discordphp did it here https://github.com/teamreflex/DiscordPHP/commit/56d388ba0a14571745514f2adf59c59f1b110520

satom99 commented 8 years ago

litcord's indev branch now supports the new rate limits.

SpaceManiac commented 8 years ago

discord-rs has implemented the new rate limit handling, hopefully correctly...

nopjmp commented 8 years ago

I'm showing that discord.py has this implemented in this commit.

abalabahaha commented 8 years ago

@nopjmp that listens to the retry_after key returned in the JSON response of a 429. These headers are different from retry-after

meew0 commented 8 years ago

discordrb handles the new rate limits now, although the changes are unreleased. Here are the relevant changes, here is the whole relevant file.

(These changes are currently unreleased; if necessary I will comment again once I released this.)

Marqin commented 8 years ago

Below is a list of libraries that are known to do queuing/throttling properly

I've digged into code of dscord and Discord4j that are marked as properly and it looks that both of them are not implementig it properly (but also it's not documented how it's really properly, please, respond to #128):

I've not digged into other libraries.

Marqin commented 8 years ago

All of those problems are because docs say

Rate limits are applied on a per-method basis

but doesn't say in which case it applies to not-compiled endpoint and when to compiled URL with IDs. Please exaplain this if you want us to implement RateLimit in our libraries.

austinv11 commented 8 years ago

@Marqin it's unfair to say that d4j does it improperly as if you had done the research, you would see that I fixed that awhile ago (as of this commit).

b1naryth1ef commented 8 years ago

I think there is some confusion based on the fact that whats in the current rate limit documentation is not entirely accurate. We adjusted things internally to conform with our original plans for rate limiting, and those things haven't been fully expressed in the documentation yet. We'll wait to review libs until @night has finished and published the final version of those docs.

DV8FromTheWorld commented 7 years ago

As of now, the 3.0 branch of JDA now supports Ratelimit headers based on the expanded, unmerged ratelimit documentation in the rate-limit-docs branch of this repository. https://github.com/hammerandchisel/discord-api-docs/commit/7e5f326cd0a1b3fa8397ba94f2b9fd9f565cb3a6 Would be nice if these changes could be merged into master so that the official docs has the proper information considering, technically, today is the deadline.

All of JDA's REST related requests pass through the Requester which handles the actual execution of a Request. The Requester class uses either BotRateLimiter or ClientRateLimiter depending on which type of account is logged in. In both situations, the Requester queries the Ratelimiter to determine if the Route is currently ratelimited before execution and handles header or 429 updating as needed after execution. Additionally, the BotRateLimiter takes advantage of the date header provided in the response to sync local time with Discord's server time in the case of incorrect time on the local machine.

Marqin commented 7 years ago

They have merged it.

b1naryth1ef commented 7 years ago

Alright lads & lasses, its that time. The official complete(ish) version of the new rate limit documentation is live here. I implore you to read DV8's implementations (see two comments above), or the dscord implementation if you have questions or want code examples.

We'll be setting the deadline for compliant implementations at October 7th (two weeks). Please post on on this thread, with a link to the source implementation of rate limits, along with a small description of how they where implemented. Implementations must be merged into a mainline/release branch/version by October 7th (although this is not a requirement for review). As a reminder, non-compliant libraries risk the following:

If you have any questions, please ask here or on the Discord API server (#docs channel is fine).

austinv11 commented 7 years ago

Discord4J has updated its implementation to reflect the clarification to the docs as of this commit.

meew0 commented 7 years ago

I've merged the discordrb implementation into master now, and modified it to account for night's PR.

It handles rate limits using mutexes: there is a hash of mutexes, one for each [route, major_parameter] pair, and before doing any request, the method waits for the particular mutex to unlock. If the request returns a 429, or if the X-RateLimit-Remaining header has been found to be zero, the mutex is locked for retry_after milliseconds (in case of a 429) or for the difference between the X-RateLimit-Reset and Date headers (otherwise). In the former case, the request that caused the mutex to be locked will be retried after the time runs out.

There is also a separate global mutex which is checked the same way before a request, and which is locked instead of the specific route/param mutex if the X-RateLimit-Global header is true.

The method that does the actual rate limit handling

Example of a method implementing a specific endpoint

This implementation should be released soon; your message required that the implementation is merged into a mainline branch, not that it is released. Tell me if I should comment here again once it's released.

amishshah commented 7 years ago

As of now, discord.js's indev branch supports the new rate-limiting, see https://github.com/hydrabolt/discord.js/commit/aef0b83c34bea1ea04b8938cd4845b67034ae497

amishshah commented 7 years ago

discord.js's support for the new rate-limiting has been released on NPM now as 9.3.0

meew0 commented 7 years ago

discordrb's support for the new changes has been released to RubyGems as version 3.0.0.

Auralytical commented 7 years ago

Discord.Net supports the new system as of 1.0b2

meew0 commented 7 years ago

discordcr has implemented the rate limits in much the same way as discordrb and should also be compliant.

abalabahaha commented 7 years ago

Eris supports the new system as of 0.4.0 on NPM

b1naryth1ef commented 7 years ago

Done the best I can to audit all the libraries for people who've implemented the rate limits, closing this now. If any of y'all who missed the deadline finish your integrations/implementations, please open a pull request adding your lib w/ details on its rate limiting implementation.