Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 [RUM-2721] fix upload retry #1180

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Feb 7, 2024

What and why?

This is another attempt to fix #1129

The file upload retry mechanism is incorrect as it tries to upload the same stream instances multiple times. When a FormData (from the form-data package) is built with a used stream, the resulting stream never ends, so the request never ends, resulting in a "socket hang up" error.

Minimal example with form-data only
import FormData from "form-data";
import fs from "fs";

(async () => {
  const stream = fs.createReadStream("./repro.mjs");
  await sendStream(stream); // This works, prints "Start, Data, Data, End"
  await sendStream(stream); // This does not end, prints only "Start"
})();

function sendStream(stream) {
  return new Promise((resolve, reject) => {
    const formData = new FormData();
    formData.append("file", stream);
    formData.on("error", reject);

    formData.resume();

    console.log("Start");

    formData.on("data", () => {
      console.log("Data");
    });
    formData.on("end", () => {
      console.log("End");
      resolve();
    });
  });
}
Example with an actual request
import https from "https";
import FormData from "form-data";
import fs from "fs";

(async () => {
  const stream = fs.createReadStream("./repro.mjs");
  console.log("Sending first request...");
  console.log("First request files:", (await sendRequest(stream)).files); // succeeds
  console.log("Sending second request...");
  console.log("Second request files:", (await sendRequest(stream)).files); // throws "socket hang up"
})();

async function sendRequest(stream) {
  return new Promise((resolve, reject) => {
    const data = new FormData();
    data.append("file", stream);

    const request = https.request("https://httpbin.org/post", {
      method: "POST",
      headers: data.getHeaders(),
    });

    request.on("response", (response) => {
      const chunks = [];
      response.on("data", (chunk) => {
        chunks.push(chunk);
      });
      response.on("end", () => {
        resolve(JSON.parse(Buffer.concat(chunks).toString()));
      });
    });

    request.on("error", reject);

    data.pipe(request);
  });
}

How?

This PR solves the issue by making sure the MultipartPayload does not contain streams but only file paths, and the upload function is then responsible to create streams every time the request is retried.

Note

While this particular behavior happens when form-data is involved, this is not a form-data bug, but really an issue related to our own usage of streams. Without form-data:

  • pipe-ing a closed stream to a standard nodejs https.request sends an empty request
  • using a closed stream to the fetch nodejs function throws immediately

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)

@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/fix-retry-upload branch 2 times, most recently from 57ddaf5 to 6564439 Compare February 7, 2024 17:15
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Feb 7, 2024

Datadog Report

Branch report: benoit/fix-retry-upload
Commit report: 33732e9
Test service: datadog-ci-tests

✅ 0 Failed, 2952 Passed, 0 Skipped, 2m 36.04s Total duration (33.66s time saved)

@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/fix-retry-upload branch 2 times, most recently from 3a612db to 4ffdfb8 Compare February 7, 2024 18:05
@BenoitZugmeyer BenoitZugmeyer marked this pull request as ready for review February 8, 2024 09:18
@BenoitZugmeyer BenoitZugmeyer requested review from a team as code owners February 8, 2024 09:18
Copy link

@bcaudan bcaudan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BenoitZugmeyer BenoitZugmeyer merged commit 5c5d096 into master Feb 8, 2024
14 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/fix-retry-upload branch February 8, 2024 15:19
@Drarig29 Drarig29 mentioned this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Socket hang up while uploading sourcemaps
5 participants