fix(rate-limit): revert rate-limit logic back to counting retries per…

… request ID (#79)
contentful · Jun 5, 2019 · c3489d3 · c3489d3
1 parent 9088f46
commit c3489d3
Show file tree

Hide file tree

Showing 2 changed files with 83 additions and 80 deletions.
diff --git a/lib/rate-limit.js b/lib/rate-limit.js
@@ -1,106 +1,90 @@
+
+const attempts = {}
 const defaultsByInstance = new Map()
 let networkErrorAttempts = 0
-let retryAttemps = 0
 
 export default function rateLimit (instance, defaults, maxRetry = 5) {
   defaultsByInstance.set(instance, defaults)
   const instanceDefaults = defaultsByInstance.get(instance)
-  const {
-    responseLogger = () => undefined,
-    requestLogger = () => undefined
-  } = instanceDefaults
+  const { responseLogger = () => undefined, requestLogger = () => undefined } = instanceDefaults
+
+  instance.interceptors.request.use(function (config) {
+    requestLogger(config)
+    return config
+  }, function (error) {
+    return Promise.reject(error)
+  })
 
-  instance.interceptors.request.use(
-    function (config) {
-      requestLogger(config)
-      return config
-    },
-    function (error) {
+  instance.interceptors.response.use(function (response) {
+    // we don't need to do anything here
+    responseLogger(response)
+    return response
+  }, function (error) {
+    let { response, config } = error
+    // Do not retry if it is disabled or no request config exists (not an axios error)
+    if (!config || !instanceDefaults.retryOnError) {
       return Promise.reject(error)
     }
-  )
 
-  instance.interceptors.response.use(
-    function (response) {
-      // we don't need to do anything here
-      retryAttemps = 0 // reset when a response is a success
-      responseLogger(response)
-      return response
-    },
-    function (error) {
-      let { response, config } = error
-      // Do not retry if it is disabled or no request config exists (not an axios error)
-      if (!config || !instanceDefaults.retryOnError) {
-        return Promise.reject(error)
-      }
+    let retryErrorType = null
+    let wait = 0
 
-      let retryErrorType = null
-      let wait = 0
+    // Errors without response did not recieve anything from the server
+    if (!response) {
+      retryErrorType = 'Connection'
+      networkErrorAttempts++
 
-      // Errors without response did not recieve anything from the server
-      if (!response) {
-        retryErrorType = 'Connection'
-        networkErrorAttempts++
-
-        if (networkErrorAttempts > maxRetry) {
-          error.attempts = networkErrorAttempts
-          return Promise.reject(error)
-        }
-
-        wait = Math.pow(Math.SQRT2, networkErrorAttempts)
-        response = {}
-      } else {
-        networkErrorAttempts = 0
+      if (networkErrorAttempts > maxRetry) {
+        error.attempts = networkErrorAttempts
+        return Promise.reject(error)
       }
 
-      // we reject if there are too much errors of with the same request id
-
-      if (response.status >= 500 && response.status < 600) {
-        // 5** errors are server related
-        retryErrorType = `Server ${response.status}`
-        retryAttemps++
+      wait = Math.pow(Math.SQRT2, networkErrorAttempts)
+      response = {}
+    } else {
+      networkErrorAttempts = 0
+    }
 
-        wait = Math.pow(Math.SQRT2, retryAttemps)
-      } else if (response.status === 429) {
-        // 429 errors are exceeded rate limit exceptions
-        retryErrorType = 'Rate limit'
-        retryAttemps++
-        // all headers are lowercased by axios https://github.com/mzabriskie/axios/issues/413
-        if (
-          response.headers &&
-          error.response.headers['x-contentful-ratelimit-reset']
-        ) {
-          wait = response.headers['x-contentful-ratelimit-reset']
-        }
-      }
+    if (response.status >= 500 && response.status < 600) {
+      // 5** errors are server related
+      retryErrorType = `Server ${response.status}`
+      const headers = response.headers || {}
+      const requestId = headers['x-contentful-request-id'] || null
+      attempts[requestId] = attempts[requestId] || 0
+      attempts[requestId]++
 
-      if (retryAttemps > maxRetry) {
-        error.attempts = retryAttemps
+      // we reject if there are too many errors with the same request id or request id is not defined
+      if (attempts[requestId] > maxRetry || !requestId) {
+        error.attempts = attempts[requestId]
         return Promise.reject(error)
       }
+      wait = Math.pow(Math.SQRT2, attempts[requestId])
+    } else if (response.status === 429) {
+      // 429 errors are exceeded rate limit exceptions
+      retryErrorType = 'Rate limit'
+      // all headers are lowercased by axios https://github.com/mzabriskie/axios/issues/413
+      if (response.headers && error.response.headers['x-contentful-ratelimit-reset']) {
+        wait = response.headers['x-contentful-ratelimit-reset']
+      }
+    }
 
-      const delay = ms =>
-        new Promise(resolve => {
-          setTimeout(resolve, ms)
-        })
+    const delay = ms => new Promise((resolve) => {
+      setTimeout(resolve, ms)
+    })
 
-      if (retryErrorType) {
-        // convert to ms and add jitter
-        wait = Math.floor(wait * 1000 + Math.random() * 200 + 500)
-        instanceDefaults.logHandler(
-          'warning',
-          `${retryErrorType} error occurred. Waiting for ${wait} ms before retrying...`
-        )
+    if (retryErrorType) {
+      // convert to ms and add jitter
+      wait = Math.floor(wait * 1000 + (Math.random() * 200) + 500)
+      instanceDefaults.logHandler('warning', `${retryErrorType} error occurred. Waiting for ${wait} ms before retrying...`)
 
-        /* Somehow between the interceptor and retrying the request the httpAgent/httpsAgent gets transformed from an Agent-like object
+      /* Somehow between the interceptor and retrying the request the httpAgent/httpsAgent gets transformed from an Agent-like object
          to a regular object, causing failures on retries after rate limits. Removing these properties here fixes the error, but retry
          requests still use the original http/httpsAgent property */
-        delete config.httpAgent
-        delete config.httpsAgent
+      delete config.httpAgent
+      delete config.httpsAgent
 
-        return delay(wait).then(() => instance(config))
-      }
-      return Promise.reject(error)
+      return delay(wait).then(() => instance(config))
     }
-  )
+    return Promise.reject(error)
+  })
 }
diff --git a/test/unit/rate-limit-test.js b/test/unit/rate-limit-test.js
@@ -170,6 +170,25 @@ test('Should Fail if it hits maxRetries', (t) => {
     })
 })
 
+test('Rejects error straight away when X-Contentful-Request-Id header is missing', (t) => {
+  const { client } = setupWithOneRetry()
+  mock.onGet('/error').replyOnce(500, 'error attempt')
+  mock.onGet('/error').replyOnce(200, 'works')
+
+  return client.get('/error')
+    .then((response) => {
+      t.fail('the request should return error')
+      teardown()
+    }).catch((error) => {
+      t.ok(error)
+      t.equals(error.response.data, 'error attempt')
+      t.equals(logHandlerStub.callCount, 0, 'did not log anything')
+      t.equals(error.message, 'Request failed with status code 500')
+      t.equals(error.attempts, 1)
+      teardown()
+    })
+})
+
 test('Rejects errors with strange status codes', (t) => {
   const { client } = setup()
   mock.onGet('/error').replyOnce(765, 'error attempt')