Merge pull request #1803 from stripe/richardm-do-one-network-retry-by-default

richardm-stripe · web-flow · commit be686fe57e72 · 2023-06-02T13:22:22.000-07:00
(next major) one network retry by default
diff --git a/README.md b/README.md
@@ -186,7 +186,7 @@ const stripe = Stripe('sk_test_...', {
 | Option              | Default            | Description                                                                                                                                                                                                                                       |
 | ------------------- | ------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | `apiVersion`        | `null`             | Stripe API version to be used. If not set, stripe-node will use the latest version at the time of release.                                                                                                                                        |
-| `maxNetworkRetries` | 0                  | The amount of times a request should be [retried](#network-retries).                                                                                                                                                                              |
+| `maxNetworkRetries` | 1                  | The amount of times a request should be [retried](#network-retries).                                                                                                                                                                              |
 | `httpAgent`         | `null`             | [Proxy](#configuring-a-proxy) agent to be used by the library.                                                                                                                                                                                    |
 | `timeout`           | 80000              | [Maximum time each request can take in ms.](#configuring-timeout)                                                                                                                                                                                 |
 | `host`              | `'api.stripe.com'` | Host that requests are made to.                                                                                                                                                                                                                   |
@@ -253,10 +253,14 @@ if (process.env.http_proxy) {
 
 ### Network retries
 
-Automatic network retries can be enabled with the `maxNetworkRetries` config option.
-This will retry requests `n` times with exponential backoff if they fail due to an intermittent network problem.
-[Idempotency keys](https://stripe.com/docs/api/idempotent_requests) are added where appropriate to prevent duplication.
+As of [v13](https://github.com/stripe/stripe-node/releases/tag/v13.0.0) stripe-node will automatically do one reattempt for failed requests that are safe to retry. Automatic network retries can be disabled by setting the `maxNetworkRetries` config option to `0`. You can also set a higher number to reattempt multiple times, with exponential backoff. [Idempotency keys](https://stripe.com/docs/api/idempotent_requests) are added where appropriate to prevent duplication.
 
+```js
+const stripe = Stripe('sk_test_...', {
+  maxNetworkRetries: 0, // Disable retries
+});
+
+```
 ```js
 const stripe = Stripe('sk_test_...', {
   maxNetworkRetries: 2, // Retry a request twice before giving up
diff --git a/src/stripe.core.ts b/src/stripe.core.ts
@@ -113,7 +113,7 @@ export function createStripe(
       maxNetworkRetries: validateInteger(
         'maxNetworkRetries',
         props.maxNetworkRetries,
-        0
+        1
       ),
       agent: agent,
       httpClient:
diff --git a/test/RequestSender.spec.ts b/test/RequestSender.spec.ts
@@ -92,7 +92,7 @@ describe('RequestSender', () => {
     // Use a real instance of stripe as we're mocking the http.request responses.
     const realStripe = require('../src/stripe.cjs.node.js')('sk_test_xyz');
 
-    after(() => {
+    afterEach(() => {
       nock.cleanAll();
     });
 
@@ -337,7 +337,9 @@ describe('RequestSender', () => {
 
   describe('Retry Network Requests', () => {
     // Use a real instance of stripe as we're mocking the http.request responses.
-    const realStripe = require('../src/stripe.cjs.node.js')('sk_test_xyz');
+    const realStripe = require('../src/stripe.cjs.node.js')(FAKE_API_KEY, {
+      maxNetworkRetries: 0,
+    });
 
     // Override the sleep timer to speed up tests
     realStripe.charges._getSleepTimeInMS = () => 0;
@@ -359,10 +361,6 @@ describe('RequestSender', () => {
       stripe._setApiNumberField('maxNetworkRetries', 0);
     });
 
-    after(() => {
-      nock.cleanAll();
-    });
-
     describe('_request', () => {
       it('throws an error on connection failure', (done) => {
         // Mock the connection error.
@@ -378,7 +376,7 @@ describe('RequestSender', () => {
 
       it('throws an error on connection timeout', (done) => {
         return getTestServerStripe(
-          {timeout: 10},
+          {timeout: 10, maxNetworkRetries: 0},
           (req, res) => {
             // Do nothing. This will trigger a timeout.
           },
@@ -603,6 +601,60 @@ describe('RequestSender', () => {
         });
       });
 
+      it('should give precedence to request-level (1) vs client-level maxNetworkRetries (0)', (done) => {
+        let nReceivedRequests = 0;
+        nock(`https://${options.host}`)
+          .post(options.path, options.params)
+          .reply((_1, _2, callback) => {
+            nReceivedRequests += 1;
+            callback(new Error('bad stuff'));
+          })
+          .post(options.path, options.params)
+          .reply((_1, _2, callback) => {
+            nReceivedRequests += 1;
+            callback(new Error('worse stuff'));
+          });
+
+        realStripe._setApiNumberField('maxNetworkRetries', 0);
+
+        realStripe.charges.create(
+          options.data,
+          {maxNetworkRetries: 1},
+          (err) => {
+            const errorMessage = RequestSender._generateConnectionErrorMessage(
+              1
+            );
+            expect(err.message).to.equal(errorMessage);
+            expect(err.detail.message).to.deep.equal('worse stuff');
+            expect(nReceivedRequests).to.equal(2);
+            done();
+          }
+        );
+      });
+
+      it('should give precedence to request-level (0) vs client-level maxNetworkRetries (1)', (done) => {
+        nock(`https://${options.host}`)
+          .post(options.path, options.params)
+          .reply((_1, _2, callback) => {
+            callback(new Error('bad stuff'));
+          });
+
+        realStripe._setApiNumberField('maxNetworkRetries', 1);
+
+        realStripe.charges.create(
+          options.data,
+          {maxNetworkRetries: 0},
+          (err) => {
+            expect(err.detail.message).to.deep.equal('bad stuff');
+            const errorMessage = RequestSender._generateConnectionErrorMessage(
+              0
+            );
+            expect(err.message).to.equal(errorMessage);
+            done();
+          }
+        );
+      });
+
       it('should retry on a 409 error', (done) => {
         nock(`https://${options.host}`)
           .post(options.path, options.params)
diff --git a/test/StripeResource.spec.ts b/test/StripeResource.spec.ts
@@ -211,7 +211,7 @@ describe('StripeResource', () => {
     });
 
     it('failure', (callback) => {
-      const handleRequest = (req, res) => {
+      const handleRequest = (_req, res, nPreviousRequests) => {
         setTimeout(() => res.writeHead(500));
         setTimeout(
           () =>
@@ -221,6 +221,8 @@ describe('StripeResource', () => {
           10
         );
         setTimeout(() => res.end(), 20);
+        // fail twice, then close the server
+        return {shouldStayOpen: nPreviousRequests < 1};
       };
 
       getTestServerStripe({}, handleRequest, (err, stripe, closeServer) => {
@@ -230,17 +232,20 @@ describe('StripeResource', () => {
 
         const foos = makeResourceWithPDFMethod(stripe);
 
-        return foos.pdf({id: 'foo_123'}, {host: 'localhost'}, (err, res) => {
-          closeServer();
-          expect(err).to.exist;
-          expect(err.raw.type).to.equal('api_error');
-          expect(err.raw.message).to.equal('this is bad');
-          return callback();
-        });
+        return foos.pdf(
+          {id: 'foo_123'},
+          {host: 'localhost', maxNetworkRetries: 1},
+          (err, res) => {
+            closeServer();
+            expect(err).to.exist;
+            expect(err.raw.type).to.equal('api_error');
+            expect(err.raw.message).to.equal('this is bad');
+            return callback();
+          }
+        );
       });
     });
   });
-
   describe('makeRequest args', () => {
     it('does not mutate user-supplied deprecated opts', () => {
       const args = [
diff --git a/test/resources/Quotes.spec.js b/test/resources/Quotes.spec.js
@@ -169,7 +169,7 @@ describe('Quotes Resource', () => {
     });
 
     it('failure', (callback) => {
-      const handleRequest = (req, res) => {
+      const handleRequest = (req, res, nPreviousRequests) => {
         setTimeout(() => res.writeHead(500));
         setTimeout(
           () =>
@@ -179,6 +179,7 @@ describe('Quotes Resource', () => {
           10
         );
         setTimeout(() => res.end(), 20);
+        return {shouldStayOpen: nPreviousRequests < 1};
       };
 
       testUtils.getTestServerStripe(
@@ -191,7 +192,7 @@ describe('Quotes Resource', () => {
 
           return stripe.quotes.pdf(
             'foo_123',
-            {host: 'localhost'},
+            {host: 'localhost', maxNetworkRetries: 1},
             (err, res) => {
               closeServer();
               expect(err).to.exist;
diff --git a/test/stripe.spec.ts b/test/stripe.spec.ts
@@ -586,12 +586,12 @@ describe('Stripe Module', function() {
     });
 
     describe('when passed in via the config object', () => {
-      it('should default to 0 if a non-integer is passed', () => {
+      it('should default to 1 if a non-integer is passed', () => {
         const newStripe = Stripe(FAKE_API_KEY, {
           maxNetworkRetries: 'foo',
         });
 
-        expect(newStripe.getMaxNetworkRetries()).to.equal(0);
+        expect(newStripe.getMaxNetworkRetries()).to.equal(1);
 
         expect(() => {
           Stripe(FAKE_API_KEY, {
@@ -613,7 +613,7 @@ describe('Stripe Module', function() {
       it('should use the default', () => {
         const newStripe = Stripe(FAKE_API_KEY);
 
-        expect(newStripe.getMaxNetworkRetries()).to.equal(0);
+        expect(newStripe.getMaxNetworkRetries()).to.equal(1);
       });
     });
   });
diff --git a/test/testUtils.ts b/test/testUtils.ts
@@ -25,16 +25,19 @@ export const getTestServerStripe = (
   clientOptions: RequestSettings,
   handler: (
     req: http.IncomingMessage,
-    res: http.ServerResponse
-  ) => {shouldStayOpen?: true} | null,
+    res: http.ServerResponse,
+    nPreviousRequests: number
+  ) => {shouldStayOpen?: boolean} | null,
   callback: (
     err: Error | null,
     stripe: StripeClient,
     closeServer: () => void
   ) => void
 ): void => {
+  let nPreviousRequests = 0;
   const server = http.createServer((req, res) => {
-    const {shouldStayOpen} = handler(req, res) || {};
+    const {shouldStayOpen} = handler(req, res, nPreviousRequests) || {};
+    nPreviousRequests += 1;
     if (!shouldStayOpen) {
       res.on('close', () => {
         server.close();
