From 8a1fa76d3f6278b833afe49bb4fa1edfb7f98100 Mon Sep 17 00:00:00 2001
From: Joe Lencioni <joe.lencioni@airbnb.com>
Date: Wed, 29 May 2024 09:50:47 -0500
Subject: [PATCH] Move FormData instantiation inside makeRequest retry

I have been troubleshooting a perplexing issue when updating the
happo.io package at Airbnb. It seems that on this new version when a
request fails and is retried it ends up erroring out with a socket hang
up error after 60 seconds.

After some investigation, this error seems to be exactly what was
reported here:

  https://github.com/node-fetch/node-fetch/issues/1743

The issue is that the `FormData` instance is being reused but since its
buffer is already consumed it puts fetch into a bad state where it never
actually sends the request and hangs until it hits a socket timeout of
60 seconds. The proposed fix is to always use a fresh `FormData`
instance.

In this commit I am moving the prepareFormData call to inside of the
retry function so that we get a fresh FormData instance on each retry. I
am hopeful that this will resolve this issue.

I added a test to cover this behavior. When I run the test before my
change I observe that the test suite hangs when making this request.
After my change everything works as expected. This makes me fairly
confident in the value of this change.
---
 src/makeRequest.js       | 15 +++++++++-----
 test/makeRequest-test.js | 43 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/src/makeRequest.js b/src/makeRequest.js
index a6dd51d..25fce1f 100644
--- a/src/makeRequest.js
+++ b/src/makeRequest.js
@@ -38,15 +38,20 @@ export default async function makeRequest(
   { HTTP_PROXY } = process.env,
 ) {
   const { url, method = 'GET', formData, body: jsonBody } = requestAttributes;
-  const body = formData
-    ? prepareFormData(formData)
-    : jsonBody
-    ? JSON.stringify(jsonBody)
-    : undefined;
 
   return asyncRetry(
     async () => {
       const start = Date.now();
+
+      // We must avoid reusing FormData instances when retrying requests
+      // because they are consumed and cannot be reused.
+      // More info: https://github.com/node-fetch/node-fetch/issues/1743
+      const body = formData
+        ? prepareFormData(formData)
+        : jsonBody
+        ? JSON.stringify(jsonBody)
+        : undefined;
+
       const signed = jwt.sign({ key: apiKey }, apiSecret, {
         header: { kid: apiKey },
       });
diff --git a/test/makeRequest-test.js b/test/makeRequest-test.js
index 9df0999..e09bd2f 100644
--- a/test/makeRequest-test.js
+++ b/test/makeRequest-test.js
@@ -35,7 +35,7 @@ beforeAll(async () => {
           result: 'Hello world!',
         }),
       );
-    } else if (req.url === '/form-data') {
+    } else if (req.url === '/form-data' || (req.url === '/form-data-failure-retry' && errorTries > 2)) {
       const form = new multiparty.Form();
 
       form.parse(req, (err, fields, files) => {
@@ -146,6 +146,47 @@ it('can upload form data with buffers', async () => {
   });
 });
 
+it('can retry uploading form data with buffers', async () => {
+  props.url = 'http://localhost:8990/form-data-failure-retry';
+  props.method = 'POST';
+  props.formData = {
+    type: 'browser-chrome',
+    targetName: 'chrome',
+    payloadHash: 'foobar',
+    payload: {
+      options: {
+        filename: 'payload.json',
+        contentType: 'application/json',
+      },
+      value: Buffer.from('{"foo": "bar"}'),
+    },
+  };
+  const response = await subject();
+  expect(response).toEqual({
+    fields: {
+      payloadHash: ['foobar'],
+      targetName: ['chrome'],
+      type: ['browser-chrome'],
+    },
+    files: {
+      payload: [
+        {
+          fieldName: 'payload',
+          headers: {
+            'content-disposition':
+              'form-data; name="payload"; filename="payload.json"',
+            'content-type': 'application/json',
+          },
+          originalFilename: 'payload.json',
+          path: expect.any(String),
+          size: 14,
+        },
+      ],
+    },
+  });
+});
+
+
 it('can upload form data with streams', async () => {
   props.url = 'http://localhost:8990/form-data';
   props.method = 'POST';