fix(error handling): applies proper axios error handling (#24)

Author: axe312ger

lib/rate-limit.js

@@ -1,39 +1,47 @@
-export default function rateLimit (instance, maxRetry = 5) {
-  let attempt = 0
-  let lastErrorAt
+const attempts = {}
 
+export default function rateLimit (instance, maxRetry = 5) {
   instance.interceptors.response.use(function (response) {
     // we don't need to do anything here
     return response
   }, function (error) {
-    const {config} = error
-    if (!config || !instance.defaults.retryOnError) {
+    const {request, response, config} = error
+    if ((!request && !response) || !config || !instance.defaults.retryOnError) {
       return Promise.reject(error)
     }
-    // in axios reponse will be available only if there a response from server
-    // in this case will assume it is a 500 error
-    error.response = error.response || {status: 500}
-    if (error.response.status === 500) {
-      attempt++
-      lastErrorAt = lastErrorAt || Date.now()
-      lastErrorAt = Date.now()
-      // we reject if there are too much errors in a short period of time
-      if (attempt >= maxRetry) {
+
+    let retryErrorType = null
+    let wait = 0
+
+    if (!response) {
+      // Errors without response or config did not recieve anything from the server
+      retryErrorType = 'Connection'
+    } else 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]++
+
+      // we reject if there are too much errors of with the same request id
+      if (attempts[requestId] >= maxRetry || !requestId) {
         return Promise.reject(error)
       }
-    }
-
-    // retry if we receive 429 or 500
-    if (error.response.status === 429 || error.response.status === 500) {
-      let rateLimitHeaderValue = 0
+      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 (error.response.headers && error.response.headers['x-contentful-ratelimit-reset']) {
-        rateLimitHeaderValue = error.response.headers['x-contentful-ratelimit-reset']
+      if (response.headers && error.response.headers['x-contentful-ratelimit-reset']) {
+        wait = response.headers['x-contentful-ratelimit-reset']
       }
-      let wait = rateLimitHeaderValue > 0 ? rateLimitHeaderValue : Math.pow(Math.SQRT2, attempt)
+    }
+
+    if (retryErrorType) {
       // convert to ms and add jitter
       wait = Math.floor(wait * 1000 + (Math.random() * 200) + 500)
-      console.log(`${error.response.status === 429 ? 'Rate limit' : 'Server'} error occured. Waiting for ${wait} ms before retrying....`)
+      console.log(`${retryErrorType} error occured. Waiting for ${wait} ms before retrying....`)
       return new Promise((resolve) => {
         setTimeout(() => {
           resolve(instance(config))

test/unit/rate-limit-test.js

@@ -37,10 +37,31 @@ test('Retry on 429 after a duration >= rateLimit header', (t) => {
     teardown()
   })
 })
-test('Retry on 500', (t) => {
+test('Retry on 5** - multiple errors', (t) => {
   const { client } = setup()
-  mock.onGet('/rate-limit-me').replyOnce(500)
-  mock.onGet('/rate-limit-me').replyOnce(500)
+  mock.onGet('/rate-limit-me').replyOnce(500, 'Server Error', {'x-contentful-request-id': 1})
+  mock.onGet('/rate-limit-me').replyOnce(500, 'Server Error', {'x-contentful-request-id': 1})
+  mock.onGet('/rate-limit-me').replyOnce(200, 'works #1')
+  mock.onGet('/rate-limit-me').replyOnce(503, 'Another Server Error', {'x-contentful-request-id': 2})
+  mock.onGet('/rate-limit-me').replyOnce(200, 'works #2')
+  t.plan(5)
+  return client.get('/rate-limit-me').then((response) => {
+    t.ok(response.data)
+    t.equals(response.data, 'works #1')
+    const startTime = Date.now()
+    return client.get('/rate-limit-me').then((response) => {
+      t.ok(Date.now() - startTime <= 3000, 'First error should not influence second errors retry delay')
+      t.ok(response.data)
+      t.equals(response.data, 'works #2')
+      teardown()
+    })
+  })
+})
+// Disabled till new version of axios-mock-adapter is out
+// https://github.com/ctimmerm/axios-mock-adapter/issues/52
+test.skip('Retry on network error', (t) => {
+  const { client } = setup()
+  mock.onGet('/rate-limit-me').networkError()
   mock.onGet('/rate-limit-me').replyOnce(200, 'works')
   t.plan(2)
   return client.get('/rate-limit-me').then((response) => {
@@ -52,7 +73,7 @@ test('Retry on 500', (t) => {
 test('no retry when automatic handling flag is disabled', (t) => {
   const { client } = setupWithoutErrorRetry()
   const responseError = new Error('Mocked 500 Error')
-  mock.onGet('/rate-limit-me').replyOnce(500, responseError)
+  mock.onGet('/rate-limit-me').replyOnce(500, responseError, {'x-contentful-request-id': 3})
   t.plan(2)
   return client.get('/rate-limit-me').then((response) => {
     t.fail('Promise should reject not resolve')
@@ -66,14 +87,43 @@ test('no retry when automatic handling flag is disabled', (t) => {
 })
 test('Should Fail if it hits maxRetries', (t) => {
   const { client } = setupWithOneRetry()
-  mock.onGet('/error').replyOnce(500, 'error attempt #1')
-  mock.onGet('/error').replyOnce(500, 'error attempt #2')
+  mock.onGet('/error').replyOnce(500, 'error attempt #1', {'x-contentful-request-id': 4})
+  mock.onGet('/error').replyOnce(500, 'error attempt #2', {'x-contentful-request-id': 4})
   t.plan(2)
   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 #2')
     teardown()
   })
 })
+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')
+  t.plan(2)
+  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')
+    teardown()
+  })
+})
+test('Rejects errors with strange status codes', (t) => {
+  const { client } = setupWithOneRetry()
+  mock.onGet('/error').replyOnce(765, 'error attempt')
+  mock.onGet('/error').replyOnce(200, 'works')
+  t.plan(2)
+  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')
+    teardown()
+  })
+})