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

Jump to bottom

feat: allow strategies other than polynomial #412

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: allow strategies other than polynomial #412

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb2218e
feat: allow strategies other than polynomial
galargh Mar 8, 2023
53b1683
fix: wrap Math.pow with Math.round
galargh Mar 13, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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 base = (options.request.retryCount || 0) + 1;
let retryAfter;
switch (state.strategy) {
case "exponential":
retryAfter = Math.pow(2, base);
Copy link
Contributor

Choose a reason for hiding this comment

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

You have base inverted with your power - You'll need to do:Math.pow(base, exp).

Also, you might want to wrap this in Math.round(Math.pow(base, exp)) just because pow returns a double

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I renamed base variable to retryCount so that it's a bit less confusing. I also wrapped it in Math.round as suggested.

It reads as following now:

Math.round(Math.pow(2, retryCount))

As for the inversion, as per https://en.wikipedia.org/wiki/Exponential_backoff, exponential backoff is formed by taking the multiplicative factor (in this case 2) to the power of the number of observed events (in this case the number of retries that have already been performed). So I think it is correct now. Let me know if you wanted me to use different names though.

break;
case "linear":
retryAfter = base;
break;
default: // "polynomial"
retryAfter = Math.pow(base, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

For the polynomial strategy, you'll want Math.pow(2, base)

Also, you might want to wrap this in Math.round(Math.pow(base, exp)) just because pow returns a double

Copy link
Author

Choose a reason for hiding this comment

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

Wrapped Math.pow with Math.round - great spot, thanks 😄

As mentioned in #412 (comment), I think that would make it exponential backoff.

Full disclosure, I named this one polynomial by looking at what others tool call it (e.g. https://backoff-utils.readthedocs.io/en/latest/strategies.html#polynomial).

break;
}
throw octokit.retry.retryRequest(error, retries, retryAfter);
}

Expand Down
11 changes: 11 additions & 0 deletions src/index.ts
View file
Open in desktop
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
View file
Open in desktop
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