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

PDE-2826 fix(core): backpressure issue with node-fetch patch #461

Merged
merged 4 commits into from
Dec 3, 2021

Conversation

eliangcs
Copy link
Member

@eliangcs eliangcs commented Dec 2, 2021

Using node-fetch 2.6.6 (Node.js 14), this program hangs on my laptop:

const fs = require('fs');
const fetch = require('node-fetch');

async function main() {
  // 36864 bytes = 16 KB
  // If it's 36863, the bug won't happen
  const downloadUrl = 'https://httpbin.org/stream-bytes/36864';
  const downloadResponse = await fetch(downloadUrl);

  const uploadUrl = 'https://httpbin.org/put';
  const uploadOpts = {
    method: 'PUT',
    body: downloadResponse.body,
  };

  const uploadRequest = new fetch.Request(uploadUrl, uploadOpts);
  const uploadPromise = fetch(uploadRequest);
  const uploadResponse = await uploadPromise;
  console.log(uploadResponse.status);
}

main();

When you pass a fetch.Request object to fetch(), fetch() copies the Request object. And if the Request has a streamable body, the Request constructor clones the body by teeing the stream into two PassThrough streams. Then we read one of the PassThrough stream (A) and not the other (B). This causes B's internal buffer to fill up, so Node.js has to pause reading from the source until someone consumes B's buffer.

To fix, we add that someone to consume B. Right after the fetch(uploadRequest) call, we pipe uploadRequest.body to /dev/null:

  const uploadPromise = fetch(uploadRequest);

  uploadRequest.body.pipe(fs.createWriteStream('/dev/null'));  // <---

  const uploadResponse = await uploadPromise;

And that solves the hanging problem.

This PR does something similar with our node-fetch patch.

@eliangcs eliangcs changed the base branch from release-9.x to master December 2, 2021 09:14
@eliangcs eliangcs changed the title Pde 2826 body clone backpressure PDE-2826 fix(core): piping request body hangs (backpressure issue) Dec 2, 2021
@eliangcs eliangcs marked this pull request as ready for review December 2, 2021 13:54
@eliangcs eliangcs requested a review from xavdid as a code owner December 2, 2021 13:54
@eliangcs eliangcs changed the title PDE-2826 fix(core): piping request body hangs (backpressure issue) PDE-2826 fix(core): backpessure issue with node-fetch patch Dec 2, 2021
@eliangcs eliangcs changed the title PDE-2826 fix(core): backpessure issue with node-fetch patch PDE-2826 fix(core): backpressure issue with node-fetch patch Dec 2, 2021
xavdid
xavdid previously approved these changes Dec 2, 2021
Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

wowza! Nice one

@eliangcs eliangcs merged commit 0032a88 into master Dec 3, 2021
@eliangcs eliangcs deleted the PDE-2826-body-clone-backpressure branch December 3, 2021 05:28
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.

None yet

2 participants