Skip to content

Commit c0773b6

Browse files
pvdlggr2m
authored andcommitted
feat: throttle and retry GitHub API calls
wip
1 parent ec510ec commit c0773b6

File tree

12 files changed

+362
-19
lines changed

12 files changed

+362
-19
lines changed

lib/fail.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ module.exports = async (pluginConfig, {options: {branch, repositoryUrl}, errors,
1212
pluginConfig
1313
);
1414
const {name: repo, owner} = parseGithubUrl(repositoryUrl);
15-
const github = getClient(githubToken, githubUrl, githubApiPathPrefix);
15+
const github = getClient({githubToken, githubUrl, githubApiPathPrefix});
1616
const body = failComment ? template(failComment)({branch, errors}) : getFailComment(branch, errors);
1717
const [srIssue] = await findSRIssues(github, failTitle, owner, repo);
1818

lib/get-client.js

Lines changed: 90 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,100 @@
11
const url = require('url');
2-
const GitHubApi = require('@octokit/rest');
2+
const {memoize} = require('lodash');
3+
const Octokit = require('@octokit/rest');
4+
const pRetry = require('p-retry');
5+
const pThrottle = require('p-throttle');
36

4-
module.exports = (githubToken, githubUrl, githubApiPathPrefix) => {
7+
/**
8+
* Default exponential backoff configuration for retries.
9+
*/
10+
const DEFAULT_RETRY = {retries: 3, factor: 2, minTimeout: 1000};
11+
12+
/**
13+
* Rate limit per API endpoints.
14+
*
15+
* See {@link https://developer.github.com/v3/search/#rate-limit|Search API rate limit}.
16+
* See {@link https://developer.github.com/v3/#rate-limiting|Rate limiting}.
17+
*/
18+
const RATE_LIMITS = {
19+
search: [30, 60 * 1000],
20+
core: [5000, 60 * 60 * 1000],
21+
};
22+
23+
/**
24+
* Http error codes for which to not retry.
25+
*/
26+
const SKIP_RETRY_CODES = [400, 401, 403];
27+
28+
/**
29+
* @typedef {Function} Throttler
30+
* @param {Function} func The function to throttle.
31+
* @param {Arguments} args The arguments to pass to the function to throttle.
32+
*/
33+
34+
/**
35+
* Create or retrieve the throller function for a given rate limit group.
36+
*
37+
* @param {Array} rate The rate limit group.
38+
* @param {String} limit The rate limits per API endpoints.
39+
* @type {Throttler} The throller function for the given rate limit group.
40+
*/
41+
const getThrottler = memoize((rate, limit) => pThrottle((func, ...args) => func(...args), ...limit[rate]));
42+
43+
/**
44+
* Create a`handler` for a `Proxy` wrapping an Octokit instance to:
45+
* - Recursively wrap the child objects of the Octokit instance in a `Proxy`
46+
* - Throttle and retry the Octokit instance functions
47+
*
48+
* @param {Object} retry The configuration to pass to `p-retry`.
49+
* @param {Array} limit The rate limits per API endpoints.
50+
* @param {String} endpoint The API endpoint to handle.
51+
* @return {Function} The `handler` for a `Proxy` wrapping an Octokit instance.
52+
*/
53+
const handler = (retry, limit, endpoint) => ({
54+
/**
55+
* If the target has the property as own, determine the rate limit based on the property name and recursively wrap the value in a `Proxy`. Otherwise returns the property value.
56+
*
57+
* @param {Object} target The target object.
58+
* @param {String} name The name of the property to get.
59+
* @param {Any} receiver The `Proxy` object.
60+
* @return {Any} The property value or a `Proxy` of the property value.
61+
*/
62+
get: (target, name, receiver) =>
63+
Object.prototype.hasOwnProperty.call(target, name)
64+
? new Proxy(target[name], handler(retry, limit, endpoint || name))
65+
: Reflect.get(target, name, receiver),
66+
67+
/**
68+
* Create a throlled version of the called function tehn call it and retry it if the call fails with certain error code.
69+
*
70+
* @param {Function} func The target function.
71+
* @param {Any} that The this argument for the call.
72+
* @param {Array} args The list of arguments for the call.
73+
* @return {Promise<Any>} The result of the function called.
74+
*/
75+
apply: (func, that, args) => {
76+
const throttler = getThrottler(limit[endpoint] ? endpoint : 'core', limit);
77+
return pRetry(async () => {
78+
try {
79+
return await throttler(func, ...args);
80+
} catch (err) {
81+
if (SKIP_RETRY_CODES.includes(err.code)) {
82+
throw new pRetry.AbortError(err);
83+
}
84+
throw err;
85+
}
86+
}, retry);
87+
},
88+
});
89+
90+
module.exports = ({githubToken, githubUrl, githubApiPathPrefix, retry = DEFAULT_RETRY, limit = RATE_LIMITS}) => {
591
const {port, protocol, hostname} = githubUrl ? url.parse(githubUrl) : {};
6-
const github = new GitHubApi({
92+
const github = new Octokit({
793
port,
894
protocol: (protocol || '').split(':')[0] || null,
995
host: hostname,
1096
pathPrefix: githubApiPathPrefix,
1197
});
1298
github.authenticate({type: 'token', token: githubToken});
13-
return github;
99+
return new Proxy(github, handler(retry, limit));
14100
};

lib/publish.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ const getClient = require('./get-client');
1212
module.exports = async (pluginConfig, {options: {branch, repositoryUrl}, nextRelease: {gitTag, notes}, logger}) => {
1313
const {githubToken, githubUrl, githubApiPathPrefix, assets} = resolveConfig(pluginConfig);
1414
const {name: repo, owner} = parseGithubUrl(repositoryUrl);
15-
const github = getClient(githubToken, githubUrl, githubApiPathPrefix);
15+
const github = getClient({githubToken, githubUrl, githubApiPathPrefix});
1616
const release = {owner, repo, tag_name: gitTag, name: gitTag, target_commitish: branch, body: notes}; // eslint-disable-line camelcase
1717

1818
debug('release owner: %o', owner);

lib/success.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ module.exports = async (
1616
) => {
1717
const {githubToken, githubUrl, githubApiPathPrefix, successComment, failTitle} = resolveConfig(pluginConfig);
1818
const {name: repo, owner} = parseGithubUrl(repositoryUrl);
19-
const github = getClient(githubToken, githubUrl, githubApiPathPrefix);
19+
const github = getClient({githubToken, githubUrl, githubApiPathPrefix});
2020
const releaseInfos = releases.filter(release => Boolean(release.name));
2121

2222
const prs = await pReduce(

lib/verify.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ module.exports = async (pluginConfig, {options: {repositoryUrl}, logger}) => {
7474
}
7575

7676
if (githubToken) {
77-
const github = getClient(githubToken, githubUrl, githubApiPathPrefix);
77+
const github = getClient({githubToken, githubUrl, githubApiPathPrefix});
7878

7979
try {
8080
const {data: {permissions: {push}}} = await github.repos.get({repo, owner});

package.json

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626
"lodash": "^4.17.4",
2727
"mime": "^2.0.3",
2828
"p-reduce": "^1.0.0",
29+
"p-retry": "^1.0.0",
30+
"p-throttle": "^1.1.0",
2931
"parse-github-url": "^1.0.1",
3032
"url-join": "^4.0.0"
3133
},
@@ -37,6 +39,7 @@
3739
"cz-conventional-changelog": "^2.0.0",
3840
"nock": "^9.1.0",
3941
"nyc": "^11.2.1",
42+
"proxyquire": "^1.8.0",
4043
"semantic-release": "^12.2.2",
4144
"sinon": "^4.0.0",
4245
"tempy": "^0.2.1",

test/fail.test.js

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,18 @@ import {escape} from 'querystring';
22
import test from 'ava';
33
import nock from 'nock';
44
import {stub} from 'sinon';
5+
import proxyquire from 'proxyquire';
56
import SemanticReleaseError from '@semantic-release/error';
67
import ISSUE_ID from '../lib/definitions/sr-issue-id';
7-
import fail from '../lib/fail';
8+
import getClient from '../lib/get-client';
89
import {authenticate} from './helpers/mock-github';
910

1011
/* eslint camelcase: ["error", {properties: "never"}] */
1112

13+
const fail = proxyquire('../lib/fail', {
14+
'./get-client': conf => getClient({...conf, ...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}}}),
15+
});
16+
1217
// Save the current process.env
1318
const envBackup = Object.assign({}, process.env);
1419

@@ -65,6 +70,52 @@ test.serial('Open a new issue with the list of errors', async t => {
6570
t.true(github.isDone());
6671
});
6772

73+
test.serial('Open a new issue with the list of errors, retrying 4 times', async t => {
74+
const owner = 'test_user';
75+
const repo = 'test_repo';
76+
process.env.GITHUB_TOKEN = 'github_token';
77+
const failTitle = 'The automated release is failing 🚨';
78+
const pluginConfig = {failTitle};
79+
const options = {branch: 'master', repositoryUrl: `https://github.com/${owner}/${repo}.git`};
80+
const errors = [
81+
new SemanticReleaseError('Error message 1', 'ERR1', 'Error 1 details'),
82+
new SemanticReleaseError('Error message 2', 'ERR2', 'Error 2 details'),
83+
new SemanticReleaseError('Error message 3', 'ERR3', 'Error 3 details'),
84+
];
85+
const github = authenticate()
86+
.get(
87+
`/search/issues?q=${escape('in:title')}+${escape(`repo:${owner}/${repo}`)}+${escape('type:issue')}+${escape(
88+
'state:open'
89+
)}+${escape(failTitle)}`
90+
)
91+
.times(3)
92+
.reply(404)
93+
.get(
94+
`/search/issues?q=${escape('in:title')}+${escape(`repo:${owner}/${repo}`)}+${escape('type:issue')}+${escape(
95+
'state:open'
96+
)}+${escape(failTitle)}`
97+
)
98+
.reply(200, {items: []})
99+
.post(`/repos/${owner}/${repo}/issues`, {
100+
title: failTitle,
101+
body: /---\n\n### Error message 1\n\nError 1 details\n\n---\n\n### Error message 2\n\nError 2 details\n\n---\n\n### Error message 3\n\nError 3 details\n\n---/,
102+
labels: ['semantic-release'],
103+
})
104+
.times(3)
105+
.reply(500)
106+
.post(`/repos/${owner}/${repo}/issues`, {
107+
title: failTitle,
108+
body: /---\n\n### Error message 1\n\nError 1 details\n\n---\n\n### Error message 2\n\nError 2 details\n\n---\n\n### Error message 3\n\nError 3 details\n\n---/,
109+
labels: ['semantic-release'],
110+
})
111+
.reply(200, {html_url: 'https://github.com/issues/1', number: 1});
112+
113+
await fail(pluginConfig, {options, errors, logger: t.context.logger});
114+
115+
t.deepEqual(t.context.log.args[0], ['Created issue #%d: %s.', 1, 'https://github.com/issues/1']);
116+
t.true(github.isDone());
117+
});
118+
68119
test.serial('Open a new issue with the list of errors and custom title and comment', async t => {
69120
const owner = 'test_user';
70121
const repo = 'test_repo';

test/find-sr-issue.test.js

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import {authenticate} from './helpers/mock-github';
1111

1212
// Save the current process.env
1313
const envBackup = Object.assign({}, process.env);
14+
const githubToken = 'github_token';
15+
const client = getClient({githubToken, retry: {retries: 3, factor: 2, minTimeout: 1, maxTimeout: 1}});
1416

1517
test.beforeEach(t => {
1618
// Delete env variables in case they are on the machine running the tests
@@ -51,7 +53,7 @@ test.serial('Filter out issues without ID', async t => {
5153
)
5254
.reply(200, {items: issues});
5355

54-
const srIssues = await findSRIssues(getClient(githubToken), title, owner, repo);
56+
const srIssues = await findSRIssues(client, title, owner, repo);
5557

5658
t.deepEqual(srIssues, [
5759
{number: 2, body: 'Issue 2 body\n\n<!-- semantic-release:github -->', title},
@@ -75,7 +77,7 @@ test.serial('Return empty array if not issues found', async t => {
7577
)
7678
.reply(200, {items: issues});
7779

78-
const srIssues = await findSRIssues(getClient(githubToken), title, owner, repo);
80+
const srIssues = await findSRIssues(client, title, owner, repo);
7981

8082
t.deepEqual(srIssues, []);
8183

@@ -96,9 +98,45 @@ test.serial('Return empty array if not issues has matching ID', async t => {
9698
)
9799
.reply(200, {items: issues});
98100

99-
const srIssues = await findSRIssues(getClient(githubToken), title, owner, repo);
101+
const srIssues = await findSRIssues(client, title, owner, repo);
100102

101103
t.deepEqual(srIssues, []);
104+
t.true(github.isDone());
105+
});
106+
107+
test.serial('Retries 4 times', async t => {
108+
const owner = 'test_user';
109+
const repo = 'test_repo';
110+
const title = 'The automated release is failing :rotating_light:';
111+
const github = authenticate({githubToken})
112+
.get(
113+
`/search/issues?q=${escape('in:title')}+${escape(`repo:${owner}/${repo}`)}+${escape('type:issue')}+${escape(
114+
'state:open'
115+
)}+${escape(title)}`
116+
)
117+
.times(4)
118+
.reply(422);
119+
120+
const error = await t.throws(findSRIssues(client, title, owner, repo));
121+
122+
t.is(error.code, 422);
123+
t.true(github.isDone());
124+
});
125+
126+
test.serial('Do not retry on 401 error', async t => {
127+
const owner = 'test_user';
128+
const repo = 'test_repo';
129+
const title = 'The automated release is failing :rotating_light:';
130+
const github = authenticate({githubToken})
131+
.get(
132+
`/search/issues?q=${escape('in:title')}+${escape(`repo:${owner}/${repo}`)}+${escape('type:issue')}+${escape(
133+
'state:open'
134+
)}+${escape(title)}`
135+
)
136+
.reply(401);
137+
138+
const error = await t.throws(findSRIssues(client, title, owner, repo));
102139

140+
t.is(error.code, 401);
103141
t.true(github.isDone());
104142
});

test/get-client.test.js

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
import test from 'ava';
2+
import {isFunction, isPlainObject, inRange} from 'lodash';
3+
import {stub} from 'sinon';
4+
import proxyquire from 'proxyquire';
5+
import getClient from '../lib/get-client';
6+
7+
test('Wrap Octokit in a proxy', t => {
8+
const github = getClient({githubToken: 'github_token'});
9+
10+
t.true(Object.prototype.hasOwnProperty.call(github, 'repos'));
11+
t.true(isPlainObject(github.repos));
12+
t.true(Object.prototype.hasOwnProperty.call(github.repos, 'createRelease'));
13+
t.true(isFunction(github.repos.createRelease));
14+
15+
t.true(Object.prototype.hasOwnProperty.call(github, 'search'));
16+
t.true(isPlainObject(github.search));
17+
t.true(Object.prototype.hasOwnProperty.call(github.search, 'issues'));
18+
t.true(isFunction(github.search.issues));
19+
20+
t.falsy(github.unknown);
21+
});
22+
23+
test('Use the same throttler for endpoints in the same rate limit group', async t => {
24+
const createRelease = stub().callsFake(async () => Date.now());
25+
const createComment = stub().callsFake(async () => Date.now());
26+
const issues = stub().callsFake(async () => Date.now());
27+
const octokit = {repos: {createRelease}, issues: {createComment}, search: {issues}, authenticate: stub()};
28+
const searchRate = 300;
29+
const coreRate = 150;
30+
const github = proxyquire('../lib/get-client', {'@octokit/rest': stub().returns(octokit)})({
31+
limit: {search: [1, searchRate], core: [1, coreRate]},
32+
});
33+
34+
const a = await github.repos.createRelease();
35+
const b = await github.issues.createComment();
36+
const c = await github.repos.createRelease();
37+
const d = await github.issues.createComment();
38+
const e = await github.search.issues();
39+
const f = await github.search.issues();
40+
41+
// `issues.createComment` should be called `coreRate` ms after `repos.createRelease`
42+
t.true(inRange(b - a, coreRate - 50, coreRate + 50));
43+
// `repos.createRelease` should be called `coreRate` ms after `issues.createComment`
44+
t.true(inRange(c - b, coreRate - 50, coreRate + 50));
45+
// `issues.createComment` should be called `coreRate` ms after `repos.createRelease`
46+
t.true(inRange(d - c, coreRate - 50, coreRate + 50));
47+
48+
// The first search should be called immediatly as it uses a different throttler
49+
t.true(inRange(e - d, -50, 50));
50+
// The second search should be called only after `searchRate` ms
51+
t.true(inRange(f - e, searchRate - 50, searchRate + 50));
52+
});
53+
54+
test('Use the same throttler when retrying', async t => {
55+
const createRelease = stub().callsFake(async () => {
56+
const err = new Error();
57+
err.time = Date.now();
58+
err.code = 404;
59+
throw err;
60+
});
61+
62+
const octokit = {repos: {createRelease}, authenticate: stub()};
63+
const coreRate = 200;
64+
const github = proxyquire('../lib/get-client', {'@octokit/rest': stub().returns(octokit)})({
65+
limit: {core: [1, coreRate]},
66+
retry: {retries: 3, factor: 1, minTimeout: 1},
67+
});
68+
69+
await t.throws(github.repos.createRelease());
70+
t.is(createRelease.callCount, 4);
71+
72+
const {time: a} = await t.throws(createRelease.getCall(0).returnValue);
73+
const {time: b} = await t.throws(createRelease.getCall(1).returnValue);
74+
const {time: c} = await t.throws(createRelease.getCall(2).returnValue);
75+
const {time: d} = await t.throws(createRelease.getCall(3).returnValue);
76+
77+
// Each retry should be done after `coreRate` ms
78+
t.true(inRange(b - a, coreRate - 50, coreRate + 50));
79+
t.true(inRange(c - b, coreRate - 50, coreRate + 50));
80+
t.true(inRange(d - c, coreRate - 50, coreRate + 50));
81+
});

0 commit comments

Comments
 (0)