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

cryptic error message for already aborted signals #2242

Closed
jimmywarting opened this issue Sep 2, 2023 · 4 comments · Fixed by #2243 or #2833
Closed

cryptic error message for already aborted signals #2242

jimmywarting opened this issue Sep 2, 2023 · 4 comments · Fixed by #2243 or #2833
Labels
enhancement New feature or request

Comments

@jimmywarting
Copy link
Contributor

jimmywarting commented Sep 2, 2023

Undici have the most crypting error message when sending in an already aborted signal.

example:

fetch('http://localhost', { signal: AbortSignal.abort('Already aborted') })
  .catch(e => console.log({ctor: e.constructor.name, code: e.code, message: e.message}))

undici:

{ ctor: 'TypeError', code: undefined, message: 'invalid_argument' }

Chrome:

{ ctor: 'DOMException', code: 20, message: "Failed to execute 'fetch' on 'Window': The user aborted a request."}

Safari:

{ ctor: "DOMException", code: 20, message: "Request signal is aborted"}

Firefox & Deno:

{ ctor: "String", code: undefined, message: undefined }
// returns the abort reason, so the result is "Already aborted"

// With an undefined reason it throw
{ ctor: "DOMException", code: 20, message: "The operation was aborted. " }

i tough i sent in something really wrong type when i was calling fetch when i received a TypeError.
but it's of a correct "type".

{ signal: "foo" } is an TypeError and a invalid argument.

@jimmywarting jimmywarting added the enhancement New feature or request label Sep 2, 2023
@jimmywarting jimmywarting changed the title cryptic error message. cryptic error message for already aborted signals Sep 2, 2023
@ronag
Copy link
Member

ronag commented Sep 2, 2023

@KhafraDev

@KhafraDev
Copy link
Member

KhafraDev commented Sep 2, 2023

the error comes from

undici/index.js

Line 109 in 68e2275

Error.captureStackTrace(err, this)
but I think undici would be in the right here by throwing the signal's reason "Already aborted" because of the first 4 steps of https://fetch.spec.whatwg.org/#dom-global-fetch. I think firefox's behavior is right here, idk.

@jimmywarting
Copy link
Contributor Author

I guess firefox and Deno's behavior is also right.
It have more value by rejecting with the signals own reason... so that you know why it got aborted.

reason why you get a DOMException when undefined is passed is b/c:
AbortSignal.abort().reason.constructor === DOMException

@KhafraDev
Copy link
Member

KhafraDev commented Sep 2, 2023

it was the correct behavior before a spec change that made requests abort with the signal's reason rather than the standard error from abortsignals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants