Skip to content

Commit

Permalink
feat(Rest): improve global rate limit and invalid request tracking
Browse files Browse the repository at this point in the history 
Ported from discordjs/discord.js#4711

refactor(SequentialHandler): use timers/promises

refactor(SequentialHandler): move global checks to each request

Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>

fix: separate queue for sub limits

refactor(SequentialHandler): address PR comments

Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>

refactor(SequentialHandler): better sublimit handling

refactor: address pr comments

Co-Authored-By: Vlad Frangu <kingdgrizzle@gmail.com>
Co-Authored-By: Antonio Román <kyradiscord@gmail.com>
Co-Authored-By: Noel <icrawltogo@gmail.com>

test: add sublimit tests

refactor(Util): only check route once

refactor: rename globalLimit

Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>

fix: address pr comments

Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>

refactor(Utils): use enum

Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>

Update packages/rest/src/lib/handlers/SequentialHandler.ts
  • Loading branch information
@ckohen
ckohen committed Oct 26, 2021
1 parent 9eb6afb commit c18f541
Show file tree
Hide file tree
Showing 6 changed files with 445 additions and 41 deletions.
172 changes: 170 additions & 2 deletions packages/rest/__tests__/RequestHandler.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
import nock from 'nock';
import { DefaultRestOptions, DiscordAPIError, HTTPError, REST } from '../src';
import { DefaultRestOptions, DiscordAPIError, HTTPError, REST, RESTEvents } from '../src';

const api = new REST({ timeout: 2000 }).setToken('A-Very-Fake-Token');
const api = new REST({ timeout: 2000, offset: 5 }).setToken('A-Very-Fake-Token');
const invalidAuthApi = new REST({ timeout: 2000 }).setToken('Definitely-Not-A-Fake-Token');

let resetAfter = 0;
let sublimitResetAfter = 0;
let retryAfter = 0;
let sublimitRequests = 0;
let sublimitHits = 0;
let serverOutage = true;
let unexpected429 = true;
let unexpected429cf = true;
const sublimitIntervals = {
reset: null,
retry: null,
};

nock(`${DefaultRestOptions.api}/v${DefaultRestOptions.version}`)
.persist()
Expand Down Expand Up @@ -63,6 +71,75 @@ nock(`${DefaultRestOptions.api}/v${DefaultRestOptions.version}`)
})
.get('/regularRequest')
.reply(204, { test: true })
.patch('/channels/:id', (body) => ['name', 'topic'].some((key) => Reflect.has(body, key)))
.reply(function handler(): nock.ReplyFnResult {
sublimitHits += 1;
sublimitRequests += 1;
const response = 2 - sublimitHits >= 0 && 10 - sublimitRequests >= 0 ? 204 : 429;
startSublimitIntervals();
if (response === 204) {
return [
204,
undefined,
{
'x-ratelimit-limit': '10',
'x-ratelimit-remaining': `${10 - sublimitRequests}`,
'x-ratelimit-reset-after': ((sublimitResetAfter - Date.now()) / 1000).toString(),
via: '1.1 google',
},
];
}
return [
429,
{
limit: '10',
remaining: `${10 - sublimitRequests}`,
resetAfter: (sublimitResetAfter / 1000).toString(),
retryAfter: ((retryAfter - Date.now()) / 1000).toString(),
},
{
'x-ratelimit-limit': '10',
'x-ratelimit-remaining': `${10 - sublimitRequests}`,
'x-ratelimit-reset-after': ((sublimitResetAfter - Date.now()) / 1000).toString(),
'retry-after': ((retryAfter - Date.now()) / 1000).toString(),
via: '1.1 google',
},
];
})
.patch('/channels/:id', (body) => ['name', 'topic'].every((key) => !Reflect.has(body, key)))
.reply(function handler(): nock.ReplyFnResult {
sublimitRequests += 1;
const response = 10 - sublimitRequests >= 0 ? 204 : 429;
startSublimitIntervals();
if (response === 204) {
return [
204,
undefined,
{
'x-ratelimit-limit': '10',
'x-ratelimit-remaining': `${10 - sublimitRequests}`,
'x-ratelimit-reset-after': ((sublimitResetAfter - Date.now()) / 1000).toString(),
via: '1.1 google',
},
];
}
return [
429,
{
limit: '10',
remaining: `${10 - sublimitRequests}`,
resetAfter: (sublimitResetAfter / 1000).toString(),
retryAfter: ((sublimitResetAfter - Date.now()) / 1000).toString(),
},
{
'x-ratelimit-limit': '10',
'x-ratelimit-remaining': `${10 - sublimitRequests}`,
'x-ratelimit-reset-after': ((sublimitResetAfter - Date.now()) / 1000).toString(),
'retry-after': ((sublimitResetAfter - Date.now()) / 1000).toString(),
via: '1.1 google',
},
];
})
.get('/unexpected')
.times(2)
.reply(function handler(): nock.ReplyFnResult {
Expand Down Expand Up @@ -117,6 +194,44 @@ nock(`${DefaultRestOptions.api}/v${DefaultRestOptions.version}`)
.get('/malformedRequest')
.reply(601);

// This is tested first to ensure the count remains accurate
test('Significant Invalid Requests', async () => {
const invalidListener = jest.fn();
const invalidListener2 = jest.fn();
api.on(RESTEvents.InvalidRequestWarning, invalidListener);
// Ensure listeners on REST do not get double added
api.on(RESTEvents.InvalidRequestWarning, invalidListener2);
api.off(RESTEvents.InvalidRequestWarning, invalidListener2);
const [a, b, c, d, e] = [
api.get('/badRequest'),
api.get('/badRequest'),
api.get('/badRequest'),
api.get('/badRequest'),
api.get('/badRequest'),
];
await expect(a).rejects.toThrowError('Missing Permissions');
await expect(b).rejects.toThrowError('Missing Permissions');
await expect(c).rejects.toThrowError('Missing Permissions');
await expect(d).rejects.toThrowError('Missing Permissions');
await expect(e).rejects.toThrowError('Missing Permissions');
expect(invalidListener).toHaveBeenCalledTimes(0);
api.requestManager.options.invalidRequestWarningInterval = 2;
const [f, g, h, i, j] = [
api.get('/badRequest'),
api.get('/badRequest'),
api.get('/badRequest'),
api.get('/badRequest'),
api.get('/badRequest'),
];
await expect(f).rejects.toThrowError('Missing Permissions');
await expect(g).rejects.toThrowError('Missing Permissions');
await expect(h).rejects.toThrowError('Missing Permissions');
await expect(i).rejects.toThrowError('Missing Permissions');
await expect(j).rejects.toThrowError('Missing Permissions');
expect(invalidListener).toHaveBeenCalledTimes(3);
api.off(RESTEvents.InvalidRequestWarning, invalidListener);
});

test('Handle standard rate limits', async () => {
const [a, b, c] = [api.get('/standard'), api.get('/standard'), api.get('/standard')];

Expand All @@ -137,6 +252,40 @@ test('Handle global rate limits', async () => {
expect(Date.now()).toBeGreaterThanOrEqual(earlier + 100);
});

test('Handle sublimits', async () => {
const sublimit = { body: { name: 'newname' } };
const noSublimit = { body: { bitrate: 40000 } };
// Return the current time on these results as their response does not indicate anything
// Queue all requests, don't wait, to allow retroactive check
const [aP, bP, cP, dP, eP] = [
api.patch('/channels/:id', sublimit).then(() => Date.now()),
api.patch('/channels/:id', sublimit).then(() => Date.now()),
api.patch('/channels/:id', sublimit).then(() => Date.now()), // Limit hits
api.patch('/channels/:id', noSublimit).then(() => Date.now()), // Ensure normal request passes
api.patch('/channels/:id', sublimit).then(() => Date.now()), // For retroactive check
];

const [a, b, c, d] = await Promise.all([aP, bP, cP, dP]);

const [f, g] = await Promise.all([
api.patch('/channels/:id', sublimit).then(() => Date.now()),
api.patch('/channels/:id', noSublimit).then(() => Date.now()),
]); // For additional sublimited checks
const e = await eP;

expect(a).toBeLessThan(b);
expect(b).toBeLessThan(c);
expect(d).toBeLessThan(c);
expect(c).toBeLessThan(e);
expect(d).toBeLessThan(e);
expect(e).toBeLessThan(f);
expect(e).toBeLessThan(g);
expect(g).toBeLessThan(f);

clearInterval(sublimitIntervals.reset);
clearInterval(sublimitIntervals.retry);
});

test('Handle unexpected 429', async () => {
const previous = Date.now();
expect(await api.get('/unexpected')).toStrictEqual({ test: true });
Expand Down Expand Up @@ -179,3 +328,22 @@ test('Unauthorized', async () => {
test('malformedRequest', async () => {
expect(await api.get('/malformedRequest')).toBe(null);
});

function startSublimitIntervals() {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (!sublimitIntervals.reset) {
sublimitResetAfter = Date.now() + 250;
sublimitIntervals.reset = setInterval(() => {
sublimitRequests = 0;
sublimitResetAfter = Date.now() + 250;
}, 250);
}
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (!sublimitIntervals.retry) {
retryAfter = Date.now() + 1000;
sublimitIntervals.retry = setInterval(() => {
sublimitHits = 0;
retryAfter = Date.now() + 1000;
}, 1000);
}
}
30 changes: 29 additions & 1 deletion packages/rest/src/lib/REST.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,17 @@ export interface RESTOptions {
* @default {}
*/
headers: Record<string, string>;
/**
* 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.
* @default 0
*/
invalidRequestWarningInterval: number;
/**
* How many requests to allow sending per second (Infinity for unlimited, 50 for the standard global limit used by Discord)
* @default 50
*/
globalRequestsPerSecond: number;
/**
* The extra offset to add to rate limits in milliseconds
* @default 50
Expand Down Expand Up @@ -81,9 +92,25 @@ export interface RateLimitData {
* If there is no major parameter (e.g: `/bot/gateway`) this will be `global`.
*/
majorParameter: string;
/**
* Whether the rate limit that was reached was the global limit
*/
global: boolean;
}

export interface InvalidRequestWarningData {
/**
* Number of invalid requests that have been made in the window
*/
count: number;
/**
* Time in ms remaining before the count resets
*/
remainingTime: number;
}

interface RestEvents {
invalidRequestWarning: [invalidRequestInfo: InvalidRequestWarningData];
restDebug: [info: string];
rateLimited: [rateLimitInfo: RateLimitData];
}
Expand Down Expand Up @@ -114,7 +141,8 @@ export class REST extends EventEmitter {
this.cdn = new CDN(options.cdn ?? DefaultRestOptions.cdn);
this.requestManager = new RequestManager(options)
.on(RESTEvents.Debug, this.emit.bind(this, RESTEvents.Debug))
.on(RESTEvents.RateLimited, this.emit.bind(this, RESTEvents.RateLimited));
.on(RESTEvents.RateLimited, this.emit.bind(this, RESTEvents.RateLimited))
.on(RESTEvents.InvalidRequestWarning, this.emit.bind(this, RESTEvents.InvalidRequestWarning));
}

/**
Expand Down
16 changes: 13 additions & 3 deletions packages/rest/src/lib/RequestManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,19 @@ export interface RouteData {
*/
export class RequestManager extends EventEmitter {
/**
* A timeout promise that is set when we hit the global rate limit
* @default null
* The number of requests remaining in the global bucket
*/
public globalTimeout: Promise<void> | null = null;
public globalRemaining: number;

/**
* The promise used to wait out the global rate limit
*/
public globalDelay: Promise<void> | null = null;

/**
* The timestamp at which the global bucket resets
*/
public globalReset = -1;

/**
* API bucket hashes that are cached from provided routes
Expand All @@ -132,6 +141,7 @@ export class RequestManager extends EventEmitter {
super();
this.options = { ...DefaultRestOptions, ...options };
this.options.offset = Math.max(0, this.options.offset);
this.globalRemaining = this.options.globalRequestsPerSecond;
}

/**
Expand Down

0 comments on commit c18f541

Please sign in to comment.