From 13f46b924be15371f377d3c5f128ac75ac98e4b4 Mon Sep 17 00:00:00 2001 From: Isabella Date: Thu, 23 Aug 2018 23:29:44 -0500 Subject: [PATCH] refactor: clean up rate limit handling (#2694) * refactor: clean up rate limit handling * requested changes * remove request mode option * fix dupe requests * hardcode reaction ratelimits * suggested changes * fix small thing * re-add restTimeOffset * move restTimeOffset a bit * i swear i know english its my native language ok * requested changes * fix: a bit *too* pre-emptive with ratelimits, now less so * fix: dapi error shoudl reject with path * fix: make errors in execute catchable * fix promise return * rebase is hard --- src/rest/HTTPError.js | 31 +++++ src/rest/RESTManager.js | 15 +-- src/rest/RequestHandler.js | 180 ++++++++++++++++++++++++++++ src/rest/handlers/RequestHandler.js | 124 ------------------- src/rest/handlers/burst.js | 15 --- src/rest/handlers/index.js | 5 - src/rest/handlers/sequential.js | 18 --- src/util/Constants.js | 2 - 8 files changed, 215 insertions(+), 175 deletions(-) create mode 100644 src/rest/HTTPError.js create mode 100644 src/rest/RequestHandler.js delete mode 100644 src/rest/handlers/RequestHandler.js delete mode 100644 src/rest/handlers/burst.js delete mode 100644 src/rest/handlers/index.js delete mode 100644 src/rest/handlers/sequential.js diff --git a/src/rest/HTTPError.js b/src/rest/HTTPError.js new file mode 100644 index 000000000..293be7c5c --- /dev/null +++ b/src/rest/HTTPError.js @@ -0,0 +1,31 @@ +class HTTPError extends Error { + constructor(message, name, code, method, path) { + super(message); + + /** + * The name of the error + * @type {string} + */ + this.name = name; + + /** + * HTTP error code returned from the request + * @type {number} + */ + this.code = code || 500; + + /** + * The HTTP method used for the request + * @type {string} + */ + this.method = method; + + /** + * The path of the request relative to the HTTP endpoint + * @type {string} + */ + this.path = path; + } +} + +module.exports = HTTPError; diff --git a/src/rest/RESTManager.js b/src/rest/RESTManager.js index 1d2f66e5e..4b725f4d2 100644 --- a/src/rest/RESTManager.js +++ b/src/rest/RESTManager.js @@ -1,4 +1,4 @@ -const handlers = require('./handlers'); +const RequestHandler = require('./RequestHandler'); const APIRequest = require('./APIRequest'); const routeBuilder = require('./APIRouter'); const { Error } = require('../errors'); @@ -12,6 +12,7 @@ class RESTManager { this.globallyRateLimited = false; this.tokenPrefix = tokenPrefix; this.versioned = true; + this.globalTimeout = null; if (client.options.restSweepInterval > 0) { client.setInterval(() => { this.handlers.sweep(handler => handler._inactive); @@ -41,24 +42,16 @@ class RESTManager { request: apiRequest, resolve, reject, - }); + }).catch(reject); }); } - getRequestHandler() { - const method = this.client.options.apiRequestMethod; - if (typeof method === 'function') return method; - const handler = handlers[method]; - if (!handler) throw new Error('RATELIMIT_INVALID_METHOD'); - return handler; - } - request(method, url, options = {}) { const apiRequest = new APIRequest(this, method, url, options); let handler = this.handlers.get(apiRequest.route); if (!handler) { - handler = new handlers.RequestHandler(this, this.getRequestHandler()); + handler = new RequestHandler(this); this.handlers.set(apiRequest.route, handler); } diff --git a/src/rest/RequestHandler.js b/src/rest/RequestHandler.js new file mode 100644 index 000000000..1e3128bd0 --- /dev/null +++ b/src/rest/RequestHandler.js @@ -0,0 +1,180 @@ +const DiscordAPIError = require('./DiscordAPIError'); +const HTTPError = require('./HTTPError'); +const Util = require('../util/Util'); +const { Events: { RATE_LIMIT }, browser } = require('../util/Constants'); + +function parseResponse(res) { + if (res.headers.get('content-type').startsWith('application/json')) return res.json(); + if (browser) return res.blob(); + return res.buffer(); +} + +function getAPIOffset(serverDate) { + return new Date(serverDate).getTime() - Date.now(); +} + +function calculateReset(reset, serverDate) { + return new Date(Number(reset) * 1000).getTime() - getAPIOffset(serverDate); +} + +class RequestHandler { + constructor(manager) { + this.manager = manager; + this.busy = false; + this.queue = []; + this.reset = -1; + this.remaining = -1; + this.limit = -1; + this.retryAfter = -1; + } + + push(request) { + if (this.busy) { + this.queue.push(request); + return this.run(); + } else { + return this.execute(request); + } + } + + run() { + if (this.queue.length === 0) return Promise.resolve(); + return this.execute(this.queue.shift()); + } + + get limited() { + return (this.manager.globallyRateLimited || this.remaining <= 0) && Date.now() < this.reset; + } + + get _inactive() { + return this.queue.length === 0 && !this.limited && this.busy !== true; + } + + /* eslint-disable-next-line complexity */ + async execute(item) { + // Insert item back to the beginning if currently busy + if (this.busy) { + this.queue.unshift(item); + return null; + } + + this.busy = true; + const { reject, request, resolve } = item; + + // 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(); + + if (this.manager.client.listenerCount(RATE_LIMIT)) { + /** + * Emitted when the client hits a rate limit while making a request + * @event Client#rateLimit + * @param {Object} rateLimitInfo Object containing the rate limit info + * @param {number} rateLimitInfo.timeout Timeout in ms + * @param {number} rateLimitInfo.limit Number of requests that can be made to this endpoint + * @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 + */ + this.manager.client.emit(RATE_LIMIT, { + timeout, + limit: this.limit, + method: request.method, + path: request.path, + route: request.route, + }); + } + + if (this.manager.globallyRateLimited && !this.manager.globalTimeout) { + // Set a global rate limit for all of the handlers instead of each one individually + this.manager.globalTimeout = this.manager.client.setTimeout(() => { + this.manager.globalTimeout = null; + this.manager.globallyRateLimited = false; + this.busy = false; + this.run(); + }, timeout); + } else if (this.manager.globalTimeout) { + // Already waiting for a global rate limit to clear + this.queue.unshift(item); + return null; + } else { + // Wait for the timeout to expire in order to avoid an actual 429 + await Util.delayFor(timeout); + } + } + + // Perform the request + let res; + try { + res = await request.make(); + } catch (error) { + // NodeFetch error expected for all "operational" errors, such as 500 status code + this.busy = false; + return reject( + new HTTPError(error.message, error.constructor.name, error.status, request.method, request.route) + ); + } + + if (res && res.headers) { + if (res.headers.get('x-ratelimit-global')) this.manager.globallyRateLimited = true; + + 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) : -1; + + // https://github.com/discordapp/discord-api-docs/issues/182 + if (item.request.route.includes('reactions')) { + this.reset = Date.now() + getAPIOffset(serverDate) + 250; + } + } + + // Finished handling headers, safe to unlock manager + this.busy = false; + + if (res.ok) { + const success = await parseResponse(res); + // Nothing wrong with the request, proceed with the next + resolve(success); + return this.run(); + } else if (res.status === 429) { + // A ratelimit was hit - this should never happen + this.queue.unshift(item); + this.manager.client.emit('debug', `429 hit on route ${item.request.route}`); + await Util.delayFor(this.retryAfter); + return this.run(); + } else if (res.status >= 500 && res.status < 600) { + // Retry once for possible serverside issues + if (item.retried) { + return reject( + new HTTPError(res.statusText, res.constructor.name, res.status, item.request.method, request.route) + ); + } else { + item.retried = true; + this.queue.unshift(item); + return this.run(); + } + } else { + // Handle possible malformed requessts + try { + const data = await parseResponse(res); + if (res.status >= 400 && res.status < 500) { + return reject(new DiscordAPIError(request.path, data, request.method)); + } + return null; + } catch (err) { + return reject( + new HTTPError(err.message, err.constructor.name, err.status, request.method, request.route) + ); + } + } + } +} + +module.exports = RequestHandler; diff --git a/src/rest/handlers/RequestHandler.js b/src/rest/handlers/RequestHandler.js deleted file mode 100644 index 464c71781..000000000 --- a/src/rest/handlers/RequestHandler.js +++ /dev/null @@ -1,124 +0,0 @@ -const DiscordAPIError = require('../DiscordAPIError'); -const { Events: { RATE_LIMIT }, browser } = require('../../util/Constants'); - -function parseResponse(res) { - if (res.headers.get('content-type').startsWith('application/json')) return res.json(); - if (browser) return res.blob(); - return res.buffer(); -} - -class RequestHandler { - constructor(manager, handler) { - this.manager = manager; - this.client = this.manager.client; - this.handle = handler.bind(this); - this.limit = Infinity; - this.resetTime = null; - this.remaining = 1; - this.busy = false; - this.queue = []; - } - - get limited() { - return (this.manager.globallyRateLimited || this.remaining <= 0) && Date.now() < this.resetTime; - } - - push(request) { - this.queue.push(request); - this.handle(); - } - - get _inactive() { - return this.queue.length === 0 && !this.limited && this.busy !== true; - } - - /* eslint-disable prefer-promise-reject-errors */ - execute(item) { - return new Promise((resolve, reject) => { - const finish = timeout => { - if (timeout || this.limited) { - if (!timeout) { - timeout = this.resetTime - Date.now() + this.client.options.restTimeOffset; - } - if (!this.manager.globalTimeout && this.manager.globallyRateLimited) { - this.manager.globalTimeout = setTimeout(() => { - this.manager.globalTimeout = undefined; - this.manager.globallyRateLimited = false; - this.busy = false; - this.handle(); - }, timeout); - reject({ }); - } else { - reject({ timeout }); - } - if (this.client.listenerCount(RATE_LIMIT)) { - /** - * Emitted when the client hits a rate limit while making a request - * @event Client#rateLimit - * @param {Object} rateLimitInfo Object containing the rate limit info - * @param {number} rateLimitInfo.timeout Timeout in ms - * @param {number} rateLimitInfo.limit Number of requests that can be made to this endpoint - * @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 - */ - this.client.emit(RATE_LIMIT, { - timeout, - limit: this.limit, - method: item.request.method, - path: item.request.path, - route: item.request.route, - }); - } - } else { - resolve(); - } - }; - item.request.make().then(res => { - if (res && res.headers) { - if (res.headers.get('x-ratelimit-global')) this.manager.globallyRateLimited = true; - this.limit = Number(res.headers.get('x-ratelimit-limit') || Infinity); - const reset = res.headers.get('x-ratelimit-reset'); - this.resetTime = reset !== null ? - (Number(reset) * 1e3) - new Date(res.headers.get('date') || Date.now()).getTime() + Date.now() : - Date.now(); - const remaining = res.headers.get('x-ratelimit-remaining'); - this.remaining = remaining !== null ? Number(remaining) : 1; - } - - if (res.ok) { - parseResponse(res).then(item.resolve, item.reject); - finish(); - return; - } - - if (res.status === 429) { - this.queue.unshift(item); - finish(Number(res.headers.get('retry-after')) + this.client.options.restTimeOffset); - } else if (res.status >= 500 && res.status < 600) { - if (item.retried) { - item.reject(res); - finish(); - } else { - item.retried = true; - this.queue.unshift(item); - finish(1e3 + this.client.options.restTimeOffset); - } - } else { - parseResponse(res).then(data => { - item.reject(res.status >= 400 && res.status < 500 ? - new DiscordAPIError(item.request.path, data, item.request.method) : res); - }, item.reject); - finish(); - } - }); - }); - } - - reset() { - this.manager.globallyRateLimited = false; - this.remaining = 1; - } -} - -module.exports = RequestHandler; diff --git a/src/rest/handlers/burst.js b/src/rest/handlers/burst.js deleted file mode 100644 index 54e4600ff..000000000 --- a/src/rest/handlers/burst.js +++ /dev/null @@ -1,15 +0,0 @@ -module.exports = function burst() { - if (this.limited || this.queue.length === 0) return; - this.execute(this.queue.shift()) - .then(this.handle.bind(this)) - .catch(({ timeout }) => { - if (timeout) { - this.client.setTimeout(() => { - this.reset(); - this.handle(); - }, timeout); - } - }); - this.remaining--; - this.handle(); -}; diff --git a/src/rest/handlers/index.js b/src/rest/handlers/index.js deleted file mode 100644 index 47792c46c..000000000 --- a/src/rest/handlers/index.js +++ /dev/null @@ -1,5 +0,0 @@ -module.exports = { - sequential: require('./sequential'), - burst: require('./burst'), - RequestHandler: require('./RequestHandler'), -}; diff --git a/src/rest/handlers/sequential.js b/src/rest/handlers/sequential.js deleted file mode 100644 index 71020282d..000000000 --- a/src/rest/handlers/sequential.js +++ /dev/null @@ -1,18 +0,0 @@ -module.exports = function sequential() { - if (this.busy || this.limited || this.queue.length === 0) return; - this.busy = true; - this.execute(this.queue.shift()) - .then(() => { - this.busy = false; - this.handle(); - }) - .catch(({ timeout }) => { - if (timeout) { - this.client.setTimeout(() => { - this.reset(); - this.busy = false; - this.handle(); - }, timeout); - } - }); -}; diff --git a/src/util/Constants.js b/src/util/Constants.js index 3e1b317d4..1bc05ef78 100644 --- a/src/util/Constants.js +++ b/src/util/Constants.js @@ -5,7 +5,6 @@ const browser = exports.browser = typeof window !== 'undefined'; /** * Options for a client. * @typedef {Object} ClientOptions - * @property {string} [apiRequestMethod='sequential'] One of `sequential` or `burst`. The sequential handler executes * all requests in the order they are triggered, whereas the burst handler runs multiple in parallel, and doesn't * provide the guarantee of any particular order. Burst mode is more likely to hit a 429 ratelimit error by its nature, * and is therefore slightly riskier to use. @@ -36,7 +35,6 @@ const browser = exports.browser = typeof window !== 'undefined'; * @property {HTTPOptions} [http] HTTP options */ exports.DefaultOptions = { - apiRequestMethod: 'sequential', shardId: 0, shardCount: 0, internalSharding: false,