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.

Sign up for GitHub

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

Replace 'abuse limit' with 'secondary limit' #455

Merged
merged 3 commits into from
Mar 2, 2022
Merged

Replace 'abuse limit' with 'secondary limit' #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1e9c3d
feat(secondary-limit): replace 'abuse-limit' logic with 'secondary-li…
oscard0m Feb 12, 2022
f7ed271
test(secondary-limit): replace 'abuse-limit' with 'secondary-limit'
oscard0m Feb 12, 2022
872d4a1
docs(secondary-limit): replace 'abuse-limits' with 'secondary-limits'
oscard0m Feb 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
[![@latest](https://img.shields.io/npm/v/@octokit/plugin-throttling.svg)](https://www.npmjs.com/package/@octokit/plugin-throttling)
[![Build Status](https://github.com/octokit/plugin-throttling.js/workflows/Test/badge.svg)](https://github.com/octokit/plugin-throttling.js/actions?workflow=Test)

Implements all [recommended best practices](https://docs.github.com/en/rest/guides/best-practices-for-integrators) to prevent hitting abuse rate limits.
Implements all [recommended best practices](https://docs.github.com/en/rest/guides/best-practices-for-integrators) to prevent hitting secondary rate limits.

## Usage

Expand Down Expand Up @@ -44,7 +44,7 @@ const { throttling } = require("@octokit/plugin-throttling");

The code below creates a "Hello, world!" issue on every repository in a given organization. Without the throttling plugin it would send many requests in parallel and would hit rate limits very quickly. But the `@octokit/plugin-throttling` slows down your requests according to the official guidelines, so you don't get blocked before your quota is exhausted.

The `throttle.onAbuseLimit` and `throttle.onRateLimit` options are required. Return `true` to automatically retry the request after `retryAfter` seconds.
The `throttle.onSecondaryRateLimit` and `throttle.onRateLimit` options are required. Return `true` to automatically retry the request after `retryAfter` seconds.

```js
const MyOctokit = Octokit.plugin(throttling);
Expand All @@ -63,10 +63,10 @@ const octokit = new MyOctokit({
return true;
}
},
onAbuseLimit: (retryAfter, options, octokit) => {
onSecondaryRateLimit: (retryAfter, options, octokit) => {
// does not retry, only logs a warning
octokit.log.warn(
`Abuse detected for request ${options.method} ${options.url}`
`SecondaryRateLimit detected for request ${options.method} ${options.url}`
);
},
},
Expand Down Expand Up @@ -119,7 +119,7 @@ connection.on("error", err => console.error(err));
const octokit = new MyOctokit({
auth: 'secret123'
throttle: {
onAbuseLimit: (retryAfter, options, octokit) => {
onSecondaryRateLimit: (retryAfter, options, octokit) => {
/* ... */
},
onRateLimit: (retryAfter, options, octokit) => {
Expand Down
30 changes: 20 additions & 10 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export function throttling(octokit: Octokit, octokitOptions = {}) {
{
clustering: connection != null,
triggersNotification,
minimumAbuseRetryAfter: 5,
minimumSecondaryRateRetryAfter: 5,
retryAfterBaseValue: 1000,
retryLimiter: new Bottleneck(),
id,
Expand All @@ -79,16 +79,17 @@ export function throttling(octokit: Octokit, octokitOptions = {}) {
);

if (
typeof state.onAbuseLimit !== "function" ||
(typeof state.onSecondaryRateLimit !== "function" &&
typeof state.onAbuseLimit !== "function") ||
typeof state.onRateLimit !== "function"
) {
throw new Error(`octokit/plugin-throttling error:
You must pass the onAbuseLimit and onRateLimit error handlers.
You must pass the onSecondaryRateLimit and onRateLimit error handlers.
See https://github.com/octokit/rest.js#throttling

const octokit = new Octokit({
throttle: {
onAbuseLimit: (retryAfter, options) => {/* ... */},
onSecondaryRateLimit: (retryAfter, options) => {/* ... */},
onRateLimit: (retryAfter, options) => {/* ... */}
}
})
Expand All @@ -98,7 +99,16 @@ export function throttling(octokit: Octokit, octokitOptions = {}) {
const events = {};
const emitter = new Bottleneck.Events(events);
// @ts-ignore
events.on("abuse-limit", state.onAbuseLimit);
events.on(
"secondary-limit",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming secondary-limit event will be sent from the L145 (so we have full control of the name). Is that correct or it should be listening to both (["abuse-limit", "secondary-limit"]?

state.onSecondaryRateLimit ||
function (...args: any[]) {
console.warn(
"[@octokit/plugin-throttling] `onAbuseLimit()` is deprecated and will be removed in a future release of `@octokit/plugin-throttling`, please use the `onSecondaryRateLimit` handler instead"
);
return state.onAbuseLimit(...args);
}
);
// @ts-ignore
events.on("rate-limit", state.onRateLimit);
// @ts-ignore
Expand All @@ -122,17 +132,17 @@ export function throttling(octokit: Octokit, octokitOptions = {}) {

const { wantRetry, retryAfter } = await (async function () {
if (/\bsecondary rate\b/i.test(error.message)) {
// The user has hit the abuse rate limit. (REST and GraphQL)
// https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits
// The user has hit the secondary rate limit. (REST and GraphQL)
// https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits

// The Retry-After header can sometimes be blank when hitting an abuse limit,
// The Retry-After header can sometimes be blank when hitting a secondary rate limit,
// but is always present after 2-3s, so make sure to set `retryAfter` to at least 5s by default.
const retryAfter = Math.max(
~~error.response.headers["retry-after"],
state.minimumAbuseRetryAfter
state.minimumSecondaryRateRetryAfter
);
const wantRetry = await emitter.trigger(
"abuse-limit",
"secondary-limit",
retryAfter,
options,
octokit
Expand Down
250 changes: 127 additions & 123 deletions test/events.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { TestOctokit } from "./octokit";
describe("Events", function () {
it("Should support non-limit 403s", async function () {
const octokit = new TestOctokit({
throttle: { onAbuseLimit: () => 1, onRateLimit: () => 1 },
throttle: { onSecondaryRateLimit: () => 1, onRateLimit: () => 1 },
});
let caught = false;

Expand Down Expand Up @@ -32,144 +32,148 @@ describe("Events", function () {
]);
});

describe("'abuse-limit'", function () {
it("Should detect abuse limit and broadcast event", async function () {
let eventCount = 0;
const octokit = new TestOctokit({
throttle: {
onAbuseLimit: (
retryAfter: number,
options: object,
octokitFromOptions: InstanceType<typeof TestOctokit>
) => {
expect(octokit).toBe(octokitFromOptions);
expect(retryAfter).toEqual(60);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
request: { retryCount: 0 },
});
eventCount++;
describe.each(["onSecondaryRateLimit", "onAbuseLimit"])(
"secondary-limit",
function (eventHandlerName) {
it("Should detect SecondaryRate limit and broadcast event", async function () {
let eventCount = 0;

const octokit = new TestOctokit({
throttle: {
[eventHandlerName]: (
retryAfter: number,
options: object,
octokitFromOptions: InstanceType<typeof TestOctokit>
) => {
expect(octokit).toBe(octokitFromOptions);
expect(retryAfter).toEqual(60);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
request: { retryCount: 0 },
});
eventCount++;
},
onRateLimit: () => 1,
},
onRateLimit: () => 1,
},
});
});

await octokit.request("GET /route1", {
request: {
responses: [{ status: 201, headers: {}, data: {} }],
},
});
try {
await octokit.request("GET /route2", {
await octokit.request("GET /route1", {
request: {
responses: [
{
status: 403,
headers: { "retry-after": "60" },
data: {
message: "You have exceeded a secondary rate limit",
},
},
],
responses: [{ status: 201, headers: {}, data: {} }],
},
});
throw new Error("Should not reach this point");
} catch (error: any) {
expect(error.status).toEqual(403);
}

expect(eventCount).toEqual(1);
});
try {
await octokit.request("GET /route2", {
request: {
responses: [
{
status: 403,
headers: { "retry-after": "60" },
data: {
message: "You have exceeded a secondary rate limit",
},
},
],
},
});
throw new Error("Should not reach this point");
} catch (error: any) {
expect(error.status).toEqual(403);
}

expect(eventCount).toEqual(1);
});

it("Should ensure retryAfter is a minimum of 5s", async function () {
let eventCount = 0;
const octokit = new TestOctokit({
throttle: {
onAbuseLimit: (retryAfter: number, options: object) => {
expect(retryAfter).toEqual(5);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
request: { retryCount: 0 },
});
eventCount++;
it("Should ensure retryAfter is a minimum of 5s", async function () {
let eventCount = 0;
const octokit = new TestOctokit({
throttle: {
[eventHandlerName]: (retryAfter: number, options: object) => {
expect(retryAfter).toEqual(5);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
request: { retryCount: 0 },
});
eventCount++;
},
onRateLimit: () => 1,
},
onRateLimit: () => 1,
},
});
});

await octokit.request("GET /route1", {
request: {
responses: [{ status: 201, headers: {}, data: {} }],
},
});
try {
await octokit.request("GET /route2", {
await octokit.request("GET /route1", {
request: {
responses: [
{
status: 403,
headers: { "retry-after": "2" },
data: {
message: "You have exceeded a secondary rate limit",
},
},
],
responses: [{ status: 201, headers: {}, data: {} }],
},
});
throw new Error("Should not reach this point");
} catch (error: any) {
expect(error.status).toEqual(403);
}

expect(eventCount).toEqual(1);
});
try {
await octokit.request("GET /route2", {
request: {
responses: [
{
status: 403,
headers: { "retry-after": "2" },
data: {
message: "You have exceeded a secondary rate limit",
},
},
],
},
});
throw new Error("Should not reach this point");
} catch (error: any) {
expect(error.status).toEqual(403);
}

expect(eventCount).toEqual(1);
});

it("Should broadcast retryAfter of 5s even when the header is missing", async function () {
let eventCount = 0;
const octokit = new TestOctokit({
throttle: {
onAbuseLimit: (retryAfter: number, options: object) => {
expect(retryAfter).toEqual(5);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
request: { retryCount: 0 },
});
eventCount++;
it("Should broadcast retryAfter of 5s even when the header is missing", async function () {
let eventCount = 0;
const octokit = new TestOctokit({
throttle: {
[eventHandlerName]: (retryAfter: number, options: object) => {
expect(retryAfter).toEqual(5);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
request: { retryCount: 0 },
});
eventCount++;
},
onRateLimit: () => 1,
},
onRateLimit: () => 1,
},
});
});

await octokit.request("GET /route1", {
request: {
responses: [{ status: 201, headers: {}, data: {} }],
},
});
try {
await octokit.request("GET /route2", {
await octokit.request("GET /route1", {
request: {
responses: [
{
status: 403,
headers: {},
data: {
message: "You have exceeded a secondary rate limit",
},
},
],
responses: [{ status: 201, headers: {}, data: {} }],
},
});
throw new Error("Should not reach this point");
} catch (error: any) {
expect(error.status).toEqual(403);
}

expect(eventCount).toEqual(1);
});
});
try {
await octokit.request("GET /route2", {
request: {
responses: [
{
status: 403,
headers: {},
data: {
message: "You have exceeded a secondary rate limit",
},
},
],
},
});
throw new Error("Should not reach this point");
} catch (error: any) {
expect(error.status).toEqual(403);
}

expect(eventCount).toEqual(1);
});
}
);

describe("'rate-limit'", function () {
it("Should detect rate limit exceeded and broadcast event", async function () {
Expand All @@ -191,7 +195,7 @@ describe("Events", function () {
});
eventCount++;
},
onAbuseLimit: () => 1,
onSecondaryRateLimit: () => 1,
},
});
const t0 = Date.now();
Expand Down
Loading