-
Notifications
You must be signed in to change notification settings - Fork 570
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
feat: implement spec-compliant body mixins #1694
Conversation
Codecov ReportBase: 93.98% // Head: 94.05% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1694 +/- ##
==========================================
+ Coverage 93.98% 94.05% +0.06%
==========================================
Files 53 53
Lines 4907 4912 +5
==========================================
+ Hits 4612 4620 +8
+ Misses 295 292 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
I will make a follow up PR for the formData parsing, since it's much harder than everything else. |
this feels like a worse solution to first consume the hole body first. a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
According to the spec, you do have to consume the whole body. See: https://fetch.spec.whatwg.org/#ref-for-fully-reading-body-as-promise%E2%91%A0 With this assumption (that the whole body is in memory), the only logical outcome is that, similar to every other body mixin, the parsing happens synchronously. Every other platform has similarly came up with this solution. |
As far as I understand the specification specifies how to give the result, but not how we process it under the hood. A simple example showing memory consumption when reading the whole body before decoding instead of decoding by chunks. const { randomFillSync } = require('crypto');
const { ReadableStream } = require('node:stream/web');
const { Request } = require('.');
function stream(length) {
const buffer = Buffer.alloc(32 * 1024);
const encoder = new TextEncoder();
let size = 0;
return new ReadableStream({
pull(ctr) {
const data = encoder.encode(randomFillSync(buffer).toString('base64'));
ctr.enqueue(data);
if ((size += data.length) > length) ctr.close();
}
})
}
void async function() {
const request = new Request('http://localhost', {
method: 'POST',
duplex: 'half',
body: stream(256 * 1024 * 1024),
});
await request.text();
console.log(`RSS: ${(process.memoryUsage.rss() / 1024 / 1024).toFixed(2)} MB`);
}(); beforeRSS: 365.86 MB afterRSS: 601.42 MB |
* feat: implement spec-compliant body mixins * fix: skip tests on v16.8
* feat: implement spec-compliant body mixins * fix: skip tests on v16.8
Improves performance of
.text()
,.arrayBuffer()
,.json()
, and.blob()
by 60%.The next step is to introduce a synchronous FormData parser - similar to what every other runtime has. It doesn't make sense to asynchronously parse the FormData if all of the bytes are in memory.
If a library needs asynchronous parsing:
As mentioned in the issue implementing
.formData
, it's very inefficient and should never be used on the server.Firefox - https://github.com/mozilla/gecko-dev/blob/7f3ff3f4d34e7d234da4f5f5b345d2add7f30e95/dom/base/BodyUtil.cpp#L67
Chromium - https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/fetch/multipart_parser.cc;l=1;bpv=1;bpt=0
Deno - https://github.com/denoland/deno/blob/0cd05d737729b4cfab1d5e22077b3b9ad4ed5e30/ext/fetch/21_formdata.js#L490
Webkit - https://github.com/WebKit/WebKit/blob/f0350d6575c366d884d98f9937e77fe499b93398/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp#L129