Add abort reason to AbortSignal

[nidhijaju]

Based on the discussion in whatwg/streams#1165, it was suggested that we add an abort reason property to AbortSignal. This PR makes the initial changes to the base primitives in the DOM spec, before we make changes to all other related specs that use the AbortSignal.

@domenic Is it okay if you please help take a look?
/cc @yutakahirano

• At least two implementers are interested (and none opposed):
  • Google
  • Mozilla
• Tests are written and can be reviewed and commented upon at:
  • DOM: add tests for AbortSignal.reason web-platform-tests/wpt#31291
• Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1263410
  • Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1737771
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=232299

---

Preview | Diff

[yutakahirano]

With setting nits aside, I have a question - when no reason is given to AbortController.abort(), should AbortController.reason be undefined, or an AbortError? I think undefined is simpler and I prefer the option if there is no reason to create a new AbortError.

[domenic]

With setting nits aside, I have a question - when no reason is given to AbortController.abort(), should AbortController.reason be undefined, or an AbortError? I think undefined is simpler and I prefer the option if there is no reason to create a new AbortError.

I think it's important to create a new AbortError so that author code can do something like

async function doSomething({ signal }) {
  if (signal.aborted) {
    throw signal.reason;
  }
}

to propagate the error as expected. This is also how I would expect us to write the other specs, e.g. Fetch would say something like "If signal is aborted, then reject promise with signal's abort reason."

[domenic]

Hmm, I wonder if the property should be signal.reason or signal.abortReason... Given that AbortSignal is being used as a base class for things like TaskSignal, and that the variable is usually named signal instead of abortSignal, spelling it out as abortReason might be a good idea.

[nidhijaju]

I don't have strong opinions about signal.reason vs signal.abortReason, but I was thinking that if someone uses an AbortSignal object, despite it being called signal, they would still know it is related to "aborting" which is why I thought signal.reason might be okay. However, if you think signal.abortReason is clearer, then I can change it to that too 😊

[domenic]

The main logic looks great now.

I'm still interested to gather more opinions on reason vs. abortReason... maybe @jasnell can help represent other-ecosystems perspectives.

[jasnell]

This LGTM. It's a good idea. Having the reason default to an AbortError makes sense but I don't think it's entirely necessary.

I'll add this to both Node.js and Workers.

[nidhijaju]

Thank you @jasnell! Do you have any opinions on the naming of the abort reason by the way (i.e. signal.reason vs. signal.abortReason)?

[jasnell]

I think just 'reason' for the name works

[annevk]

Thanks, this looks good except for some nits.

I think it would be good to have some of the follow-up PRs against other specifications, e.g., Fetch, ready as well (at least in draft state) to ensure it works out and the complete flow can be reviewed.

[annevk]

Looks good! Excited for this as this will enable timeouts as well. As I mentioned above I think we should have the PRs against the other specifications as well (CI will complain, but that's okay) to get the full picture.

[nidhijaju]

Thank you @annevk! I am working on the Streams and Fetch PRs so hopefully I can have that up soon.