Skip to content

Commit

Permalink
fix: replace p-throttle with bottleneck
Browse files Browse the repository at this point in the history
  • Loading branch information
pvdlg authored and gr2m committed Feb 19, 2018
1 parent c8ab3fb commit 2a61d95
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 35 deletions.
35 changes: 12 additions & 23 deletions lib/get-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const url = require('url');
const {memoize} = require('lodash');
const Octokit = require('@octokit/rest');
const pRetry = require('p-retry');
const pThrottle = require('p-throttle');
const Bottleneck = require('bottleneck');

/**
* Default exponential backoff configuration for retries.
Expand All @@ -16,44 +16,33 @@ const DEFAULT_RETRY = {retries: 3, factor: 2, minTimeout: 1000};
* See {@link https://developer.github.com/v3/#rate-limiting|Rate limiting}.
*/
const RATE_LIMITS = {
search: [30, 60 * 1000],
core: [5000, 60 * 60 * 1000],
search: 60 * 1000 / 30, // 30 calls per minutes => 1 call per 2s
core: 60 * 60 * 1000 / 5000, // 5000 calls per hour => 1 call per 720ms
};

/**
* Global rate limit to prevent abuse.
*
* See {@link https://developer.github.com/v3/guides/best-practices-for-integrators/#dealing-with-abuse-rate-limits|Dealing with abuse rate limits}
*/
const GLOBAL_RATE_LIMIT = [1, 1000];
const GLOBAL_RATE_LIMIT = 1000;

/**
* Http error codes for which to not retry.
*/
const SKIP_RETRY_CODES = [400, 401, 403];

/**
* @typedef {Function} Throttler
* @param {Function} func The function to throttle.
* @param {Arguments} args The arguments to pass to the function to throttle.
*/

/**
* Create or retrieve the throttler function for a given rate limit group.
*
* @param {Array} rate The rate limit group.
* @param {String} limit The rate limits per API endpoints.
* @return {Throttler} The throller function for the given rate limit group.
*/
const getThrottler = memoize((rate, limit) => pThrottle((func, ...args) => func(...args), ...limit[rate]));

/**
* Create the global throttler function to comply with GitHub abuse prevention recommandations.
*
* @param {Array} globalLimit The global rate limit.
* @return {Throttler} The throller function for the global rate limit.
* @param {Bottleneck} globalThrottler The global throttler.
* @return {Bottleneck} The throller function for the given rate limit group.
*/
const getGlobalThrottler = globalLimit => pThrottle((func, ...args) => func(...args), ...globalLimit);
const getThrottler = memoize((rate, limit, globalThrottler) =>
new Bottleneck({minTime: limit[rate]}).chain(globalThrottler)
);

/**
* Create a`handler` for a `Proxy` wrapping an Octokit instance to:
Expand Down Expand Up @@ -89,11 +78,11 @@ const handler = (retry, limit, globalThrottler, endpoint) => ({
* @return {Promise<Any>} The result of the function called.
*/
apply: (func, that, args) => {
const throttler = getThrottler(limit[endpoint] ? endpoint : 'core', limit);
const throttler = getThrottler(limit[endpoint] ? endpoint : 'core', limit, globalThrottler);

return pRetry(async () => {
try {
return await globalThrottler((func, ...args) => throttler(func, ...args), func, ...args);
return await throttler.wrap(func)(...args);
} catch (err) {
if (SKIP_RETRY_CODES.includes(err.code)) {
throw new pRetry.AbortError(err);
Expand All @@ -120,5 +109,5 @@ module.exports = ({
pathPrefix: githubApiPathPrefix,
});
github.authenticate({type: 'token', token: githubToken});
return new Proxy(github, handler(retry, limit, getGlobalThrottler(globalLimit)));
return new Proxy(github, handler(retry, limit, new Bottleneck({minTime: globalLimit})));
};
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@
"@octokit/rest": "^14.0.9",
"@semantic-release/error": "^2.2.0",
"aggregate-error": "^1.0.0",
"bottleneck": "^2.0.1",
"debug": "^3.1.0",
"fs-extra": "^5.0.0",
"globby": "^8.0.0",
"issue-parser": "^1.0.1",
"lodash": "^4.17.4",
"mime": "^2.0.3",
"p-retry": "^1.0.0",
"p-throttle": "^1.1.0",
"parse-github-url": "^1.0.1",
"url-join": "^4.0.0"
},
Expand Down
5 changes: 4 additions & 1 deletion test/fail.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import {authenticate} from './helpers/mock-github';

const fail = proxyquire('../lib/fail', {
'./get-client': conf =>
getClient({...conf, ...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, globalLimit: [99, 1]}}),
getClient({
...conf,
...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, limit: {search: 1, core: 1}, globalLimit: 1},
}),
});

// Save the current process.env
Expand Down
3 changes: 2 additions & 1 deletion test/find-sr-issue.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ const githubToken = 'github_token';
const client = getClient({
githubToken,
retry: {retries: 3, factor: 2, minTimeout: 1, maxTimeout: 1},
globalLimit: [99, 1],
globalLimit: 1,
limit: {search: 1, core: 1},
});

test.beforeEach(t => {
Expand Down
12 changes: 6 additions & 6 deletions test/get-client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ test('Use the global throttler for all endpoints', async t => {
const octokit = {repos: {createRelease}, issues: {createComment}, search: {issues}, authenticate: stub()};
const rate = 150;
const github = proxyquire('../lib/get-client', {'@octokit/rest': stub().returns(octokit)})({
limit: {search: [99, 1], core: [99, 1]},
globalLimit: [1, rate],
limit: {search: 1, core: 1},
globalLimit: rate,
});

const a = await github.repos.createRelease();
Expand Down Expand Up @@ -58,8 +58,8 @@ test('Use the same throttler for endpoints in the same rate limit group', async
const searchRate = 300;
const coreRate = 150;
const github = proxyquire('../lib/get-client', {'@octokit/rest': stub().returns(octokit)})({
limit: {search: [1, searchRate], core: [1, coreRate]},
globalLimit: [99, 1],
limit: {search: searchRate, core: coreRate},
globalLimit: 1,
});

const a = await github.repos.createRelease();
Expand Down Expand Up @@ -93,9 +93,9 @@ test('Use the same throttler when retrying', async t => {
const octokit = {repos: {createRelease}, authenticate: stub()};
const coreRate = 200;
const github = proxyquire('../lib/get-client', {'@octokit/rest': stub().returns(octokit)})({
limit: {core: [1, coreRate]},
limit: {core: coreRate},
retry: {retries: 3, factor: 1, minTimeout: 1},
globalLimit: [6, 1],
globalLimit: 1,
});

await t.throws(github.repos.createRelease());
Expand Down
5 changes: 4 additions & 1 deletion test/publish.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import {authenticate, upload} from './helpers/mock-github';

const publish = proxyquire('../lib/publish', {
'./get-client': conf =>
getClient({...conf, ...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, globalLimit: [99, 1]}}),
getClient({
...conf,
...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, limit: {search: 1, core: 1}, globalLimit: 1},
}),
});

// Save the current process.env
Expand Down
5 changes: 4 additions & 1 deletion test/success.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import {authenticate} from './helpers/mock-github';

const success = proxyquire('../lib/success', {
'./get-client': conf =>
getClient({...conf, ...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, globalLimit: [99, 1]}}),
getClient({
...conf,
...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, limit: {search: 1, core: 1}, globalLimit: 1},
}),
});

// Save the current process.env
Expand Down
5 changes: 4 additions & 1 deletion test/verify.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ const envBackup = Object.assign({}, process.env);

const verify = proxyquire('../lib/verify', {
'./get-client': conf =>
getClient({...conf, ...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, globalLimit: [99, 1]}}),
getClient({
...conf,
...{retry: {retries: 3, factor: 1, minTimeout: 1, maxTimeout: 1}, limit: {search: 1, core: 1}, globalLimit: 1},
}),
});

test.beforeEach(t => {
Expand Down

0 comments on commit 2a61d95

Please sign in to comment.