Skip to content

Commit

Permalink
Replace 'abuse limit' with 'secondary limit' (#455)
Browse files Browse the repository at this point in the history 
* feat(secondary-limit): replace 'abuse-limit' logic with 'secondary-limit'

* test(secondary-limit): replace 'abuse-limit' with 'secondary-limit'

* docs(secondary-limit): replace 'abuse-limits' with 'secondary-limits'

docs(secondary-limits): replace 'abuse-limits' with 'secondary-limits'
  • Loading branch information
@oscard0m
oscard0m authored Mar 2, 2022
1 parent e797c02 commit 4c45c2e
Show file tree
Hide file tree
Showing 5 changed files with 182 additions and 163 deletions.
10 changes: 5 additions & 5 deletions README.md
View file
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
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",
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
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

0 comments on commit 4c45c2e

Please sign in to comment.