From 2a61d95acfc7039555d40f6f3383f5ef429da5e0 Mon Sep 17 00:00:00 2001 From: Pierre Vanduynslager Date: Sun, 18 Feb 2018 22:48:30 -0500 Subject: [PATCH] fix: replace `p-throttle` with `bottleneck` --- lib/get-client.js | 35 ++++++++++++----------------------- package.json | 2 +- test/fail.test.js | 5 ++++- test/find-sr-issue.test.js | 3 ++- test/get-client.test.js | 12 ++++++------ test/publish.test.js | 5 ++++- test/success.test.js | 5 ++++- test/verify.test.js | 5 ++++- 8 files changed, 37 insertions(+), 35 deletions(-) diff --git a/lib/get-client.js b/lib/get-client.js index fa5319fe..9c972a04 100644 --- a/lib/get-client.js +++ b/lib/get-client.js @@ -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. @@ -16,8 +16,8 @@ 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 }; /** @@ -25,35 +25,24 @@ const RATE_LIMITS = { * * 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: @@ -89,11 +78,11 @@ const handler = (retry, limit, globalThrottler, endpoint) => ({ * @return {Promise} 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); @@ -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}))); }; diff --git a/package.json b/package.json index 1fabbd6e..47556598 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,7 @@ "@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", @@ -26,7 +27,6 @@ "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" }, diff --git a/test/fail.test.js b/test/fail.test.js index 7397a863..ba6c3103 100644 --- a/test/fail.test.js +++ b/test/fail.test.js @@ -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 diff --git a/test/find-sr-issue.test.js b/test/find-sr-issue.test.js index 799c0cdb..9d527532 100644 --- a/test/find-sr-issue.test.js +++ b/test/find-sr-issue.test.js @@ -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 => { diff --git a/test/get-client.test.js b/test/get-client.test.js index 7befe7d0..3d5d51c8 100644 --- a/test/get-client.test.js +++ b/test/get-client.test.js @@ -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(); @@ -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(); @@ -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()); diff --git a/test/publish.test.js b/test/publish.test.js index d87f860b..905e0e83 100644 --- a/test/publish.test.js +++ b/test/publish.test.js @@ -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 diff --git a/test/success.test.js b/test/success.test.js index 76eab972..5dcd72a0 100644 --- a/test/success.test.js +++ b/test/success.test.js @@ -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 diff --git a/test/verify.test.js b/test/verify.test.js index 84831f34..6027828d 100644 --- a/test/verify.test.js +++ b/test/verify.test.js @@ -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 => {