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

[Regression] #fetch() is now returning generic terminated error message in some cases #1140

Closed
ghost opened this issue Dec 11, 2021 · 5 comments
Labels

Comments

@ghost
Copy link

ghost commented Dec 11, 2021

Bug Description

It seems like #fetch() is returning different (i.e. less useful) error messages starting in 4.11.2. I'm not exactly sure which change caused it or if it was intentional, but it's now just returning a super generic terminated error message in some cases. Is there any way to access the root cause / detailed description of getaddrinfo ENOTFOUND not-real-subdomain.github23.com in the below example?

Reproducible by

  1. npm install [email protected]
  2. Open up Node REPL
  3. Run the following code: require('undici').fetch('https://not-real-subdomain.github23.com').catch(console.log)

Pre-4.11.2 behavior

TypeError: fetch failed
    at Object.processResponse (/<redacted>/node_modules/undici/lib/fetch/index.js:176:23)
    at fetchFinale (/<redacted>/test/node_modules/undici/lib/fetch/index.js:720:17)
    at EventEmitter.mainFetch (/<redacted>/test/node_modules/undici/lib/fetch/index.js:695:5)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  cause: Error: getaddrinfo ENOTFOUND not-real-subdomain.github23.com
      at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26)
      at GetAddrInfoReqWrap.callbackTrampoline (node:internal/async_hooks:130:17) {
    errno: -3008,
    code: 'ENOTFOUND',
    syscall: 'getaddrinfo',
    hostname: 'not-real-subdomain.github23.com'
  }
}

Post-4.11.2 behavior

TypeError: fetch failed
    at Object.processResponse (/<redacted>/test/node_modules/undici/lib/fetch/index.js:175:23)
    at fetchFinale (/<redacted>/test/node_modules/undici/lib/fetch/index.js:717:17)
    at EventEmitter.mainFetch (/<redacted>/test/node_modules/undici/lib/fetch/index.js:692:5)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  cause: Error: terminated
      at EventEmitter.onRequestAborted (/<redacted>/test/node_modules/undici/lib/fetch/index.js:1507:51)
      at EventEmitter.emit (node:events:390:28)
      at EventEmitter.emit (node:domain:537:15)
      at EventEmitter.terminate (/<redacted>/test/node_modules/undici/lib/fetch/index.js:92:12)
      at Object.onError (/<redacted>/test/node_modules/undici/lib/fetch/index.js:1815:24)
      at Request.onError (/<redacted>/test/node_modules/undici/lib/core/request.js:212:27)
      at errorRequest (/<redacted>/test/node_modules/undici/lib/client.js:1830:13)
      at onError (/<redacted>/test/node_modules/undici/lib/client.js:1056:7)
      at connect (/<redacted>/test/node_modules/undici/lib/client.js:1223:7)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
}

Environment

[email protected]

@ghost ghost added the bug Something isn't working label Dec 11, 2021
@ronag
Copy link
Member

ronag commented Dec 12, 2021

We follow the spec quite literally. fetch is not very good at forwarding error causes. You have the same problem in Chrome. We do implement diagnostic channels which might help getting more useful information.

Otherwise, if you are using node and don't need web compat I would recommend you use the request API.

@ronag ronag added fetch and removed bug Something isn't working labels Dec 12, 2021
@ronag ronag closed this as completed in ce6ee81 Dec 12, 2021
ronag added a commit that referenced this issue Dec 12, 2021
ronag added a commit that referenced this issue Dec 12, 2021
@ghost
Copy link
Author

ghost commented Dec 12, 2021

@ronag - Can you confirm whether these types of changes are considered bugs/regressions in terms of version bumps? Even though this change seemingly aligned with Chrome's implementation, it still seems to be a borderline breaking change.

Would you also happen to have a link to the spec where it states the error message must be XYZ? I understand this project's goal of matching the behavior of Chrome, but intentionally making the errors less useful seems odd to me. Matching the error types, codes, etc. all makes sense, but I'm having a hard time how returning back a string of "terminated" would ever be a preferred state.

Anyways, I am using node so it looks like switching from fetch to request is maybe the simplest option, so appreciate the suggestion there. Thanks.

EDIT: With a little more time to think...

There are basically two "specs" here. The first is the fetch spec itself, which as far as I'm aware doesn't prevent additional things from being exposed (e.g. cause property of error how this was previously being surfaced). The second is more an implementation of the spec in terms of Chrome's behavior. But similar to the fetch spec itself, I don't see why returning more information or additional information would break anything or be seen as not aligned with Chrome.

While I understand the stated goals of trying to stay aligned with Chrome, I suppose my confusion is more focused on certain changes like this that, at least at first glance, are almost objectively worse behavior/functionality at least in a practical sense. This looks like a case of adherence to Chrome for sake of adherence to Chrome without any real benefits.

@ronag
Copy link
Member

ronag commented Dec 13, 2021

Anything that is beyond the spec is IMO not breaking. Anyway, I don't know if you noticed. We've actually fixed this now so that the error is forwarded in cause. I can cut a release for you it it's urgent.

@ronag
Copy link
Member

ronag commented Dec 13, 2021

If you want please open a PR to test for this behaviour and that way we are less likely to change this again.

@ghost
Copy link
Author

ghost commented Dec 13, 2021

Thanks for the fix, and not urgent, but appreciate the offer. I'll definitely make a mental note to see if I can loop back to this sometime in January to add a few tests. Thanks!

KhafraDev pushed a commit to KhafraDev/undici that referenced this issue Jun 23, 2022
metcoder95 pushed a commit to metcoder95/undici that referenced this issue Dec 26, 2022
crysmags pushed a commit to crysmags/undici that referenced this issue Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant