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.

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

feat: allow strategies other than polynomial #412

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,19 @@ const octokit = new MyOctokit({
});
```

To override the retry strategy:

```js
const octokit = new MyOctokit({
auth: "secret123",
retry: {
// retry strategy, can be one of `exponential`, `linear`, or `polynomial`
// defaults to `polynomial`
strategy: "polynomial",
},
});
```

You can manually ask for retries for any request by passing `{ request: { retries: numRetries, retryAfter: delayInSeconds }}`. Note that the `doNotRetry` option from the constructor is ignored in this case, requests will be retried no matter their response code.

```js
Expand Down
14 changes: 13 additions & 1 deletion src/error-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,19 @@ export async function errorRequest(state, octokit, error, options) {
if (error.status >= 400 && !state.doNotRetry.includes(error.status)) {
const retries =
options.request.retries != null ? options.request.retries : state.retries;
const retryAfter = Math.pow((options.request.retryCount || 0) + 1, 2);
const retryCount = (options.request.retryCount || 0) + 1;
let retryAfter;
switch (state.strategy) {
case "exponential":
retryAfter = Math.round(Math.pow(2, retryCount));
break;
case "linear":
retryAfter = retryCount;
break;
default: // "polynomial"
retryAfter = Math.round(Math.pow(retryCount, 2));
break;
}
throw octokit.retry.retryRequest(error, retries, retryAfter);
}

Expand Down
11 changes: 11 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,28 @@ import { wrapRequest } from "./wrap-request";

export const VERSION = "0.0.0-development";

export enum RetryStrategy {
exponential = "exponential",
linear = "linear",
polynomial = "polynomial",
}

export function retry(octokit: Octokit, octokitOptions: any) {
const state = Object.assign(
{
enabled: true,
retryAfterBaseValue: 1000,
doNotRetry: [400, 401, 403, 404, 422],
retries: 3,
strategy: RetryStrategy.polynomial,
},
octokitOptions.retry
);

if (!RetryStrategy.hasOwnProperty(state.strategy)) {
throw new Error(`Invalid retry strategy: ${state.strategy}`);
}

if (state.enabled) {
octokit.hook.error("request", errorRequest.bind(null, state, octokit));
octokit.hook.wrap("request", wrapRequest.bind(null, state, octokit));
Expand Down
112 changes: 106 additions & 6 deletions test/retry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@ import { errorRequest } from "../src/error-request";
import { RequestError } from "@octokit/request-error";
import { RequestMethod } from "@octokit/types";

describe("Octokit", function () {
it("Should fail on creation if strategy is invalid", function () {
expect(() => {
new TestOctokit({ retry: { strategy: "invalid" } });
}).toThrow("Invalid retry strategy: invalid");
});
});

describe("Automatic Retries", function () {
it("Should be possible to disable the plugin", async function () {
const octokit = new TestOctokit({ retry: { enabled: false } });
Expand Down Expand Up @@ -164,7 +172,7 @@ describe("Automatic Retries", function () {
expect(ms).toBeLessThan(45);
});

it("Should trigger exponential retries on HTTP 500 errors", async function () {
it("Should trigger polynomial retries on HTTP 500 errors", async function () {
const octokit = new TestOctokit({ retry: { retryAfterBaseValue: 50 } });

const res = await octokit.request("GET /route", {
Expand Down Expand Up @@ -202,7 +210,7 @@ describe("Automatic Retries", function () {

const ms2 = octokit.__requestTimings[3] - octokit.__requestTimings[2];
expect(ms2).toBeLessThan(470);
expect(ms2).toBeGreaterThan(420);
expect(ms2).toBeGreaterThan(430);
});

it("Should not retry 3xx/400/401/403/422 errors", async function () {
Expand Down Expand Up @@ -266,12 +274,104 @@ describe("Automatic Retries", function () {
expect(caught).toEqual(testStatuses.length);
});

it("Should allow to override the strategy with exponential", async function () {
const octokit = new TestOctokit({
retry: {
strategy: "exponential",
retryAfterBaseValue: 50,
},
});

const res = await octokit.request("GET /route", {
request: {
responses: [
{ status: 500, headers: {}, data: { message: "Did not retry, one" } },
{ status: 500, headers: {}, data: { message: "Did not retry, two" } },
{
status: 500,
headers: {},
data: { message: "Did not retry, three" },
},
{ status: 200, headers: {}, data: { message: "Success!" } },
],
},
});

expect(res.status).toEqual(200);
expect(res.data).toStrictEqual({ message: "Success!" });
expect(octokit.__requestLog).toStrictEqual([
"START GET /route",
"START GET /route",
"START GET /route",
"START GET /route",
"END GET /route",
]);

const ms0 = octokit.__requestTimings[1] - octokit.__requestTimings[0];
expect(ms0).toBeLessThan(120);
expect(ms0).toBeGreaterThan(80);

const ms1 = octokit.__requestTimings[2] - octokit.__requestTimings[1];
expect(ms1).toBeLessThan(220);
expect(ms1).toBeGreaterThan(180);

const ms2 = octokit.__requestTimings[3] - octokit.__requestTimings[2];
expect(ms2).toBeLessThan(420);
expect(ms2).toBeGreaterThan(380);
});

it("Should allow to override the strategy with linear", async function () {
const octokit = new TestOctokit({
retry: {
strategy: "linear",
retryAfterBaseValue: 50,
},
});

const res = await octokit.request("GET /route", {
request: {
responses: [
{ status: 500, headers: {}, data: { message: "Did not retry, one" } },
{ status: 500, headers: {}, data: { message: "Did not retry, two" } },
{
status: 500,
headers: {},
data: { message: "Did not retry, three" },
},
{ status: 200, headers: {}, data: { message: "Success!" } },
],
},
});

expect(res.status).toEqual(200);
expect(res.data).toStrictEqual({ message: "Success!" });
expect(octokit.__requestLog).toStrictEqual([
"START GET /route",
"START GET /route",
"START GET /route",
"START GET /route",
"END GET /route",
]);

const ms0 = octokit.__requestTimings[1] - octokit.__requestTimings[0];
expect(ms0).toBeLessThan(70);
expect(ms0).toBeGreaterThan(30);

const ms1 = octokit.__requestTimings[2] - octokit.__requestTimings[1];
expect(ms1).toBeLessThan(120);
expect(ms1).toBeGreaterThan(80);

const ms2 = octokit.__requestTimings[3] - octokit.__requestTimings[2];
expect(ms2).toBeLessThan(170);
expect(ms2).toBeGreaterThan(130);
});

it('Should retry "Something went wrong" GraphQL error', async function () {
const octokit = new TestOctokit();

const result = await octokit.graphql({
query: `query {
viewer {
query: `query {
viewer {
login
}
}`,
Expand Down Expand Up @@ -347,8 +447,8 @@ describe("Automatic Retries", function () {

try {
await octokit.graphql({
query: `query {
viewer {
query: `query {
viewer {
login
}
}`,
Expand Down