refactor: clean up rate limit handling #2694
Conversation
src/rest/RequestHandler.js
Outdated
| function parseResponse(res) { | ||
| if (res.headers.get('content-type').startsWith('application/json')) return res.json(); | ||
| // eslint-disable-next-line no-undef | ||
| if (browser) return res.blob(); |
There was a problem hiding this comment.
My eslint was acting up, it seems, will remove it.
src/rest/RequestHandler.js
Outdated
|
|
||
| this.limit = limit ? Number(limit) : Infinity; | ||
| this.remaining = remaining ? Number(remaining) : 1; | ||
| this.reset = reset ? this._calculateReset(reset, serverDate) + 100 : Date.now(); |
There was a problem hiding this comment.
why are we offsetting reset times by 100ms
There was a problem hiding this comment.
I figured, as before, we should have some leeway in case we are ever barely off in calculations. Is it better removed entirely, or reduced?
There was a problem hiding this comment.
Removed entirely. We already have a rainy day fund that no other discord library implements (let alone has on by default), which is d.js reactions are slower than any other library, even those who don't hardcode. ClientOptions#restTimeOffset... Speaking of, it looks like you have inadvertently removed that functionality.
A rainy day fund is really not necessary, but if it's there, it should continue to be configurable.
src/rest/RequestHandler.js
Outdated
| if (this.limited) { | ||
| const timeout = this.reset - Date.now(); | ||
|
|
||
| if (this.manager.client.listenerCount(RATE_LIMIT)) { |
There was a problem hiding this comment.
no need to access client from the manager
src/rest/RequestHandler.js
Outdated
| // A ratelimit was hit - this should never happen | ||
| this.queue.unshift(item); | ||
| await Util.delayFor(this.retryAfter); | ||
| return this.run(); |
There was a problem hiding this comment.
we should emit an event for this, whether it's debug or its own special event
src/rest/RequestHandler.js
Outdated
| * @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, { |
src/rest/RequestHandler.js
Outdated
| 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() + this._getAPIOffset(serverDate) + 250 + this.client.options.restTimeOffset; | ||
| this.reset = Date.now() + this._getAPIOffset(serverDate) + 250; | ||
| } | ||
| } | ||
|
|
||
| // After calculations, pre-emptively stop farther requests |
There was a problem hiding this comment.
"Further" Versus "Farther" The quick and dirty tip is to use “farther” for physical distance and “further” for metaphorical, or figurative, distance.
src/rest/RequestHandler.js
Outdated
|
|
||
| _getAPIOffset(serverDate) { | ||
| return new Date(serverDate).getTime() - Date.now(); | ||
| } |
There was a problem hiding this comment.
this seems like it can be moved to a simple function in this file rather than being a class method, as it doesn't seem like this should be used anywhere else and doesn't require this context
src/rest/RequestHandler.js
Outdated
|
|
||
| _calculateReset(reset, serverDate) { | ||
| return new Date(Number(reset) * 1000).getTime() - this._getAPIOffset(serverDate); | ||
| } |
src/rest/RequestHandler.js
Outdated
|
|
||
| run() { | ||
| if (this.queue.length === 0) return; | ||
| this.execute(this.queue.shift()); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| /* eslint-disable-next-line complexity */ | ||
| async execute(item) { | ||
| // Insert item back to the beginning if currently busy |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/rest/HTTPError.js
Outdated
| */ | ||
| this.path = path; | ||
|
|
||
| if (Error.captureStackTrace) Error.captureStackTrace(this, this.constructor); |
There was a problem hiding this comment.
If #2744 lands first, this line could be removed since it would capture the stack trace twice.
Class was introduced with #2694 * typings: Add HTTPError class definition * typings: Sort HTTPError's members by name
Please describe the changes this PR makes and why it should be merged:
This (tries to) accomplish a few things:
Client#debugnotifying of an actual 429 being hit if it ever happensRequestHandler#executecatchable (via normally catching the request at top level)Please suggest anything that can be done better, as there probably are things. Thanks!
Semantic versioning classification: