feat(#8): onAbuseLimit/onRateLimit

BREAKING CHANGE: Changed `octokit.throttle.on('abuse-limit', handler)` and `'rate-limit'` to
```
const octokit = new Octokit({
  throttle: {
    onAbuseLimit: handler,
    onRateLimit: handler
  }
});
```

Author: SGrondin

README.md

@@ -11,13 +11,31 @@ Implements all [recommended best practises](https://developer.github.com/v3/guid
 
 ## Usage
 
-The code below creates a "Hello, world!" issue on every repository in a given organization. Without the throttling plugin it would send many requests in parallel and would hit rate limits very quickly. But the `@octokit/plugin-throttling` makes sure that no requests using the same authentication token are throttled correctly.
+The code below creates a "Hello, world!" issue on every repository in a given organization. Without the throttling plugin it would send many requests in parallel and would hit rate limits very quickly. But the `@octokit/plugin-throttling` slows down your requests according to the official guidelines, so you don't get blocked before your quota is exhausted.
+
+The `throttle.onAbuseLimit` and `throttle.onRateLimit` options are required. Return `true` to automatically retry the request after `retryAfter` seconds.
 
 ```js
-const Octokit = require('@ocotkit/rest')
+const Octokit = require('@octokit/rest')
   .plugin(require('@octokit/plugin-throttling'))
 
-const octokit = new Octokit()
+const octokit = new Octokit({
+  throttle: {
+    onRateLimit: (retryAfter, options) => {
+      console.warn(`Request quota exhausted for request ${options.method} ${options.url}`)
+
+      if (options.request.retryCount === 0) { // only retries once
+        console.log(`Retrying after ${retryAfter} seconds!`)
+        return true
+      }
+    },
+    onAbuseLimit: (retryAfter, options) => {
+      // does not retry, only logs a warning
+      console.warn(`Abuse detected for request ${options.method} ${options.url}`)
+    }
+  }
+})
+
 octokit.authenticate({
   type: 'token',
   token: process.env.TOKEN
@@ -35,13 +53,6 @@ async function createIssueOnAllRepos (org) {
 }
 ```
 
-Handle events
-
-```js
-octokit.throttle.on('rate-limit', (retryAfter) => console.warn(`Rate-limit hit, retrying after ${retryAfter}s`))
-octokit.throttle.on('abuse-limit', (retryAfter) => console.warn(`Abuse-limit hit, retrying after ${retryAfter}s`))
-```
-
 ## LICENSE
 
 [MIT](LICENSE)

lib/index.js

@@ -5,11 +5,11 @@ const wrapRequest = require('./wrap-request')
 
 const triggersNotificationPaths = require('./triggers-notification-paths')
 
-function throttlingPlugin (octokit) {
-  const state = {
+function throttlingPlugin (octokit, octokitOptions = {}) {
+  const state = Object.assign({
     triggersNotification: triggersNotificationPaths,
     minimumAbuseRetryAfter: 5,
-    maxRetries: 1,
+    retryAfterBaseValue: 1000,
     globalLimiter: new Bottleneck({
       maxConcurrent: 1
     }),
@@ -20,13 +20,67 @@ function throttlingPlugin (octokit) {
     triggersNotificationLimiter: new Bottleneck({
       maxConcurrent: 1,
       minTime: 3000
-    })
-  }
+    }),
+    retryLimiter: new Bottleneck()
+  }, octokitOptions.throttle)
+
+  if (typeof state.onAbuseLimit !== 'function' || typeof state.onRateLimit !== 'function') {
+    throw new Error(`octokit/plugin-throttling error:
+        You must pass the onAbuseLimit and onRateLimit error handlers.
+        See https://github.com/octokit/rest.js#throttling
 
-  octokit.throttle = {
-    _options: (options = {}) => Object.assign(state, options)
+        const octokit = new Octokit({
+          throttle: {
+            onAbuseLimit: (error, options) => {/* ... */},
+            onRateLimit: (error, options) => {/* ... */}
+          }
+        })
+    `)
   }
-  const emitter = new Bottleneck.Events(octokit.throttle)
 
-  octokit.hook.wrap('request', wrapRequest.bind(null, state, emitter))
+  const events = {}
+  const emitter = new Bottleneck.Events(events)
+  events.on('abuse-limit', state.onAbuseLimit)
+  events.on('rate-limit', state.onRateLimit)
+  events.on('error', e => console.warn('Error in throttling-plugin limit handler', e))
+
+  state.retryLimiter.on('failed', async function (error, info) {
+    if (error.status !== 403) {
+      return
+    }
+
+    const options = info.args[info.args.length - 1]
+    const retryCount = ~~options.request.retryCount
+    options.request.retryCount = retryCount
+
+    const { wantRetry, retryAfter } = await (async function () {
+      if (/\babuse\b/i.test(error.message)) {
+        // The user has hit the abuse rate limit.
+        // https://developer.github.com/v3/#abuse-rate-limits
+
+        // The Retry-After header can sometimes be blank when hitting an abuse limit,
+        // but is always present after 2-3s, so make sure to set `retryAfter` to at least 5s by default.
+        const retryAfter = Math.max(~~error.headers['retry-after'], state.minimumAbuseRetryAfter)
+        const wantRetry = await emitter.trigger('abuse-limit', retryAfter, options)
+        return { wantRetry, retryAfter }
+      }
+      if (error.headers['x-ratelimit-remaining'] === '0') {
+        // The user has used all their allowed calls for the current time period
+        // https://developer.github.com/v3/#rate-limiting
+
+        const rateLimitReset = new Date(~~error.headers['x-ratelimit-reset'] * 1000).getTime()
+        const retryAfter = Math.max(Math.ceil((rateLimitReset - Date.now()) / 1000), 0)
+        const wantRetry = await emitter.trigger('rate-limit', retryAfter, options)
+        return { wantRetry, retryAfter }
+      }
+      return {}
+    })()
+
+    if (wantRetry) {
+      options.request.retryCount++
+      return retryAfter * state.retryAfterBaseValue
+    }
+  })
+
+  octokit.hook.wrap('request', wrapRequest.bind(null, state))
 }

lib/wrap-request.js

@@ -1,13 +1,12 @@
-/* eslint padded-blocks: 0 */
 module.exports = wrapRequest
 
 const noop = () => Promise.resolve()
 
-async function wrapRequest (state, emitter, request, options) {
-  const retryRequest = function (after) {
-    return new Promise(resolve => setTimeout(resolve, after * 1000))
-      .then(() => wrapRequest(state, emitter, request, options))
-  }
+function wrapRequest (state, request, options) {
+  return state.retryLimiter.schedule(doRequest, state, request, options)
+}
+
+async function doRequest (state, request, options) {
   const isWrite = options.method !== 'GET' && options.method !== 'HEAD'
   const retryCount = ~~options.request.retryCount
   const jobOptions = retryCount > 0 ? { priority: 0, weight: 0 } : {}
@@ -22,40 +21,5 @@ async function wrapRequest (state, emitter, request, options) {
     await state.triggersNotificationLimiter.schedule(jobOptions, noop)
   }
 
-  return state.globalLimiter.schedule(jobOptions, async function () {
-    try {
-      // Execute request
-      return await request(options)
-    } catch (error) {
-      if (error.status === 403 && /\babuse\b/i.test(error.message)) {
-        // The user has hit the abuse rate limit.
-        // https://developer.github.com/v3/#abuse-rate-limits
-
-        // The Retry-After header can sometimes be blank when hitting an abuse limit,
-        // but is always present after 2-3s, so make sure to set `retryAfter` to at least 5s by default.
-        const retryAfter = Math.max(~~error.headers['retry-after'], state.minimumAbuseRetryAfter)
-        emitter.trigger('abuse-limit', retryAfter)
-
-        if (state.maxRetries > retryCount) {
-          options.request.retryCount = retryCount + 1
-          return retryRequest(retryAfter)
-        }
-
-      } else if (error.status === 403 && error.headers['x-ratelimit-remaining'] === '0') {
-        // The user has used all their allowed calls for the current time period
-        // https://developer.github.com/v3/#rate-limiting
-
-        const rateLimitReset = new Date(~~error.headers['x-ratelimit-reset'] * 1000).getTime()
-        const retryAfter = Math.max(Math.ceil((rateLimitReset - Date.now()) / 1000), 0)
-        emitter.trigger('rate-limit', retryAfter)
-
-        if (state.maxRetries > retryCount) {
-          options.request.retryCount = retryCount + 1
-          return retryRequest(retryAfter)
-        }
-      }
-
-      throw error
-    }
-  })
+  return state.globalLimiter.schedule(jobOptions, request, options)
 }

package-lock.json

@@ -35,7 +35,7 @@
     ]
   },
   "dependencies": {
-    "bottleneck": "^2.14.1"
+    "bottleneck": "^2.15.0"
   },
   "devDependencies": {
     "@octokit/request": "2.2.0",