Skip to content

Cloning a Request with a FormData body hangs the process #52167

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.

Sign up for GitHub

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

Jump to bottom
Closed
kettanaito opened this issue Mar 20, 2024 · 8 comments
Closed

Cloning a Request with a FormData body hangs the process #52167

kettanaito opened this issue Mar 20, 2024 · 8 comments
Assignees
@mcollina

Comments

@kettanaito
Copy link

kettanaito commented Mar 20, 2024 •
edited
Loading

Version

v18.19.0, v20.11.0, v21.7.0

Platform

Darwin Artems-Air.home 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:59:33 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T8112 arm64

Subsystem

No response

What steps will reproduce the bug?

// index.js
const data = new FormData()
data.set('foo', 'bar')

const request = new Request('http://localhost', { method: 'POST', body: data })
request.clone()
console.log('cloned')
node ./index.js

How often does it reproduce? Is there a required condition?

This reproduces reliably 100% of the time.

What is the expected behavior? Why is that the expected behavior?

The process exits with 0. If you replace the request body with any other supported value, the process does exit.

What do you see instead?

The process hangs forever, failing to execute request.clone() when a Request instance has FormData as its body. Seems to be sensitive to FormData with values. Removing the data.set(...) call makes the issue go away (nothing to copy while cloning the request, I presume).

Additional information

Originally reported in MSW: mswjs/msw#2078
This was referenced Mar 20, 2024
Closed
Closed
@kettanaito
Copy link
Author

This is also very likely an issue with Undici since that's what implements FormData (nodejs/undici#2413). Reporting it here for posterity mostly.

@kettanaito kettanaito mentioned this issue Mar 20, 2024
Closed
4 tasks
@mcollina
Copy link
Member

This is fixed in undici v6.7.0. The fix for this will likely come to Node.js v21 very soon. Then we need to think what we want to do for Node.js v18 and v20 (which ships undici v5).

cc @nodejs/undici

@kettanaito
Copy link
Author

@mcollina, incredible! Is the backport viable for the fix?

@kettanaito kettanaito mentioned this issue Mar 21, 2024
Merged
28 tasks
@mcollina
Copy link
Member

I actually have no idea what fixed it, and if we should backport to v5 or just update v18 and v20 to latest undici.

@mcollina mcollina mentioned this issue Mar 21, 2024
Closed
@kettanaito
Copy link
Author

If possible, updating Undici to the latest sounds like a good strategy 👍

@mcollina
Copy link
Member

@nodejs/releasers any objections to update to Undici v6 in the next v20 release?

@mcollina mcollina self-assigned this Apr 19, 2024
@richardlau
Copy link
Member

richardlau commented Apr 19, 2024 •
edited
Loading

@nodejs/releasers any objections to update to Undici v6 in the next v20 release?

No objections, on the basis that the change is not breaking for users of Node.js, but it may have to be done in a backport pull request as I'm not sure the commits will land cleanly as there were separate updates on Node.js 20 for Undici 5.x.

@mcollina
Copy link
Member

mcollina commented May 2, 2024

@marco-ippolito the updates are already in #52793. Closing as this will come out in the next few days.

@mcollina mcollina closed this as completed May 2, 2024
Soltus added a commit to Hi-Windom/Sillot that referenced this issue May 21, 2024
@Soltus
https://github.com/nodejs/node/issues/52167
Loading
Loading status checks…
f4ceb1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mcollina mcollina

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mcollina @richardlau @kettanaito