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

TypeError: fetch failed #1271

Closed
minas90 opened this issue Mar 10, 2022 · 13 comments · Fixed by #1276
Closed

TypeError: fetch failed #1271

minas90 opened this issue Mar 10, 2022 · 13 comments · Fixed by #1276
Labels
bug Something isn't working

Comments

@minas90
Copy link

minas90 commented Mar 10, 2022

Bug Description

Fetch fails with some URLs (after redirect?).

Reproducible By

let res = await fetch('https://www.luzernerzeitung.ch/kultur/literatur-im-pool-ld.2260742');

Expected Behavior

It should get the URL without any exception. The same code works just fine with 'node-fetch'.

Logs & Screenshots

    at Object.processResponse (node:internal/deps/undici/undici:7081:34)
    at Fetch.fetchFinale (node:internal/deps/undici/undici:7411:21)
    at Fetch.mainFetch (node:internal/deps/undici/undici:7305:21)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  cause: Error: incorrect header check
      at Zlib.zlibOnError [as onerror] (node:zlib:189:17) {
    errno: -3,
    code: 'Z_DATA_ERROR'
  }
}

Environment

macOS 11.6.4, Node v17.6.0

Additional context

The provided URL redirects and adds ?reduced=true at the end of the URL:
https://www.luzernerzeitung.ch/kultur/literatur-im-pool-ld.2260742?reduced=true
When I run the same code with this redirected URL, it works fine.

I guess a little tweak with the gzip code or redirect logic should fix this.

@minas90 minas90 added the bug Something isn't working label Mar 10, 2022
@KhafraDev
Copy link
Member

KhafraDev commented Mar 11, 2022

The error originates at

decoders.push(zlib.createGunzip())
because we get a content-encoding header that is gzip.

@mcollina
Copy link
Member

It seems the actual content is not gzipped.

@minas90
Copy link
Author

minas90 commented Mar 12, 2022

Yeah, before redirect the content size is 142 bytes and the gzipped size is 2x more. So maybe the server has a check to send the original content if the size is smaller. I think fetch should not get the content before redirect, it's waste of recourses.

➜  ~ curl "https://www.luzernerzeitung.ch/kultur/literatur-im-pool-ld.2260742" --silent --write-out "%{size_download}\n" --output /dev/null                                         
142
➜  ~ curl "https://www.luzernerzeitung.ch/kultur/literatur-im-pool-ld.2260742" --silent -H "Accept-Encoding: gzip,deflate" --write-out "%{size_download}\n" --output /dev/null 
142
<html>
<head><title>302 Found</title></head>
<body>
<center><h1>302 Found</h1></center>
<hr><center>openresty</center>
</body>
</html>

After redirect it is gzipped, you can check with curl.

➜  ~ curl "https://www.luzernerzeitung.ch/kultur/literatur-im-pool-ld.2260742?reduced=true" --silent --write-out "%{size_download}\n" --output /dev/null
323859
➜  ~ curl "https://www.luzernerzeitung.ch/kultur/literatur-im-pool-ld.2260742?reduced=true" --silent -H "Accept-Encoding: gzip,deflate" --write-out "%{size_download}\n" --output /dev/null
66129

@mcollina
Copy link
Member

Ah, I understand now. Basically we need to avoid parsing the body for the redirect. Would you like to send a PR?

@mcollina mcollina added the good first issue Good for newcomers label Mar 12, 2022
@ronag
Copy link
Member

ronag commented Mar 12, 2022

I’m not sure it’s spec compliant to not read the body? Or is a body on a redirect even allowed?

@targos
Copy link
Member

targos commented Mar 12, 2022

Chrome, Firefox and Safari all have no issue with the reproduction code and follow the redirection.
Example in Chrome:
image

@minas90
Copy link
Author

minas90 commented Mar 12, 2022

@mcollina I'm still not familiar with the codebase, if you can fix it quickly please go ahead. Otherwise I will push a PR in a few days when I will have some free time.

@ronag It seems like browsers ignore the body when status is 302. It's not even mandatory, maybe only needed for browsers which can't handle redirect?

https://stackoverflow.com/questions/8059662/http-302-redirect-is-a-message-body-needed

@ronag
Copy link
Member

ronag commented Mar 12, 2022

I think we should read the body but just ignore the content so that we can keep the connection alive.

@minas90
Copy link
Author

minas90 commented Mar 12, 2022

Btw, it's an unlikely scenario, but it's possible to have the same issue with gzip handling, when body size is small and the server has some logic to avoid gzip for small body. For example responses from API calls, such as simply ok

@ronag
Copy link
Member

ronag commented Mar 12, 2022

For non-fetch undici ignores the body. Not sure yet where it goes in fetch.

@ronag
Copy link
Member

ronag commented Mar 12, 2022

This is actually quite tricky to fix.

@ronag ronag closed this as completed in da9950b Mar 12, 2022
@ronag ronag reopened this Mar 12, 2022
ronag added a commit that referenced this issue Mar 12, 2022
Don't start prefetching the body before headers and status
has been processed.

Fixes: #1271
ronag added a commit that referenced this issue Mar 12, 2022
Don't start decoding the body before headers and status
has been processed.

Fixes: #1271
ronag added a commit that referenced this issue Mar 12, 2022
Don't start decoding the body before headers and status
has been processed.

Fixes: #1271
@ronag
Copy link
Member

ronag commented Mar 12, 2022

Going back to the spec:

For e.g. HEAD and CONNECT the spec says:

  1. If response is not a network error and either request’s method is
    HEAD or CONNECT, or internalResponse’s status is a null body status,
    set internalResponse’s body to null and disregard any enqueuing toward
    it (if any).

But there is nothing along those lines for redirects. Is this a spec bug? Since Chrome does seem to disregard the body in thise case. @annevk

@ronag ronag removed the good first issue Good for newcomers label Mar 12, 2022
@annevk
Copy link

annevk commented Mar 14, 2022

Redirects can have a response body. I don't think it ever ends up being exposed though.

ronag added a commit that referenced this issue Mar 19, 2022
* fix: lazy decode body

Don't start decoding the body before headers and status
has been processed.

Fixes: #1271

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup
KhafraDev pushed a commit to KhafraDev/undici that referenced this issue Jun 23, 2022
* fix: lazy decode body

Don't start decoding the body before headers and status
has been processed.

Fixes: nodejs#1271

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup
metcoder95 pushed a commit to metcoder95/undici that referenced this issue Dec 26, 2022
* fix: lazy decode body

Don't start decoding the body before headers and status
has been processed.

Fixes: nodejs#1271

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup
crysmags pushed a commit to crysmags/undici that referenced this issue Feb 27, 2024
* fix: lazy decode body

Don't start decoding the body before headers and status
has been processed.

Fixes: nodejs#1271

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup
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

Successfully merging a pull request may close this issue.

6 participants