Skip to content

Commit

Permalink
fix: bump minimal retry in case of secondary rate limiting to 60s (#594)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gr2m
gr2m authored May 19, 2023
1 parent 28098cb commit 5335870
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 169 deletions.
52 changes: 29 additions & 23 deletions src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,34 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
createGroups(Bottleneck, common);
}

if (
octokitOptions.throttle &&
octokitOptions.throttle.minimalSecondaryRateRetryAfter
) {
octokit.log.warn(
"[@octokit/plugin-throttling] `options.throttle.minimalSecondaryRateRetryAfter` is deprecated, please use `options.throttle.fallbackSecondaryRateRetryAfter` instead"
);
octokitOptions.throttle.fallbackSecondaryRateRetryAfter =
octokitOptions.throttle.minimalSecondaryRateRetryAfter;
delete octokitOptions.throttle.minimalSecondaryRateRetryAfter;
}

if (octokitOptions.throttle && octokitOptions.throttle.onAbuseLimit) {
octokit.log.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"
);
// @ts-ignore types don't allow for both properties to be set
octokitOptions.throttle.onSecondaryRateLimit =
octokitOptions.throttle.onAbuseLimit;
// @ts-ignore
delete octokitOptions.throttle.onAbuseLimit;
}

const state = Object.assign(
{
clustering: connection != null,
triggersNotification,
minimumSecondaryRateRetryAfter: 5,
fallbackSecondaryRateRetryAfter: 60,
retryAfterBaseValue: 1000,
retryLimiter: new Bottleneck(),
id,
Expand All @@ -72,13 +95,8 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
octokitOptions.throttle
);

const isUsingDeprecatedOnAbuseLimitHandler =
typeof state.onAbuseLimit === "function" && state.onAbuseLimit;

if (
typeof (isUsingDeprecatedOnAbuseLimitHandler
? state.onAbuseLimit
: state.onSecondaryRateLimit) !== "function" ||
typeof state.onSecondaryRateLimit !== "function" ||
typeof state.onRateLimit !== "function"
) {
throw new Error(`octokit/plugin-throttling error:
Expand All @@ -97,18 +115,7 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
const events = {};
const emitter = new Bottleneck.Events(events);
// @ts-expect-error
events.on(
"secondary-limit",
isUsingDeprecatedOnAbuseLimitHandler
? function (...args: [number, OctokitOptions, Octokit]) {
octokit.log.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"
);
// @ts-expect-error
return state.onAbuseLimit(...args);
}
: state.onSecondaryRateLimit
);
events.on("secondary-limit", state.onSecondaryRateLimit);
// @ts-expect-error
events.on("rate-limit", state.onRateLimit);
// @ts-expect-error
Expand Down Expand Up @@ -140,10 +147,9 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {

// 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.minimumSecondaryRateRetryAfter
);
const retryAfter =
Number(error.response.headers["retry-after"]) ||
state.fallbackSecondaryRateRetryAfter;
const wantRetry = await emitter.trigger(
"secondary-limit",
retryAfter,
Expand Down
6 changes: 5 additions & 1 deletion src/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ export type ThrottlingOptionsBase = {
id?: string;
timeout?: number;
connection?: Bottleneck.RedisConnection | Bottleneck.IORedisConnection;
minimumSecondaryRateRetryAfter?: number;
/**
* @deprecated use `fallbackSecondaryRateRetryAfter`
*/
minimalSecondaryRateRetryAfter?: number;
fallbackSecondaryRateRetryAfter?: number;
retryAfterBaseValue?: number;
write?: Bottleneck.Group;
search?: Bottleneck.Group;
Expand Down
46 changes: 46 additions & 0 deletions test/deprecations.test.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { Octokit } from "@octokit/core";
import { throttling } from "../src";

const TestOctokit = Octokit.plugin(throttling);

describe("deprecations", () => {
it("throttle.minimalSecondaryRateRetryAfter option", () => {
const log = {
warn: jest.fn(),
};
new TestOctokit({
// @ts-expect-error
log,
throttle: {
minimalSecondaryRateRetryAfter: 1,
onSecondaryRateLimit: () => 1,
onRateLimit: () => 1,
},
});

expect(log.warn).toHaveBeenCalledWith(
"[@octokit/plugin-throttling] `options.throttle.minimalSecondaryRateRetryAfter` is deprecated, please use `options.throttle.fallbackSecondaryRateRetryAfter` instead"
);
});

describe("throttle.onAbuseLimit", function () {
it("Should detect SecondaryRate limit and broadcast event", async function () {
const log = {
warn: jest.fn(),
};

new TestOctokit({
// @ts-expect-error
log,
throttle: {
onAbuseLimit: () => 1,
onRateLimit: () => 1,
},
});

expect(log.warn).toHaveBeenCalledWith(
"[@octokit/plugin-throttling] `onAbuseLimit()` is deprecated and will be removed in a future release of `@octokit/plugin-throttling`, please use the `onSecondaryRateLimit` handler instead"
);
});
});
});
148 changes: 8 additions & 140 deletions test/events.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,56 +80,12 @@ describe("Events", function () {
expect(eventCount).toEqual(1);
});

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

await octokit.request("GET /route1", {
request: {
responses: [{ status: 201, headers: {}, data: {} }],
},
});
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: {
onSecondaryRateLimit: (retryAfter, options) => {
expect(retryAfter).toEqual(5);
expect(retryAfter).toEqual(60);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
Expand Down Expand Up @@ -168,13 +124,13 @@ describe("Events", function () {
expect(eventCount).toEqual(1);
});
});
describe("with 'onAbuseLimit'", function () {
describe("with 'onSecondaryRateLimit'", function () {
it("Should detect SecondaryRate limit and broadcast event", async function () {
let eventCount = 0;

const octokit = new TestOctokit({
throttle: {
onAbuseLimit: (retryAfter, options, octokitFromOptions) => {
onSecondaryRateLimit: (retryAfter, options, octokitFromOptions) => {
expect(octokit).toBe(octokitFromOptions);
expect(retryAfter).toEqual(60);
expect(options).toMatchObject({
Expand Down Expand Up @@ -214,94 +170,6 @@ describe("Events", function () {

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, options) => {
expect(retryAfter).toEqual(5);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
request: { retryCount: 0 },
});
eventCount++;
},
onRateLimit: () => 1,
},
});

await octokit.request("GET /route1", {
request: {
responses: [{ status: 201, headers: {}, data: {} }],
},
});
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, options) => {
expect(retryAfter).toEqual(5);
expect(options).toMatchObject({
method: "GET",
url: "/route2",
request: { retryCount: 0 },
});
eventCount++;
},
onRateLimit: () => 1,
},
});

await octokit.request("GET /route1", {
request: {
responses: [{ status: 201, headers: {}, data: {} }],
},
});
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);
});
});
});

Expand Down Expand Up @@ -360,15 +228,15 @@ describe("Events", function () {
const octokit = new TestOctokit({
throttle: {
onRateLimit: () => {
throw new Error("Should not reach this point");
throw new Error("Error in onRateLimit handler");
},
onSecondaryRateLimit: () => {
throw new Error("Should not reach this point");
throw new Error("Error in onSecondaryRateLimit handler");
},
},
});

jest.spyOn(octokit.log, "warn");
jest.spyOn(octokit.log, "warn").mockImplementation(() => {});

const t0 = Date.now();

Expand Down Expand Up @@ -397,7 +265,7 @@ describe("Events", function () {
expect(error.status).toEqual(403);
expect(octokit.log.warn).toHaveBeenCalledWith(
"Error in throttling-plugin limit handler",
new Error("Should not reach this point")
new Error("Error in onRateLimit handler")
);
}
});
Expand Down
Loading

0 comments on commit 5335870

Please sign in to comment.