From 9d2d60691eb4bde729f40fb633ae257cf5bc6545 Mon Sep 17 00:00:00 2001 From: Alec Woods Date: Wed, 31 Mar 2021 15:56:58 -0400 Subject: [PATCH] feat(Rest): better handling of global rate limit and invalid request tracking (#4711) Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com> Co-authored-by: Tristan Guichaoua <33934311+tguichaoua@users.noreply.github.com> Co-authored-by: Papaia <43409674+Papaia@users.noreply.github.com> Co-authored-by: Vlad Frangu --- src/client/Client.js | 6 ++ src/rest/RESTManager.js | 5 +- src/rest/RequestHandler.js | 137 ++++++++++++++++++++++++++++++------- src/util/Constants.js | 8 +++ typings/index.d.ts | 10 +++ 5 files changed, 139 insertions(+), 27 deletions(-) diff --git a/src/client/Client.js b/src/client/Client.js index 96bf271f4..c3b8c516e 100644 --- a/src/client/Client.js +++ b/src/client/Client.js @@ -471,6 +471,9 @@ class Client extends BaseClient { if (typeof options.messageSweepInterval !== 'number' || isNaN(options.messageSweepInterval)) { throw new TypeError('CLIENT_INVALID_OPTION', 'messageSweepInterval', 'a number'); } + if (typeof options.invalidRequestWarningInterval !== 'number' || isNaN(options.invalidRequestWarningInterval)) { + throw new TypeError('CLIENT_INVALID_OPTION', 'invalidRequestWarningInterval', 'a number'); + } if (!Array.isArray(options.partials)) { throw new TypeError('CLIENT_INVALID_OPTION', 'partials', 'an Array'); } @@ -480,6 +483,9 @@ class Client extends BaseClient { if (typeof options.restRequestTimeout !== 'number' || isNaN(options.restRequestTimeout)) { throw new TypeError('CLIENT_INVALID_OPTION', 'restRequestTimeout', 'a number'); } + if (typeof options.restGlobalRateLimit !== 'number' || isNaN(options.restGlobalRateLimit)) { + throw new TypeError('CLIENT_INVALID_OPTION', 'restGlobalRateLimit', 'a number'); + } if (typeof options.restSweepInterval !== 'number' || isNaN(options.restSweepInterval)) { throw new TypeError('CLIENT_INVALID_OPTION', 'restSweepInterval', 'a number'); } diff --git a/src/rest/RESTManager.js b/src/rest/RESTManager.js index 799f60221..a9487fd92 100644 --- a/src/rest/RESTManager.js +++ b/src/rest/RESTManager.js @@ -13,7 +13,10 @@ class RESTManager { this.handlers = new Collection(); this.tokenPrefix = tokenPrefix; this.versioned = true; - this.globalTimeout = null; + this.globalLimit = client.options.restGlobalRateLimit > 0 ? client.options.restGlobalRateLimit : Infinity; + this.globalRemaining = this.globalLimit; + this.globalReset = null; + this.globalDelay = null; if (client.options.restSweepInterval > 0) { client.setInterval(() => { this.handlers.sweep(handler => handler._inactive); diff --git a/src/rest/RequestHandler.js b/src/rest/RequestHandler.js index 245a285cd..d5d729501 100644 --- a/src/rest/RequestHandler.js +++ b/src/rest/RequestHandler.js @@ -4,7 +4,7 @@ const AsyncQueue = require('./AsyncQueue'); const DiscordAPIError = require('./DiscordAPIError'); const HTTPError = require('./HTTPError'); const { - Events: { RATE_LIMIT }, + Events: { RATE_LIMIT, INVALID_REQUEST_WARNING }, } = require('../util/Constants'); const Util = require('../util/Util'); @@ -21,6 +21,15 @@ function calculateReset(reset, serverDate) { return new Date(Number(reset) * 1000).getTime() - getAPIOffset(serverDate); } +/* Invalid request limiting is done on a per-IP basis, not a per-token basis. + * The best we can do is track invalid counts process-wide (on the theory that + * users could have multiple bots run from one process) rather than per-bot. + * Therefore, store these at file scope here rather than in the client's + * RESTManager object. + */ +let invalidCount = 0; +let invalidCountResetTime = null; + class RequestHandler { constructor(manager) { this.manager = manager; @@ -28,7 +37,6 @@ class RequestHandler { this.reset = -1; this.remaining = -1; this.limit = -1; - this.retryAfter = -1; } async push(request) { @@ -40,18 +48,56 @@ class RequestHandler { } } + get globalLimited() { + return this.manager.globalRemaining <= 0 && Date.now() < this.manager.globalReset; + } + + get localLimited() { + return this.remaining <= 0 && Date.now() < this.reset; + } + get limited() { - return Boolean(this.manager.globalTimeout) || (this.remaining <= 0 && Date.now() < this.reset); + return this.globalLimited || this.localLimited; } get _inactive() { return this.queue.remaining === 0 && !this.limited; } + globalDelayFor(ms) { + return new Promise(resolve => { + this.manager.client.setTimeout(() => { + this.manager.globalDelay = null; + resolve(); + }, ms); + }); + } + async execute(request) { - // After calculations and requests have been done, pre-emptively stop further requests - if (this.limited) { - const timeout = this.reset + this.manager.client.options.restTimeOffset - Date.now(); + /* + * After calculations have been done, pre-emptively stop further requests + * Potentially loop until this task can run if e.g. the global rate limit is hit twice + */ + while (this.limited) { + const isGlobal = this.globalLimited; + let limit, timeout, delayPromise; + + if (isGlobal) { + // Set the variables based on the global rate limit + limit = this.manager.globalLimit; + timeout = this.manager.globalReset + this.manager.client.options.restTimeOffset - Date.now(); + // If this is the first task to reach the global timeout, set the global delay + if (!this.manager.globalDelay) { + // The global delay function should clear the global delay state when it is resolved + this.manager.globalDelay = this.globalDelayFor(timeout); + } + delayPromise = this.manager.globalDelay; + } else { + // Set the variables based on the route-specific rate limit + limit = this.limit; + timeout = this.reset + this.manager.client.options.restTimeOffset - Date.now(); + delayPromise = Util.delayFor(timeout); + } if (this.manager.client.listenerCount(RATE_LIMIT)) { /** @@ -63,24 +109,29 @@ class RequestHandler { * @param {string} rateLimitInfo.method HTTP method used for request that triggered this event * @param {string} rateLimitInfo.path Path used for request that triggered this event * @param {string} rateLimitInfo.route Route used for request that triggered this event + * @param {boolean} rateLimitInfo.global Whether the rate limit that was reached was the global limit */ this.manager.client.emit(RATE_LIMIT, { timeout, - limit: this.limit, + limit, method: request.method, path: request.path, route: request.route, + global: isGlobal, }); } - if (this.manager.globalTimeout) { - await this.manager.globalTimeout; - } else { - // Wait for the timeout to expire in order to avoid an actual 429 - await Util.delayFor(timeout); - } + // Wait for the timeout to expire in order to avoid an actual 429 + await delayPromise; // eslint-disable-line no-await-in-loop } + // As the request goes out, update the global usage information + if (!this.manager.globalReset || this.manager.globalReset < Date.now()) { + this.manager.globalReset = Date.now() + 1000; + this.manager.globalRemaining = this.manager.globalLimit; + } + this.manager.globalRemaining--; + // Perform the request let res; try { @@ -95,33 +146,64 @@ class RequestHandler { return this.execute(request); } + let sublimitTimeout; if (res && res.headers) { const serverDate = res.headers.get('date'); const limit = res.headers.get('x-ratelimit-limit'); const remaining = res.headers.get('x-ratelimit-remaining'); const reset = res.headers.get('x-ratelimit-reset'); - const retryAfter = res.headers.get('retry-after'); - this.limit = limit ? Number(limit) : Infinity; this.remaining = remaining ? Number(remaining) : 1; this.reset = reset ? calculateReset(reset, serverDate) : Date.now(); - this.retryAfter = retryAfter ? Number(retryAfter) * 1000 : -1; // https://github.com/discordapp/discord-api-docs/issues/182 if (request.route.includes('reactions')) { this.reset = new Date(serverDate).getTime() - getAPIOffset(serverDate) + 250; } - // Handle global ratelimit - if (res.headers.get('x-ratelimit-global')) { - // Set the manager's global timeout as the promise for other requests to "wait" - this.manager.globalTimeout = Util.delayFor(this.retryAfter); + // Handle retryAfter, which means we have actually hit a rate limit + let retryAfter = res.headers.get('retry-after'); + retryAfter = retryAfter ? Number(retryAfter) * 1000 : -1; + if (retryAfter > 0) { + // If the global ratelimit header is set, that means we hit the global rate limit + if (res.headers.get('x-ratelimit-global')) { + this.manager.globalRemaining = 0; + this.manager.globalReset = Date.now() + retryAfter; + } else if (!this.localLimited) { + /* + * This is a sublimit (e.g. 2 channel name changes/10 minutes) since the headers don't indicate a + * route-wide rate limit. Don't update remaining or reset to avoid rate limiting the whole + * endpoint, just set a reset time on the request itself to avoid retrying too soon. + */ + sublimitTimeout = retryAfter; + } + } + } - // Wait for the global timeout to resolve before continuing - await this.manager.globalTimeout; + // Count the invalid requests + if (res.status === 401 || res.status === 403 || res.status === 429) { + if (!invalidCountResetTime || invalidCountResetTime < Date.now()) { + invalidCountResetTime = Date.now() + 1000 * 60 * 10; + invalidCount = 0; + } + invalidCount++; - // Clean up global timeout - this.manager.globalTimeout = null; + const emitInvalid = + this.manager.client.listenerCount(INVALID_REQUEST_WARNING) && + this.manager.client.options.invalidRequestWarningInterval > 0 && + invalidCount % this.manager.client.options.invalidRequestWarningInterval === 0; + if (emitInvalid) { + /** + * Emitted periodically when the process sends invalid messages to let users avoid the + * 10k invalid requests in 10 minutes threshold that causes a ban + * @event Client#invalidRequestWarning + * @param {number} invalidRequestWarningInfo.count Number of invalid requests that have been made in the window + * @param {number} invalidRequestWarningInfo.remainingTime Time in ms remaining before the count resets + */ + this.manager.client.emit(INVALID_REQUEST_WARNING, { + count: invalidCount, + remainingTime: invalidCountResetTime - Date.now(), + }); } } @@ -136,8 +218,11 @@ class RequestHandler { // Handle ratelimited requests if (res.status === 429) { // A ratelimit was hit - this should never happen - this.manager.client.emit('debug', `429 hit on route ${request.route}`); - await Util.delayFor(this.retryAfter); + this.manager.client.emit('debug', `429 hit on route ${request.route}${sublimitTimeout ? ' for sublimit' : ''}`); + // If caused by a sublimit, wait it out here so other requests on the route can be handled + if (sublimitTimeout) { + await Util.delayFor(sublimitTimeout); + } return this.execute(request); } diff --git a/src/util/Constants.js b/src/util/Constants.js index 3f4d7dcbe..cc9786042 100644 --- a/src/util/Constants.js +++ b/src/util/Constants.js @@ -19,6 +19,9 @@ const { Error, RangeError } = require('../errors'); * @property {number} [messageSweepInterval=0] How frequently to remove messages from the cache that are older than * the message cache lifetime (in seconds, 0 for never) * @property {MessageMentionOptions} [allowedMentions] Default value for {@link MessageOptions#allowedMentions} + * @property {number} [invalidRequestWarningInterval=0] The number of invalid REST requests (those that return + * 401, 403, or 429) in a 10 minute window between emitted warnings (0 for no warnings). That is, if set to 500, + * warnings will be emitted at invalid request number 500, 1000, 1500, and so on. * @property {PartialType[]} [partials] Structures allowed to be partial. This means events can be emitted even when * they're missing all the data for a particular structure. See the "Partials" topic listed in the sidebar for some * important usage information, as partials require you to put checks in place when handling data. @@ -29,6 +32,8 @@ const { Error, RangeError } = require('../errors'); * @property {number} [restRequestTimeout=15000] Time to wait before cancelling a REST request, in milliseconds * @property {number} [restSweepInterval=60] How frequently to delete inactive request buckets, in seconds * (or 0 for never) + * @property {number} [restGlobalRateLimit=0] How many requests to allow sending per second (0 for unlimited, 50 for + * the standard global limit used by Discord) * @property {number} [retryLimit=1] How many times to retry on 5XX errors (Infinity for indefinite amount of retries) * @property {PresenceData} [presence={}] Presence data to use upon login * @property {IntentsResolvable} intents Intents to enable for this connection @@ -40,9 +45,11 @@ exports.DefaultOptions = { messageCacheMaxSize: 200, messageCacheLifetime: 0, messageSweepInterval: 0, + invalidRequestWarningInterval: 0, partials: [], restWsBridgeTimeout: 5000, restRequestTimeout: 15000, + restGlobalRateLimit: 0, retryLimit: 1, restTimeOffset: 500, restSweepInterval: 60, @@ -218,6 +225,7 @@ exports.VoiceOPCodes = { exports.Events = { RATE_LIMIT: 'rateLimit', + INVALID_REQUEST_WARNING: 'invalidRequestWarning', CLIENT_READY: 'ready', GUILD_CREATE: 'guildCreate', GUILD_DELETE: 'guildDelete', diff --git a/typings/index.d.ts b/typings/index.d.ts index 2f0935c82..dbe0877ab 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -376,6 +376,7 @@ declare module 'discord.js' { }; Events: { RATE_LIMIT: 'rateLimit'; + INVALID_REQUEST_WARNING: 'invalidRequestWarning'; CLIENT_READY: 'ready'; RESUMED: 'resumed'; GUILD_CREATE: 'guildCreate'; @@ -2424,6 +2425,7 @@ declare module 'discord.js' { messageUpdate: [oldMessage: Message | PartialMessage, newMessage: Message | PartialMessage]; presenceUpdate: [oldPresence: Presence | undefined, newPresence: Presence]; rateLimit: [rateLimitData: RateLimitData]; + invalidRequestWarning: [invalidRequestWarningData: InvalidRequestWarningData]; ready: []; invalidated: []; roleCreate: [role: Role]; @@ -2447,10 +2449,12 @@ declare module 'discord.js' { messageCacheLifetime?: number; messageSweepInterval?: number; allowedMentions?: MessageMentionOptions; + invalidRequestWarningInterval?: number; partials?: PartialTypes[]; restWsBridgeTimeout?: number; restTimeOffset?: number; restRequestTimeout?: number; + restGlobalRateLimit?: number; restSweepInterval?: number; retryLimit?: number; presence?: PresenceData; @@ -3209,6 +3213,12 @@ declare module 'discord.js' { method: string; path: string; route: string; + global: boolean; + } + + interface InvalidRequestWarningData { + count: number; + remainingTime: number; } interface RawOverwriteData {