From 681572419fe22d36bb1554723764b4e6998e038a Mon Sep 17 00:00:00 2001
From: Prateek Srivastava <f2prateek@gmail.com>
Date: Wed, 10 Jan 2018 16:14:40 -0800
Subject: [PATCH] Improve Retry Behaviour.

Previously we were only retrying network errors or a 5xx error on an idempotent request (GET, HEAD, OPTIONS, PUT or DELETE).

The latter doesn't really guard/apply to us since we use POST for uploading messages. Our POST requests are effectively idempotent since we send messages with a unique message ID that is guarded by our dedupe layer (https://segment.com/blog/exactly-once-delivery/).

For us, our retry policy should be (as per https://paper.dropbox.com/doc/analytics-foo-library-guidelines-2trBhLKQD4Soz4VatvuGR#:uid=189560039280582553736180&h2=Handling-Network-Errors):

1. Retry network errors (socket timed out, etc.)
2. Retry server errors (HTTP 5xx)
3. Retry HTTP 429.
---
 index.js | 31 +++++++++++++++++++++++++++++--
 test.js  | 17 +++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/index.js b/index.js
index e68d8b9c..8c2efccc 100644
--- a/index.js
+++ b/index.js
@@ -4,7 +4,7 @@ const assert = require('assert')
 const removeSlash = require('remove-trailing-slash')
 const validate = require('@segment/loosely-validate-event')
 const axios = require('axios')
-const retries = require('axios-retry')
+const axiosRetry = require('axios-retry')
 const ms = require('ms')
 const uid = require('crypto-token')
 const version = require('./package.json').version
@@ -38,7 +38,10 @@ class Analytics {
     this.flushInterval = options.flushInterval || 10000
     this.flushed = false
 
-    retries(axios, options.retryCount || 3)
+    axiosRetry(axios, {
+      retries: options.retryCount || 3,
+      retryCondition: this._isErrorRetryable
+    })
   }
 
   /**
@@ -247,6 +250,30 @@ class Analytics {
         done(err)
       })
   }
+
+  _isErrorRetryable (error) {
+    // Retry Network Errors.
+    if (axiosRetry.isNetworkError(error)) {
+      return true
+    }
+
+    if (!error.response) {
+      // Cannot determine if the request can be retried
+      return false
+    }
+
+    // Retry Server Errors (5xx).
+    if (error.response.status >= 500 && error.response.status <= 599) {
+      return true
+    }
+
+    // Retry if rate limited.
+    if (error.response.status === 429) {
+      return true
+    }
+
+    return false
+  }
 }
 
 module.exports = Analytics
diff --git a/test.js b/test.js
index 197d9e20..8c527ae6 100644
--- a/test.js
+++ b/test.js
@@ -503,6 +503,23 @@ test('alias - require previousId and userId', t => {
   })
 })
 
+test('isErrorRetryable', t => {
+  const client = createClient()
+
+  t.false(client._isErrorRetryable({}))
+
+  // ETIMEDOUT is retryable as per `is-retry-allowed` (used by axios-retry in `isNetworkError`).
+  t.true(client._isErrorRetryable({ code: 'ETIMEDOUT' }))
+
+  // ECONNABORTED is not retryable as per `is-retry-allowed` (used by axios-retry in `isNetworkError`).
+  t.false(client._isErrorRetryable({ code: 'ECONNABORTED' }))
+
+  t.true(client._isErrorRetryable({ response: { status: 500 } }))
+  t.true(client._isErrorRetryable({ response: { status: 429 } }))
+
+  t.false(client._isErrorRetryable({ response: { status: 200 } }))
+})
+
 const { RUN_E2E_TESTS } = process.env
 
 if (RUN_E2E_TESTS) {