Skip to content

Conversation

@philnash
Copy link
Member

The RFC 7578 spec for multipart/form-data requests does not require the body of a request to end with a CRLF, only that each section begins with a CRLF. While many clients implement multipart requests with the trailing CRLF, the implementation for fetch in Node.js version 22 and below does not.

This caused my a good number of hours debugging!

It does turn out that in September 2024 the Node.js fetch implementation added the CRLF, though this hasn't made it to a Node.js release yet.

This change allows the boundary to end with a CRLF or not, as long as the boundary is followed by the end of the request body.

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Jan 14, 2025
@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Jan 14, 2025
The RFC 7578 spec for multipart/form-data requests does not require the
body of a request to end with a CRLF, only that each section begins with
a CRLF. While many clients implement multipart requests with the
trailing CRLF, the implementation for fetch in Node.js version 22 and
below does not.

This caused my a good number of hours debugging!

It does turn out that in September 2024 the Node.js fetch implementation
added the CRLF (nodejs/undici#3625), though this
hasn't made it to a Node.js release yet.

This change allows the boundary to end with a newline or
not, as long as the boundary is followed by the end of the request body.
@philnash philnash force-pushed the multipart-boundary-end branch from ef9dea6 to 08fd912 Compare January 14, 2025 04:46
@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Jan 14, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 14, 2025

CodSpeed Performance Report

Merging #5660 will improve performances by 32.73%

Comparing philnash:multipart-boundary-end (08fd912) with main (e7a2005)

Summary

⚡ 2 improvements
✅ 13 untouched benchmarks

Benchmarks breakdown

Benchmark main philnash:multipart-boundary-end Change
test_successful_run_with_input_type_any 339.5 ms 255.8 ms +32.73%
test_successful_run_with_output_type_debug 277.6 ms 251.2 ms +10.51%

philnash added a commit to langflow-ai/langflow-client-ts that referenced this pull request Jan 14, 2025
This required installing undici, which is the more frequently updated version of Node's built in fetch. This is because Langflow doesn't consider a multipart/form-data request that doesn't end with a CRLF to be a valid request and the current version of undici embedded in Node doesn't add the CRLF. I've made a PR to Langflow to relax this requirement (langflow-ai/langflow#5660), and version 7 of undici has been merged to the Node main branch but not yet released in any version, at the time of writing.
Copy link
Contributor

@ogabrielluiz ogabrielluiz left a comment

Choose a reason for hiding this comment

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

Good catch!

Thanks, @philnash

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 21, 2025
@ogabrielluiz ogabrielluiz added this pull request to the merge queue Jan 21, 2025
Merged via the queue into langflow-ai:main with commit c7f6554 Jan 21, 2025
33 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants