Skip to content

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented May 23, 2020

AbortController impl based very closely on:
https://github.com/mysticatea/abort-controller

Marked experimental.
Global (writable, configurable)
Not currently used by any of the existing promise apis.
AbortSignal extends from EventEmitter instead of EventTarget.

constac=newAbortController();ac.onabort=()=>{};// Or ...ac.on('abort',()=>{});ac.abort();console.log(ac.aborted);
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 23, 2020
@jasnelljasnellforce-pushed the abort-controller branch 2 times, most recently from 7544a2d to 29b247cCompareMay 23, 2020 03:34
@benjamingr
Copy link
Member

That's awesome, I'm going to check this out and play with it asap.

Also post it on the summit issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr
Copy link
Member

Other than test and review this is there anything I can do to help?

@jasnell
Copy link
MemberAuthor

Other than test and review this is there anything I can do to help?

One of the next steps is to identify a list of the existing Promise APIs in core that could by updated to make use of this. Not all of them will make sense because not all of them are abortable (e.g. libuv only allows us to cancel file system requests if they have not already been started, for instance). So while we may be able to use AbortController for fs.promises.readFile() (which performs a sequence of discreet read operations) it won't for fs.promises.read() (which performs a single discreet read operation). Once we have a list of APIs for which use of AbortController makes sense, then we need to figure out the best way to incorporate it. Each will have their own set of considerations to address.

Additionally, we should make it possible for custom util.promisify implementations to support AbortController. For instance, I'm playing around with the promisification of setTimeout such that it allows:

constsleep=promisify(setTimeout);constac=newAbortController();sleep(10000,ac);ac.abort();// Calls clearTimeout on abort

@benjamingr
Copy link
Member

@jasnell I've split those tasks off into an issue here: #33528 - please feel free to edit that issue as you see fit.

I will start working on these one by one (on weekends mostly) and add tasks as I find them. Feel free to edit it.

I want to be clear that I want to be helpful here so since you took the initiative if at any point any of the things I'm doing are frustrating just tell me and I'll stop and do whatever is helpful to the effort instead. These sorts of ongoing efforts tend to become frustrating in Node.js at times, I want to be sure that I'm not doing any toe-stepping if I can help it.

@jasnell
Copy link
MemberAuthor

Initial implementation of EventTarget in #33556. Assuming that one goes through, this PR will be modified such that AbortController.prototype.signal is an instance of EventTarget rather than EventEmitter.

@jasnelljasnellforce-pushed the abort-controller branch 2 times, most recently from 02953f1 to 7290192CompareMay 26, 2020 22:46
@jasnell
Copy link
MemberAuthor

Updated based on the #33556

@jasnell
Copy link
MemberAuthor

@benjamingr ... quick note... this is currently extending from NodeEventTarget ... in this, however, the remove listener functions are not used and the only ones we definitely need here are the on/once/addListener in order to achieve the use cases we need for AbortSignal

jasnell added a commit that referenced this pull request May 28, 2020
See documentation changes for details Signed-off-by: James M Snell <[email protected]> PR-URL: #33556 Refs: #33527 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
@jasnelljasnellforce-pushed the abort-controller branch 2 times, most recently from a7b3dd8 to 890a585CompareMay 28, 2020 14:29
@jasnelljasnell marked this pull request as ready for review May 28, 2020 14:31
@jasnelljasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label May 28, 2020
@jasnell
Copy link
MemberAuthor

Marking this ready for review. This does create a new global so technically may be semver-major but I have tested it with existing ecosystem polyfills and haven't seen any breakage at all. Unless there are objections from the @nodejs/tsc, I'd like to handle this as a semver-minor.

It is still experimental and an experimental warning will be emitted the first time an AbortController is created. It is not behind a flag.

codebytere added a commit to electron/electron that referenced this pull request May 18, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / srcIssues and PRs related to general changes in the lib or src directory.semver-majorPRs that contain breaking changes and should be released in the next major version.tsc-agendaIssues and PRs to discuss during the meetings of the TSC.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@jasnell@benjamingr@nodejs-github-bot@mcollina@richardlau@targos@mhdawson@zcbenz@addaleax@ptomato@devsnek@BridgeAR@lundibundi@felixfbecker@Ethan-Arrowood