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

Undici's Fetch's Body takes a long time to load compared to just request #2014

Open
cakedan opened this issue Mar 17, 2023 · 14 comments
Open
Labels
bug Something isn't working

Comments

@cakedan
Copy link

cakedan commented Mar 17, 2023

Bug Description

I've noticed a bug where if the body of a fetch response is large (let us say above 50mb), grabbing the body contents takes a long time to load. I compared it to undici's normal request function and node-fetch's fetch response. In comparison, node-fetch took about 8.5 to 9.4 seconds to grab a 130mb file, undici's request function took 2.3 to 2.6 seconds, and undici's fetch took 95.5 to 104.2 seconds.

Reproducible By

undici fetch, 95.5 seconds, 18,605 buffers

await (async () => {
    const { fetch } = require('undici');

    const response = await fetch('https://alpha.notsobot.com/api/undici/test');
    let now = Date.now();
    const buffers = [];
    for await (let x of response.body) {
        buffers.push(x);
    }
    console.log(Date.now() - now);
    now = Date.now();
    JSON.parse(Buffer.concat(buffers));
    console.log(Date.now() - now);
})();

undici request, 2.6 seconds, 9794 buffers

await (async () => {
    const { request } = require('undici');

    const response = await request('https://alpha.notsobot.com/api/undici/test');
    let now = Date.now();
    const buffers = [];
    for await (let x of response.body) {
        buffers.push(x);
    }
    console.log(Date.now() - now);
    now = Date.now();
    JSON.parse(Buffer.concat(buffers));
    console.log(Date.now() - now);
})();

node-fetch fetch, 9.4 seconds, 28909 buffers

await (async () => {
    const fetch = require('node-fetch');
    const response = await fetch('https://alpha.notsobot.com/api/undici/test');
    let now = Date.now();
    const buffers = [];
    for await (let x of response.body) {
        buffers.push(x);
    }
    console.log(Date.now() - now);
    now = Date.now();
    JSON.parse(Buffer.concat(buffers));
    console.log(Date.now() - now);
})();

(Also, doing .arrayBuffer() yields the same results)

Expected Behavior

Similar timing to undici's request function

Logs & Screenshots

Environment

image

Ubuntu 22.10, Node v19.8.1 and v18.7.0, undici v5.2.0 and v5.21.0

Additional context

@cakedan cakedan added the bug Something isn't working label Mar 17, 2023
@KhafraDev
Copy link
Member

This is unfortunately an issue with using webstreams vs. node streams. Node streams perform much better than webstream currently.

@ronag ronag closed this as completed Mar 26, 2023
@KhafraDev KhafraDev reopened this Mar 27, 2023
@KhafraDev
Copy link
Member

KhafraDev commented Mar 27, 2023

If we don't decompress the response, it takes 4.4 seconds. If we do (using zlib.createBrotliDecompress()) it takes ~70 seconds.

@KhafraDev
Copy link
Member

... and if we re-assign this.body to the result of pipeline, it fixes the issue, but causes issues with invalid gzipped/brotli compressed bodies.

@ronag you know much more about streams than I do, do you have any idea?

@ronag
Copy link
Member

ronag commented Apr 14, 2023

... and if we re-assign this.body to the result of pipeline, it fixes the issue, but causes issues with invalid gzipped/brotli compressed bodies.

Not sure I follow.

@KhafraDev
Copy link
Member

So for example, this diff fixes the issue:

diff --git a/lib/fetch/index.js b/lib/fetch/index.js
index 0b2e3394..14e84b29 100644
--- a/lib/fetch/index.js
+++ b/lib/fetch/index.js
@@ -2023,7 +2023,7 @@ async function httpNetworkFetch (
             status,
             statusText,
             headersList: headers[kHeadersList],
-            body: decoders.length
+            body: this.body = decoders.length
               ? pipeline(this.body, ...decoders, () => { })
               : this.body.on('error', () => {})
           })

but then zlib/brotli tests fail (only tested on the node-fetch tests but it's safe to assume others will fail too)

@ronag
Copy link
Member

ronag commented Apr 14, 2023

I think it just removes the decompression...?

@KhafraDev
Copy link
Member

yeah it does, but what I don't understand is why it's causing an issue here, but not with node-fetch. Node-fetch uses pipeline & zlib too.

@ronag
Copy link
Member

ronag commented Apr 14, 2023

Webstreams...

@KhafraDev
Copy link
Member

I thought so too (made an issue in the performance repo), but considering that removing the decompression fixes the issue...?

@ronag
Copy link
Member

ronag commented Apr 14, 2023

Can be that web streams have smaller chunks which makes the decompression much less efficient... or something

@ronag
Copy link
Member

ronag commented Apr 14, 2023

I don't think this is something we can solve in undici.

@KhafraDev
Copy link
Member

web streams have smaller chunks

No, I don't think so? In the OP node-fetch has 10k more chunks than undici.fetch does. Adjusting the highwatermark/size didn't make much difference if I'm remembering correctly

@ronag
Copy link
Member

ronag commented Apr 14, 2023

🤷‍♂️

@KhafraDev
Copy link
Member

KhafraDev commented May 16, 2023

@cakedan do you have a repro that runs locally, without the external server? I can't seem to replicate the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants