Skip to content
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

[Libraries] Rate limit compliant #108

Closed
11 of 20 tasks
jhgg opened this issue Jul 18, 2016 · 44 comments
Closed
11 of 20 tasks

[Libraries] Rate limit compliant #108

jhgg opened this issue Jul 18, 2016 · 44 comments

Comments

@jhgg
Copy link
Member

jhgg commented Jul 18, 2016

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:

  • Discord.Net
  • DiscordSharp
  • dscord
  • Discordgo
  • Discord4J
  • Javacord
  • JDA
  • Discordia
  • litcord
  • discordie
  • discord.io
  • discord.js
  • Eris
  • DiscordPHP
  • discord.py
  • discordrb
  • discord-rs
  • DiscordUnity
  • disco
  • discordcr

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
Copy link
Contributor

meew0 commented Jul 18, 2016

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
Copy link
Contributor

night commented Jul 18, 2016

knowing the exact bucket sizes.

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

@Auralytical
Copy link
Contributor

Auralytical commented Jul 18, 2016

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
Copy link
Contributor

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
Copy link
Contributor

abalabahaha commented Jul 18, 2016

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
Copy link

bwmarrin commented Jul 18, 2016

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
Copy link

DatBlindArcher commented Jul 18, 2016

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

satom99 commented Jul 20, 2016

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
Copy link
Contributor

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
Copy link
Contributor

night commented Aug 12, 2016

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
Copy link
Contributor

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
Copy link
Contributor

dscord latest is compliant as of this commit

@b1naryth1ef
Copy link
Contributor

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
Copy link
Contributor

amishshah commented Aug 19, 2016

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
Copy link
Contributor

night commented Aug 19, 2016

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

night commented Aug 19, 2016

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

@austinv11
Copy link
Contributor

@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 added a commit to discordjs/discord.js that referenced this issue Aug 19, 2016
@amishshah
Copy link
Contributor

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

@davidcole1340
Copy link
Contributor

discordphp did it here discord-php/DiscordPHP@56d388b

@satom99
Copy link

satom99 commented Aug 20, 2016

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

@SpaceManiac
Copy link

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

@meew0
Copy link
Contributor

meew0 commented Aug 28, 2016

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
Copy link

Marqin commented Sep 6, 2016

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):

  • dscord - they make map of not-compiled routes, so when sending to two different channels it counts as one and can trigger 429
  • Discord4j - they just make map where key is request-method, not URLs nor routes, so obvious 429

I've not digged into other libraries.

@Marqin
Copy link

Marqin commented Sep 6, 2016

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member

DV8FromTheWorld commented Sep 21, 2016

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. 7e5f326 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
Copy link

Marqin commented Sep 21, 2016

They have merged it.

@b1naryth1ef
Copy link
Contributor

b1naryth1ef commented Sep 23, 2016

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:

  • Removal from our officially sanctioned list of libraries
  • Notifications to users of the libraries that they are using outdated/non-compliant libraries
  • Bots causing rate limiting issues from non-compliant libraries/versions may be blocked, and required to upgrade

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

@austinv11
Copy link
Contributor

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

@meew0
Copy link
Contributor

meew0 commented Sep 24, 2016

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 added a commit to discordjs/discord.js that referenced this issue Sep 24, 2016
@amishshah
Copy link
Contributor

As of now, discord.js's indev branch supports the new rate-limiting, see discordjs/discord.js@aef0b83

@amishshah
Copy link
Contributor

amishshah commented Sep 24, 2016

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

@meew0
Copy link
Contributor

meew0 commented Sep 30, 2016

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

@Auralytical
Copy link
Contributor

Discord.Net supports the new system as of 1.0b2

@meew0
Copy link
Contributor

meew0 commented Oct 13, 2016

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

@abalabahaha
Copy link
Contributor

Eris supports the new system as of 0.4.0 on NPM

@b1naryth1ef
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests