Merge c158d9c into f369337

bugsnag · May 7, 2019 · 4100b62 · 4100b62
2 parents f369337 + c158d9c
commit 4100b62
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 16 deletions.
diff --git a/lib/request.js b/lib/request.js
@@ -2,14 +2,15 @@
 
 const request = require('request')
 const MAX_ATTEMPTS = 5
-const RETRY_INTERVAL = process.env.BUGSNAG_RETRY_INTERVAL || 1000
+const RETRY_INTERVAL = parseInt(process.env.BUGSNAG_RETRY_INTERVAL) || 1000
+const TIMEOUT = parseInt(process.env.BUGSNAG_TIMEOUT) || 30000
 
 module.exports = (url, data) => {
   return new Promise((resolve, reject) => {
     let attempts = 0
     const maybeRetry = (err) => {
       attempts++
-      if (err && err.isRetryable && attempts < MAX_ATTEMPTS) return setTimeout(go, RETRY_INTERVAL)
+      if (err && err.isRetryable !== false && attempts < MAX_ATTEMPTS) return setTimeout(go, RETRY_INTERVAL)
       return reject(err)
     }
     const go = () => send(url, data, resolve, maybeRetry)
@@ -18,15 +19,29 @@ module.exports = (url, data) => {
 }
 
 const send = (url, formData, onSuccess, onError) => {
-  request.post({ url, formData }, (err, res, body) => {
-    if (err || res.statusCode !== 200) {
-      err = err || new Error(`${res.statusMessage} (${res.statusCode}) - ${body}`)
-      if (res && (res.statusCode < 400 || res.statusCode >= 500)) {
-        err.isRetryable = true
+  try {
+    request.post({ url, formData, timeout: TIMEOUT }, (err, res, body) => {
+      if (err || res.statusCode !== 200) {
+        err = err || new Error(`${res.statusMessage} (${res.statusCode}) - ${body}`)
+        if (res && !isRetryable(res.statusCode)) {
+          err.isRetryable = false
+        }
+        onError(err)
+      } else {
+        onSuccess()
       }
-      onError(err)
-    } else {
-      onSuccess()
-    }
-  })
+    })
+  } catch (e) {
+    onError(e)
+  }
+}
+
+const isRetryable = status => {
+  return (
+    status < 400 ||
+    status > 499 ||
+    [
+      408, // timeout
+      429 // too many requests
+    ].indexOf(status) !== -1)
 }
diff --git a/test/integration.test.js b/test/integration.test.js
@@ -1,11 +1,13 @@
 'use strict'
 
-/* global test, expect, beforeEach, afterEach */
+/* global test, expect, beforeEach, afterEach, fail */
 
 process.env.BUGSNAG_RETRY_INTERVAL = 100
+process.env.BUGSNAG_TIMEOUT = 100
 
 const express = require('express')
 const upload = require('../').upload
+const net = require('net')
 
 beforeEach(() => createTestServer())
 afterEach(() => closeTestServer())
@@ -41,6 +43,46 @@ test('it retries upon 50x failure', () => {
   })
 })
 
+test('it retries upon socket hangup', () => {
+  let n = 0
+  app.post('/', (req, res) => {
+    n++
+    if (n < 5) return req.connection.destroy()
+    return res.sendStatus(200)
+  })
+  return upload({
+    apiKey: 'API_KEY',
+    endpoint: `http://localhost:${server.address().port}`,
+    sourceMap: `${__dirname}/fixtures/noop.min.js.map`
+  }).then(() => {
+    expect(n).toBe(5)
+  })
+})
+
+test('it retries upon timeout', (done) => {
+  let n = 0
+  const socketServer = net.createServer(socket => {
+    n++
+    // this socket server never says anything
+  })
+  socketServer.listen(() => {
+    upload({
+      apiKey: 'API_KEY',
+      endpoint: `http://localhost:${socketServer.address().port}`,
+      sourceMap: `${__dirname}/fixtures/noop.min.js.map`
+    }).then(() => {
+      socketServer.close()
+      fail(new Error('expected promise to be rejected'))
+    }).catch(e => {
+      socketServer.close()
+      expect(n).toBe(5)
+      expect(e).toBeTruthy()
+      expect(e.code).toBe('ESOCKETTIMEDOUT')
+      done()
+    })
+  })
+})
+
 test('it eventually gives up retrying', () => {
   let n = 0
   app.post('/', (req, res) => {
@@ -52,7 +94,7 @@ test('it eventually gives up retrying', () => {
     endpoint: `http://localhost:${server.address().port}`,
     sourceMap: `${__dirname}/fixtures/noop.min.js.map`
   }).then(() => {
-    throw new Error('expected promise to be rejected')
+    fail(new Error('expected promise to be rejected'))
   }).catch(err => {
     expect(n).toBe(5)
     expect(err).toBeTruthy()
@@ -70,7 +112,7 @@ test('it doesn’t retry on a 40x failure', () => {
     endpoint: `http://localhost:${server.address().port}`,
     sourceMap: `${__dirname}/fixtures/noop.min.js.map`
   }).then(() => {
-    throw new Error('expected promise to be rejected')
+    fail(new Error('expected promise to be rejected'))
   }).catch(err => {
     expect(err).toBeTruthy()
     expect(n).toBe(1)
@@ -85,7 +127,7 @@ test('it returns the correct error in a synchronous failure', () => {
     endpoint: `1231..;`,
     sourceMap: `${__dirname}/fixtures/noop.min.js.map`
   }).then(() => {
-    throw new Error('expected promise to be rejected')
+    fail(new Error('expected promise to be rejected'))
   }).catch(err => {
     expect(err).toBeTruthy()
     expect(err.message).toBe('Invalid URI "1231..;"')